[MERGE][#230902][1.8] Fix “Must end write group ...” error due to error raised in a finally block

[Andrew Bennetts]

This patch fixes <https://bugs.launchpad.net/bzr/+bug/230902> by taking
care to propagate a prior error from _basic_push (if there is one), even
if calling target.unlock() raises a new error.  I think this change
would be good to get in 1.8.

The basic problem is that Python makes it awkward to write code that
runs unconditionally *and* that doesn't stomp on an live exception.  In
lots of places we have code like:

    some_branch.lock_write()
    try:
        do_something_with(some_branch)
    finally:
        some_branch.unlock()

Unfortunately if “do_something_with(some_branch)” raises it's fairly
common for the unlock to fail too — but the failure from the unlock is
less informative than the original failure.  It's helpful to tell users
things like “connection lost”, and not so helpful to tell them “internal
invariant violated”, which is essentially what “Must end write group
...” is.

This is the same design issue that has caused all the bug reports
involving TooManyConcurrentRequests.

Ideally Python would provide a way to say “do X and then finally Y, but
if X raised an exception it gets priority”, but it doesn't.  Checking
sys.exc_info() in the finally block is no good, because there may be old
exceptions that weren't raised by the try block there.  Even
introspecting the traceback of sys.exc_info() can't reliably tell us if
it was *this* instance of the try/finally construct (consider a
try/finally inside a loop), and would be ugly anyway.

The robust way to do this in Python is in this patch.  Because it is
considerably longer than just try/finally, I've put it in a function.
So the correct way to do the above example, assuming you want exceptions
from do_something_with in preference to exceptions from unlock, becomes:

    _run_with_write_locked_target(
        some_branch, do_something_with, some_branch)

_run_with_write_locked_target is defined in my patch.  This isn't
beautiful, but it is correct.  I've written tests for this helper to
demonstrate that it does the right thing in all cases.

Unfortunately this helper isn't easily applicable everywhere either.
But this change is fairly minimal and demonstrably correct, and I think
it's appropriate for 1.8, so rather than bloat this fix I'll follow up
with a separate patch proposing a different helper, and show how it
applies to commit.py.

-Andrew.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: push-unlock.patch
Type: text/x-diff
Size: 8730 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20081008/615ff4cf/attachment-0001.bin