[RFC] Add run_cleanup, do_with_cleanup

[Andrew Bennetts]

[Trying an RFC for a branch by cross-posting to both the merge proposal on
Launchpad, and the bazaar@ list.]

Hi all,

Here's the problem.  In this example, Python gives no way for final_func to know
if try_func raised an error or not:

    try:
        try_func()
    finally:
        final_func()

Not even by sys._getframe hacks, AFAICT.  I can go into some gory details about
why not, but more important is what to do about it.

It's problematic because, in general, we don't want an error from final_func to
override an error from try_func.  This means that sometimes users get errors
about, say, abort_write_group failing, when what originally went wrong was
“connection lost”.  The various TooManyConcurrentRequests bugs are the most
common symptom of this.

This brings me to <https://bugs.launchpad.net/bzr/+bug/429747>, and
<https://code.launchpad.net/~spiv/bzr/cleanup-hof/+merge/12318>.

Bug 429747 proposes a higher-order-function that all cleanups should use to at
least give us a common place to control policy about log vs. raise, etc.  That
branch implements that, as a function called run_cleanup (and some variations).
It lives in a new module, bzrlib.cleanups.  I have tried to give the new module
and its contents fairly reasonable docstrings, and clear tests.

The downside of run_cleanup is that it will suppress some exceptions that should
be propagated, but that seems to be unavoidable given the limitations of Python.
(And note that developers can always use -Dcleanup to get UI warnings about
failures in run_cleanup).

The branch also implements something called do_with_cleanups, which can actually
be pretty robust and correct, at the cost of making simple callsites a bit
uglier.  I have a follow-on patch that adjusts bzrlib.commit to use this, which
would fix some outstanding bugs.

So, here's the RFC: I think the policy should be:

 * use run_cleanup if a cleanup failure is likely to be unimportant to a user
   (or at least, the chance of it being important is < the chance of the main
   func raising an error that should not be obscured).
 * consider changing the cleanup function itself to never raise errors, e.g.
   abort_write_group(suppress_error=True).  Then idiomatic try/finally is safe.
   Again, only applicable if failures during cleanup are "uninteresting".
 * use run_cleanups if you are running multiple cleanups; it takes care of
   running them all even if an exception happens.
 * if you want very correct behaviour, use do_with_cleanups.  This tends to make
   simple callsites a bit uglier, but in already complex code (like commit) it
   may be no worse.  

If agreed, I can add this to HACKING.txt.

I don't think we necessarily need to update all existing try/finally blocks at
once, but we should at least fix the cases that have been the source of bug
reports.

-Andrew.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090924/951a46cc/attachment.pgp