Use try...finally... to ensure buffer is released

[jakirkham]

Follow up on this discussion: #692 (comment)

Ensure that the acquired buffer is properly released even if an exception is raised. Given we may raise an exception, this is important to check.

cc @vyasr

[jakirkham]

Tried to call out the key points of this change below and why they matter

Since a block of code is indented the GitHub diff looks more involved than it actually is

So hopefully this clarifies the nature of the change

[jakirkham]

The core of this change involves adding a try for the block of code right after PyObject_GetBuffer

[jakirkham]

Then moving the PyBuffer_Release into a finally block

That way we still release the underlying Py_buffer object if an exception is raised

[jakirkham]

For example here is one exception we raise

Though it is also possible to wind up with an exception due to some bug in the code below

In any event we want to make sure normal resource cleanup can proceed as usual (without potentially causing segfaults)

[vyasr]

Ah OK this is fine. #692 (comment) made it sound like you wanted the PyObject_GetBuffer call itself inside a try-except block, which didn't make sense to me since it wouldn't be throwing if that failed. I agree that this change makes sense to avoid leaking the buffer if anything fails in the interim.

[vyasr]

/merge

[jakirkham]

Thanks Vyas! 🙏

Yeah can understand how what I said there was unclear. Probably I should have commented on the release line or drew a clearer connection between the acquisition & release as I did here

[jakirkham]

Should have clarified one other point

...the PyObject_GetBuffer call itself inside a try-except block, which didn't make sense to me since it wouldn't be throwing if that failed.

PyObject_GetBuffer does raise if it fails. From the docs:

If the exporter cannot provide a buffer of the exact type, it MUST raise BufferError, set view->obj to NULL and return -1.

That said, I don't think we need to do anything special in this case as Py_buffer remains undefined and obj will go through normal ref-count handling

[vyasr]

Should have clarified one other point

...the PyObject_GetBuffer call itself inside a try-except block, which didn't make sense to me since it wouldn't be throwing if that failed.

PyObject_GetBuffer does raise if it fails. From the docs:

If the exporter cannot provide a buffer of the exact type, it MUST raise BufferError, set view->obj to NULL and return -1.

Yes it can raise, but a Python try-except block is not going to do anything to catch an exception raised by a CPython function. You have to check the return value and then manually reraise, hence why I was confused by the try-except suggestion. We could update the code to do that, but I think we know that we never call this function with an input that will raise so I didn't bother to do that here.