[Cython] Error handling during buffer reassignment

[Stefan Behnel]

Robert Bradshaw schrieb am 08.09.2017 um 06:36:
> On Wed, Sep 6, 2017 at 3:53 AM, Stefan Behnel wrote:
>> consider this code:
>>
>>     cdef np.ndarray[int32, ndim=1] a
>>     a = new_int32_buffer()
>>     a = new_float32_buffer()
>>
>> This fails during the second assignment, but only during validation, after
>> unpacking the buffer. The code that handles this case is generated here:
>>
>> https://github.com/cython/cython/blob/c95ca9f21a3524718a83c3415bb7102a508154be/Cython/Compiler/Buffer.py#L376
>>
>> Note the twist where it says:
>>
>>         # If acquisition failed, attempt to reacquire the old buffer
>>         # before raising the exception.
>>
>> I faintly remember a discussion about that case (apparently back in 2008),
>> but can't find it in my mail archive. Might have been off-list at the time.
>>
>> Question: Instead of re-acquiring the buffer (and thus faking the
>> non-assignment), wouldn't it be better to acquire the new buffer in a temp,
>> and only overwrite the old "Py_buffer" if all is fine with it?
> 
> That makes a lot more sense to me, assuming overwriting is cheap enough.

It's just a Py_buffer struct copy.


> Can an error happen in releasing the old value?

No, releasing buffers is guaranteed to succeed (or rather, to not fail).

While working on this, I noticed that there is a difference. Previously,
the acquire-release order was:

   1) acquire initial buffer
   2) on reassign, release old buffer
   3) acquire new buffer
   4) on failure, re-aqcuire old buffer

Now the order is:

   1) acquire initial buffer
   2) on reassign, acquire new buffer
   3) on success, release old buffer

Note that the original code never holds two buffers at the same time. I
think the old way might have been optimised for that, i.e. it expected
success and tried to minimise the potential resource usage, assuming that
acquiring and holding a buffer might bind resources of the owner.

It's unclear to me whether that assumption holds, but it makes me more
hesitant to change the currect implementation.

Stefan