Merge lp:~jameinel/bzr/2.0.4-update-exception-495023 into lp:bzr/2.0

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.

 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')

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 :)

> === 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.

-----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 s...

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

...

> 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:

So I'll go ahead and land this as is (modulo NEWS, etc). And then I'll
prepare a bigger patch.

For example, _simple_set_pyx.pyx doesn't *exist* in the 2.0 series :).

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

iEYEARECAAYFAktCaYUACgkQJdeBCYSNAAMIgwCgqmnl6Fh/ZNFhoTS8GwIz2uia
bqoAoISGG2zo75VS0cs8Xc3HHkc3PZ/9
=2j2Y
-----END PGP SIGNATURE-----

John A Meinel wrote:
[...]
> However, some *cannot* raise exceptions and the overhead of checking the
> return value is unwanted.

Hmm. I guess what I'd really like is if Pyrex required you to
explicitly opt-out of exception checking, rather than
on-by-default-or-opt-in-depending-on-return-type. e.g. a syntax like
“cdef int frobnicate(...) cannot_raise:”. Absence of cannot_raise would
imply “except *” (or that the function returns object, etc).

I guess we can manually annotate these cases with “# cannot_raise” if we
check for this with test_source.

-Andrew.

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

Andrew Bennetts wrote:
> John A Meinel wrote:
> [...]
>> However, some *cannot* raise exceptions and the overhead of checking the
>> return value is unwanted.
>
> Hmm. I guess what I'd really like is if Pyrex required you to
> explicitly opt-out of exception checking, rather than
> on-by-default-or-opt-in-depending-on-return-type. e.g. a syntax like
> “cdef int frobnicate(...) cannot_raise:”. Absence of cannot_raise would
> imply “except *” (or that the function returns object, etc).
>
> I guess we can manually annotate these cases with “# cannot_raise” if we
> check for this with test_source.
>
> -Andrew.
>

Yeah, I'm doing something like that in
  lp:///~jameinel/bzr/2.0.4-pyrex-propagation

Even better if we had smarts so that a 'cannot_raise' function could
only call other 'cannot_raise' functions :)

John
=:->

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

iEYEARECAAYFAktCffEACgkQJdeBCYSNAAOLTACgzSdGvDgSv+4Gc+ugh5cYQLD+
ZdYAoJqXWPq7CIGaecShVH6pqyJRt3X/
=MjOk
-----END PGP SIGNATURE-----

John A Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
> ...
>
> > 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:
>
> So I'll go ahead and land this as is (modulo NEWS, etc). And then I'll
> prepare a bigger patch.

Thanks, that was my intent. Sorry for not being clearer about that.

> For example, _simple_set_pyx.pyx doesn't *exist* in the 2.0 series :).

Right :)

-Andrew.