-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> Review: Approve
>  review approve
>  merge approved
> 
> John A Meinel wrote:
>> John A Meinel has proposed merging lp:~jameinel/bzr/2.0.4-update-exception-495023 into lp:bzr/2.0.
>>
>>     Requested reviews:
>>     bzr-core (bzr-core)
>> Related bugs:
>>   #495023 Interrupting commit to smart server sometimes removes files
>>   https://bugs.launchpad.net/bugs/495023
>>
>>
>> This is a quick-and-simple fix for bug #495023. It just changes the
>> _update_current_block function so that it can raise an exception. The bug is
>> that we were using a C helper that didn't define an 'except' mechanism, which
>> then called into regular python code. Any pending exception probably gets
>> triggered at that point, but we then suppressed it.
>>
>> Andrew's fix probably helps, in that we poll for pending exceptions earlier.
>> However, it doesn't get at the root, just shrinks the available window.
> 
> This patch is good, but I wonder if it goes far enough?  It's closer to
> the root, but I don't think it's there yet.  I think it's good enough to
> solve the immediate bug, so probably fine to land as is, but there's
> more work to do:
> 
> $ grep -Irn 'cdef void[^*]*(' bzrlib/*.pyx
> bzrlib/_dirstate_helpers_pyx.pyx:1376:    cdef void _gather_result_for_consistency(self, result):
> bzrlib/_dirstate_helpers_pyx.pyx:1395:    cdef void _update_current_block(self):
> bzrlib/_knit_load_data_pyx.pyx:100:    cdef void validate(self):
> 
> So there are two other Pyrex functions that might suppress exceptions.
> The _knit_load_data_pyx one is particularly ironic:
> 
>     cdef void validate(self):
>         if not PyDict_CheckExact(self.cache):
>             raise TypeError('kndx._cache must be a python dict')
>         if not PyList_CheckExact(self.history):
>             raise TypeError('kndx._history must be a python list')

Nice. :) I do wonder if it is finally time to just nuke this extension.
I know Robert wanted to get rid of it a long time ago, but at that point
was just when 0.92 was introduced, which seemed a bit early.

> 
> Similiarly, grepping for __Pyx_WriteUnraisable in *.c files reveals that:
> 
>  * _simple_set_pyx's SimpleSet_Next suppresses errors (it returns int,
>    but lacks an except -1 that it ought to have).  Normal return values
>    are 0 or 1.
>  * _known_graph_pyx._MergeSortNode.has_pending_parents: ditto.
> 
> So I think we should add a test_source test for pyx files that checks for:
> 
>  * cdef void without except.
>  * cdef int without except.
> 
> And, of course, we should fix the specific instances I just listed :)

Any cdef function that has a defined return value has to have except
given. Only ones that return 'object' (which is also ones that don't
have a defined return) auto propagate exceptions.

However, some *cannot* raise exceptions and the overhead of checking the
return value is unwanted.

Manually going through the code and evaluating each 'cdef' function
would be reasonable.

> 
>> === modified file 'NEWS'
> [...]
>> +* ``_update_current_block`` no longer suppresses exceptions, so ^C at just
>> +  the right time will get propagated, rather than silently failing to move
>> +  the block pointer. (John Arbash Meinel, #495023)
> 
> The reporter, Gareth White, has been unusually responsive, patient and
> helpful given the difficulty of diagnosing this bug.  So I'd be inclined
> to take the unusual step of publically thanking him in the NEWS entry.
> 
> -Andrew.
> 
> 

Sure.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktCZpUACgkQJdeBCYSNAAMbqwCgwgaoddBB5rwByG/bMhDrezkh
KmIAoMoh02tIYusHWBARphzexXuoACE7
=OQdL
-----END PGP SIGNATURE-----