Merge lp:~spiv/bzr/cleanup-hof into lp:bzr

[Andrew Bennetts]

[Andrew Bennetts (spiv)]

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

[Martin Pool (mbp)]

2009/9/24 Andrew Bennetts <email address hidden>:
> [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 haven't read the diff yet, but we did talk about it ...)

I was originally suggesting that we should handle this by putting all
cleanup-type code through a single bottleneck. I think you're on to
something in saying (if you are) that we should instead put it through
common policies for different types of cleanup.

My sense is that this is more about the type of cleanup than the place
...

[Vincent Ladeuil (vila)]

>>>>> "Andrew" == Andrew Bennetts <email address hidden> writes:

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

    Andrew> Hi all,

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

    Andrew> try:
    Andrew> try_func()
    Andrew> finally:
    Andrew> final_func()

try:
   no_error = False
   try_func()
   no_error = True
finally:
   final_func(no_error)

[Andrew Bennetts (spiv)]

Martin Pool wrote:
> 2009/9/24 Andrew Bennetts <email address hidden>:
[...]
>
> (I haven't read the diff yet, but we did talk about it ...)
>
> I was originally suggesting that we should handle this by putting all
> cleanup-type code through a single bottleneck. I think you're on to
> something in saying (if you are) that we should instead put it through
> common policies for different types of cleanup.

Yes, I think that's what I'm getting at (the picture has been emerging
pretty gradually!).

> My sense is that this is more about the type of cleanup than the place
> it's being called from, so I'd expect that normally we will be
> changing the functions called at cleanup, not the code with the
> try/finally block. Of course if there is code with multiply nested
> try/finally blocks and we can make it cleaner by changing to a
> higher-order-function that runs them in order, that's great.

Right, that's a good example where a h-o-f can make the code cleaner, not
just more robust.

> I think we can distinguish different types of severity of failure.
> For instance, if we fail to unlock a branch, that's worth telling the
> user about, but it's bad to give them such a large or severe message
> that it obscures their view of what actually went wrong, as often
> seems to happen in bugs.
>
> One blue-sky option here would be to indicate success or failure by
> return code: unlock (say) returns True if it actually unlocked, False
> if it failed. Then code that really cares can see, but the default
> will be not to interrupt processing. This gives you a way to have
> failures that are 'slightly interesting'.

I don't particularly like checking return values. I think it might be nice
to be able to say "foo.unlock(on_error='warn')", or
on_error='log'/'ignore'/'raise'. I'm not sure which we'd like to have as
the default, and it's also questionable about whether we really want to
burden all unlock implementations with this, but it's an interesting
strawman API.

My thinking here is that *usually* an unlock failure is probably not
something to report to the user, but there are times when we definitely want
errors to propagate directly (e.g. an explicit invocation of break-lock, and
probably during the test suite).

So there's a mix of inputs here. I'm having trouble articulating this
clearly, but roughly I feel that what's desired in any individual case may
depend on a combination of:

 - the cleanup operation itself;
 - what the callsite intends (some places may explicitly expect and suppress
   particular errors, so emitting something to the UI instead of raising
   something the callsite can catch may be a bad thing);
 - any global policy, like debug flags;
 - and maybe also the specific error (e.g. KeyboardInterrupt vs. connection
   lost vs. an assertion about unfinished progress bars)?

The challenge seems to be balancing these desires while keeping things
reasonable clean and simple.

> It seems to me the most common requests are:
>
> * TooManyConcurrentRequests as a knock-on effect from a previous
> operation being interrupted either because of a client-side bug or
> error, or a connection dropping
> * Unlock failing because ...

[Andrew Bennetts (spiv)]

Vincent Ladeuil wrote:
[...]
> try:
> no_error = False
> try_func()
> no_error = True
> finally:
> final_func(no_error)

Ugh! And of course then you need to change final_func to reliably never
raise errors depending on that flag... and then there are cases with multiple
cleanups...

But yes, changing the try/finally somehow is the only option (using
do_with_cleanups from my patch is another variation on this). It's a
shame because it means the obvious spelling is not the correct one :(

-Andrew.

[Martin Pool (mbp)]

> Vincent Ladeuil wrote:
> [...]
> > try:
> > no_error = False
> > try_func()
> > no_error = True
> > finally:
> > final_func(no_error)
>
> Ugh! And of course then you need to change final_func to reliably never
> raise errors depending on that flag... and then there are cases with multiple
> cleanups...

Not to mention the possibility of UnboundLocalErrors - unlikely in this simple case, but still possible if you just happen to get interrupted at the right time.

But Vincent's suggestion does point out something interesting, which is that the "has an error occurred" or "have things finished correctly" is often already available in the program state without needing a specific variable to be added for it. In the case we're primarily looking at here: if unlock runs and the medium(?connection?) is still in use, then we can be pretty sure that we were in fact interrupted. Now what should it do?

We can decompose this into:
How do we detect when we're in a knock-on error case?
What do we do differently?

The assumption is that we want to suppress errors only when they would be knock-on errors. I think we do want to squelch them in that case, but perhaps for some of these errors, like failure to unlock, they don't need to be propagated as errors at any time (outside of testing etc).

[Martin Pool (mbp)]

2009/9/24 Andrew Bennetts <email address hidden>:
> Martin Pool wrote:
>> 2009/9/24 Andrew Bennetts <email address hidden>:
> [...]
>>
>> (I haven't read the diff yet, but we did talk about it ...)
>>
>> I was originally suggesting that we should handle this by putting all
>> cleanup-type code through a single bottleneck.  I think you're on to
>> something in saying (if you are) that we should instead put it through
>> common policies for different types of cleanup.
>
> Yes, I think that's what I'm getting at (the picture has been emerging
> pretty gradually!).
>
>> My sense is that this is more about the type of cleanup than the place
>> it's being called from, so I'd expect that normally we will be
>> changing the functions called at cleanup, not the code with the
>> try/finally block.  Of course if there is code with multiply nested
>> try/finally blocks and we can make it cleaner by changing to a
>> higher-order-function that runs them in order, that's great.
>
> Right, that's a good example where a h-o-f can make the code cleaner, not
> just more robust.
>
>> I think we can distinguish different types of severity of failure.
>> For instance, if we fail to unlock a branch, that's worth telling the
>> user about, but it's bad to give them such a large or severe message
>> that it obscures their view of what actually went wrong, as often
>> seems to happen in bugs.
>>
>> One blue-sky option here would be to indicate success or failure by
>> return code: unlock (say) returns True if it actually unlocked, False
>> if it failed.  Then code that really cares can see, but the default
>> will be not to interrupt processing.  This gives you a way to have
>> failures that are 'slightly interesting'.
>
> I don't particularly like checking return values.

OK, but why? Obviously in general in Python one should be using
exceptions rather than return code as the normal convention, but are
there any other drawbacks? The other main reason is that return codes
can easily be ignored but that is (perhaps) just what we want here.

One problem with return codes is that, if you return a boolean, you
don't get any explanation of what went wrong. We could potentially
return an Exception object, but that might be getting too weird.

> I think it might be nice
> to be able to say "foo.unlock(on_error='warn')", or
> on_error='log'/'ignore'/'raise'.  I'm not sure which we'd like to have as
> the default, and it's also questionable about whether we really want to
> burden all unlock implementations with this, but it's an interesting
> strawman API.

That seems ok, at least assuming that you expect different callers to
prefer to get errors, warnings, or nothing. But aside from tests
specifically for locking, I'm not sure that they will.

> My thinking here is that *usually* an unlock failure is probably not
> something to report to the user, but there are times when we definitely want
> errors to propagate directly (e.g. an explicit invocation of break-lock, and
> probably during the test suite).

I don't think break-lock calls unlock, it calls break_lock, which
presumably would always error.

> So there's a mix of inputs here.  I'm having troubl...

[Stephen Turnbull (stephen-xemacs)]

Andrew Bennetts writes:

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

In 2.6, you can do

try:
    errflag = "Impossible is nothing."
    try_func()
except:
    errflag = "Uh oh ..."
finally:
    final_func(errflag)

In earlier Pythae you must do the ugly but equivalent

try:
    errflag = "Impossible is nothing."
    try:
        try_func()
    except:
        errflag = "Uh oh ..."
finally:
    final_func(errflag)

I believe Guido acknowledged not allowing the more compact notation in
the first place to be excessive caution, but it's not impossible to
distinguish exceptions raised by try_func from those raised by
final_func.

So what's the problem? You want to do this without fixing all the
broken try ... finally blocks?

 > Not even by sys._getframe hacks, AFAICT. I can go into some gory
 > details about why not,

Inquiring minds want to know....

[Martin Pool (mbp)]

2009/9/24 Stephen J. Turnbull <email address hidden>:

> I believe Guido acknowledged not allowing the more compact notation in
> the first place to be excessive caution, but it's not impossible to
> distinguish exceptions raised by try_func from those raised by
> final_func.
>
> So what's the problem?  You want to do this without fixing all the
> broken try ... finally blocks?

It's not _impossible_, we just don't want to add boilerplate to
everything that uses a try/finally block. There are many more of them
than there are distinct types of cleanup to run.

--
Martin <http://launchpad.net/~mbp/>

[Andrew Bennetts (spiv)]

Martin Pool wrote:
> 2009/9/24 Stephen J. Turnbull <email address hidden>:
>
> > I believe Guido acknowledged not allowing the more compact notation in
> > the first place to be excessive caution, but it's not impossible to
> > distinguish exceptions raised by try_func from those raised by
> > final_func.
> >
> > So what's the problem?  You want to do this without fixing all the
> > broken try ... finally blocks?
>
> It's not _impossible_, we just don't want to add boilerplate to
> everything that uses a try/finally block. There are many more of them
> than there are distinct types of cleanup to run.

Especially when the boilerplate involved is just complex enough that it
would be error-prone.

e.g. your proposed boilerplate appears to be missing a raise in the except
block... (unless you also suggest boilerplate in every final_func to start with
saving sys.exc_info() and then finally reraise it!)

-Andrew.

[Robert Collins (lifeless)]

We actually want something a lot closer to 'with', I suspect.

The problem is that we support python 2.4

-Rob

[Andrew Bennetts (spiv)]

Martin Pool wrote:
[...]
> > I don't particularly like checking return values.
>
> OK, but why? Obviously in general in Python one should be using
> exceptions rather than return code as the normal convention, but are
> there any other drawbacks? The other main reason is that return codes
> can easily be ignored but that is (perhaps) just what we want here.

It's just the “easy to forget to check them” issue that concerns me.

> One problem with return codes is that, if you return a boolean, you
> don't get any explanation of what went wrong. We could potentially
> return an Exception object, but that might be getting too weird.
>
> > I think it might be nice
> > to be able to say "foo.unlock(on_error='warn')", or
> > on_error='log'/'ignore'/'raise'.  I'm not sure which we'd like to have as
> > the default, and it's also questionable about whether we really want to
> > burden all unlock implementations with this, but it's an interesting
> > strawman API.
>
> That seems ok, at least assuming that you expect different callers to
> prefer to get errors, warnings, or nothing. But aside from tests
> specifically for locking, I'm not sure that they will.

Yeah, I'm not sure exactly how featureful we need to be. It could well be a
YAGNI, although testability is important.

> > My thinking here is that *usually* an unlock failure is probably not
> > something to report to the user, but there are times when we definitely want
> > errors to propagate directly (e.g. an explicit invocation of break-lock, and
> > probably during the test suite).
>
> I don't think break-lock calls unlock, it calls break_lock, which
> presumably would always error.

Oh, right, good point. I feel like there's probably other examples, but
perhaps not...

> > So there's a mix of inputs here.  I'm having trouble articulating this
> > clearly, but roughly I feel that what's desired in any individual case may
> > depend on a combination of:
> >
> >  - the cleanup operation itself;
> >  - what the callsite intends (some places may explicitly expect and suppress
> >   particular errors, so emitting something to the UI instead of raising
> >   something the callsite can catch may be a bad thing);
> >  - any global policy, like debug flags;
> >  - and maybe also the specific error (e.g. KeyboardInterrupt vs. connection
> >   lost vs. an assertion about unfinished progress bars)?
> >
> > The challenge seems to be balancing these desires while keeping things
> > reasonable clean and simple.
>
> My idea was that we should try, at least in a branch, just changing
> the policy, and see what happens in several dimensions: how that
> changes the look & feel of the code as you change it, what tests fail
> if any and how easy & clean it is to fix them, and what positive or
> negative effects this has on users. Then we'll have more data on the
> balance.

Ok, trying it in practice makes good sense. I might aim for trying to
update (or at least look at) every try-finally in bzrdir.py, repository.py
and branch.py and see how it goes.

[...]
> I think that policy sounds ok, though maybe we need different
> functions for important and unimportant cleanups. I suggest taking
> the im...

[Martin Pool (mbp)]

2009/9/24 Andrew Bennetts <email address hidden>:
> Martin Pool wrote:
>> 2009/9/24 Stephen J. Turnbull <email address hidden>:
>>
>> > I believe Guido acknowledged not allowing the more compact notation in
>> > the first place to be excessive caution, but it's not impossible to
>> > distinguish exceptions raised by try_func from those raised by
>> > final_func.
>> >
>> > So what's the problem?  You want to do this without fixing all the
>> > broken try ... finally blocks?
>>
>> It's not _impossible_, we just don't want to add boilerplate to
>> everything that uses a try/finally block.  There are many more of them
>> than there are distinct types of cleanup to run.
>
> Especially when the boilerplate involved is just complex enough that it
> would be error-prone.
>
> e.g. your proposed boilerplate appears to be missing a raise in the except
> block... (unless you also suggest boilerplate in every final_func to start with
> saving sys.exc_info() and then finally reraise it!)

This is the place where Stephen's supposed to say something about Lisp
being a great improvement on many languages that came after it... ;-)

--
Martin <http://launchpad.net/~mbp/>

[Martin Pool (mbp)]

2009/9/24 Andrew Bennetts <email address hidden>:
>> My idea was that we should try, at least in a branch, just changing
>> the policy, and see what happens in several dimensions: how that
>> changes the look & feel of the code as you change it, what tests fail
>> if any and how easy & clean it is to fix them, and what positive or
>> negative effects this has on users.  Then we'll have more data on the
>> balance.
>
> Ok, trying it in practice makes good sense.  I might aim for trying to
> update (or at least look at) every try-finally in bzrdir.py, repository.py
> and branch.py and see how it goes.

I'm kind of hoping you won't actually need to change them though, only
the code they call.

> [...]
>> I think that policy sounds ok, though maybe we need different
>> functions for important and unimportant cleanups.  I suggest taking
>> the implementation all the way through with say unlock so that we can
>> see how it actually behaves.
>
> That sounds like a good experiment, although I dread the effort to actually
> update all the implementations of unlock...
>
> (By “all the way through” you mean extending the API of unlock, right?)

By "all the way through" I mean to the point where you can
interactively test (somehow, maybe by killing your network connection)
creating a situation where unlock would run after an error, and
observe that it looks better than it does with the current code. Not
all the way 'across' every case at first.

--
Martin <http://launchpad.net/~mbp/>

[Stephen Turnbull (stephen-xemacs)]

Andrew Bennetts writes:

 > e.g. your proposed boilerplate appears to be missing a raise in the
 > except block...

That's not proposed boilerplate; that's proof of concept that
communication *is* possible.

That said, I don't think reraising from the try is appropriate; that
puts you in the same boat you are worried about where you can't
distinguish between an exception that occurs in try_func() and one
that occurs in final_func().

If I were writing code where there are a lot of try-finally blocks,
but only a few versions of final_func(), what I would probably
actually do is stuff the actual exception into errflag, and let each
final_func() reraise it, deal with it, or ignore it as appropriate to
that final_func().

Agreed, this is still ugly and somewhat error prone with except-less
try-finally blocks, but the semantics and syntax of a finally clause
that handles exceptions are less than transparent to me. Should the
exception be available to finally if already handled? What is the
syntax for binding the exception to an identifier in the finally
clause? Should exceptions (re)raised in an except clause be treated
differently from exceptions that were raised from the try clause?

[Andrew Bennetts (spiv)]

Martin Pool wrote:
[...]
> OK, so +1 from me, but the proof will be in how it would handle the
> bugs reported as TooMany*. It might be worth grepping for them and
> looking at their call stacks.

So, I've spent some time looking at the TooMany* bug reports. Here's a summary
of causes:

 * WorkingTree.pull has error -> then runs unlock in finally
 * Branch.push has error -> then run unlock in finally
 * Repo.fetch has error during stream -> then run unlock in finally
   (e.g. due to buggy stream from old server sending cross-format to 2a)
 * commit has an error -> then runs a bunch of unlocks in a finally

So those are all a case of unlock doomed to failure by prior error, then
obscures that prior error.

There is one other case, though:

 * WorkingTree.commit invokes -> BzrBranch5.get_master_branch -> Branch.open ->
   BzrDir.open -> do_catching_redirections... eventually 'BzrDir.open' RPC fails
   with TooMany* error.

This one doesn't appear to be a cleanup error, but some sort of failure about
charging onwards after an error, when probably we shouldn't? I'm not sure
exactly what's going on there, but I suspect a bug in the way we find formats in
BzrDir.open. Anyway, this one is not a bug during cleanup AFAICT.

So, it appears we would get excellent mileage out of changing unlock on the core
objects (Branch, WorkingTree, etc) to suppress at least most errors, although
probably we still want to allow LockNotHeld and maybe LockBroken to raise, not
just warn?

[Martin Pool (mbp)]

2009/9/25 Andrew Bennetts <email address hidden>:
> Martin Pool wrote:
> [...]
>> OK, so +1 from me, but the proof will be in how it would handle the
>> bugs reported as TooMany*.  It might be worth grepping for them and
>> looking at their call stacks.
>
> So, I've spent some time looking at the TooMany* bug reports.  Here's a summary
> of causes:
>
>  * WorkingTree.pull has error -> then runs unlock in finally
>  * Branch.push has error -> then run unlock in finally
>  * Repo.fetch has error during stream -> then run unlock in finally
>   (e.g. due to buggy stream from old server sending cross-format to 2a)
>  * commit has an error -> then runs a bunch of unlocks in a finally
>
> So those are all a case of unlock doomed to failure by prior error, then
> obscures that prior error.

OK

> There is one other case, though:
>
>  * WorkingTree.commit invokes -> BzrBranch5.get_master_branch -> Branch.open ->
>   BzrDir.open -> do_catching_redirections... eventually 'BzrDir.open' RPC fails
>   with TooMany* error.
>
> This one doesn't appear to be a cleanup error, but some sort of failure about
> charging onwards after an error, when probably we shouldn't?  I'm not sure
> exactly what's going on there, but I suspect a bug in the way we find formats in
> BzrDir.open.  Anyway, this one is not a bug during cleanup AFAICT.
>
> So, it appears we would get excellent mileage out of changing unlock on the core
> objects (Branch, WorkingTree, etc) to suppress at least most errors, although
> probably we still want to allow LockNotHeld and maybe LockBroken to raise, not
> just warn?

That sounds good. We could even consider not treating LockNotHeld etc
differently to start with, people are unlikely to try to catch them.

--
Martin <http://launchpad.net/~mbp/>

[Andrew Bennetts (spiv)]

Martin Pool wrote:
[...]
> > So, it appears we would get excellent mileage out of changing unlock on the core
> > objects (Branch, WorkingTree, etc) to suppress at least most errors, although
> > probably we still want to allow LockNotHeld and maybe LockBroken to raise, not
> > just warn?
>
> That sounds good. We could even consider not treating LockNotHeld etc
> differently to start with, people are unlikely to try to catch them.

I'm pretty sure tests do, though.

I'm currently experimenting with a decorator that looks like this:

    @only_raises(errors.LockNotHeld, errors.LockBroken)
    def unlock(self):

And so far it's looking promising, although there's still a little bit of test
suite fall out...

[lp:~spiv/bzr/cleanup-hof]

4675. By Andrew Bennetts on 2009-09-25

Add some experimental decorators: @only_raises(..) and @cleanup_method.

[John A Meinel (jameinel)]

So what is the status of this submission? Is it superseded by your other work?

[lp:~spiv/bzr/cleanup-hof]

4676. By Andrew Bennetts on 2009-09-28

Change test_unlock_in_write_group to expect a log_exception_quietly rather than a raise.

4677. By Andrew Bennetts on 2009-09-28

Suppress most errors from Branch.unlock too.

4678. By Andrew Bennetts on 2009-10-15

Merge lp:bzr.

4679. By Andrew Bennetts on 2009-10-15

Merge robust-cleanup-in-commit, but ignore its changes (which just drop some features added in this branch.)

Unmerged revisions

4679. By Andrew Bennetts on 2009-10-15

Merge robust-cleanup-in-commit, but ignore its changes (which just drop some features added in this branch.)

=== added file 'bzrlib/cleanup.py'
--- bzrlib/cleanup.py	1970-01-01 00:00:00 +0000
+++ bzrlib/cleanup.py	2009-09-25 02:11:12 +0000
@@ -0,0 +1,172 @@
+# Copyright (C) 2009 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+"""Helpers for managing cleanup functions and the errors they might raise.
+
+Generally, code that wants to perform some cleanup at the end of an action will
+look like this::
+
+    from bzrlib.cleanups import run_cleanup
+    try:
+        do_something()
+    finally:
+        run_cleanup(cleanup_something)
+
+Any errors from `cleanup_something` will be logged, but not raised.
+Importantly, any errors from do_something will be propagated.
+
+There is also convenience function for running multiple, independent cleanups
+in sequence: run_cleanups.  e.g.::
+
+    try:
+        do_something()
+    finally:
+        run_cleanups([cleanup_func_a, cleanup_func_b], ...)
+
+Developers can use the `-Dcleanup` debug flag to cause cleanup errors to be
+reported in the UI as well as logged.
+
+Note the tradeoff that run_cleanup/run_cleanups makes: errors from
+`do_something` will not be obscured by errors from `cleanup_something`, but
+errors from `cleanup_something` will never reach the user, even if there is no
+error from `do_something`.  So run_cleanup is good to use when a failure of
+internal housekeeping (e.g. failure to finish a progress bar) is unimportant to
+a user.
+
+If you want to be certain that the first, and only the first, error is raised,
+then use::
+
+    do_with_cleanups(do_something, cleanups)
+
+This is more inconvenient (because you need to make every try block a
+function), but will ensure that the first error encountered is the one raised,
+while also ensuring all cleanups are run.
+"""
+
+
+import sys
+from bzrlib import (
+    debug,
+    trace,
+    )
+
+def _log_cleanup_error(exc):
+    trace.mutter('Cleanup failed:')
+    trace.log_exception_quietly()
+    if 'cleanup' in debug.debug_flags:
+        trace.warning('bzr: warning: Cleanup failed: %s', exc)
+
+
+def run_cleanup(func, *args, **kwargs):
+    """Run func(*args, **kwargs), logging but not propagating any error it
+    raises.
+
+    :returns: True if func raised no errors, else False.
+    """
+    try:
+        func(*args, **kwargs)
+    except KeyboardInterrupt:
+        raise
+    except Exception, exc:
+        _log_cleanup_error(exc)
+        return False
+    return True
+
+
+def run_cleanup_reporting_errors(func, *args, **kwargs):
+    try:
+        func(*args, **kwargs)
+    except KeyboardInterrupt:
+        raise
+    except Exception, exc:
+        trace.mutter('Cleanup failed:')
+        trace.log_exception_quietly()
+        trace.warning('Cleanup failed: %s', exc)
+        return False
+    return True
+
+
+def run_cleanups(funcs, on_error='log'):
+    """Run a series of cleanup functions.
+
+    :param errors: One of 'log', 'warn first', 'warn all'
+    """
+    seen_error = False
+    for func in funcs:
+        if on_error == 'log' or (on_error == 'warn first' and seen_error):
+            seen_error |= run_cleanup(func)
+        else:
+            seen_error |= run_cleanup_reporting_errors(func)
+
+
+def do_with_cleanups(func, cleanup_funcs):
+    """Run `func`, then call all the cleanup_funcs.
+
+    All the cleanup_funcs are guaranteed to be run.  The first exception raised
+    by func or any of the cleanup_funcs is the one that will be propagted by
+    this function (subsequent errors are caught and logged).
+
+    Conceptually similar to::
+
+        try:
+            return func()
+        finally:
+            for cleanup in cleanup_funcs:
+                cleanup()
+
+    It avoids several problems with using try/finally directly:
+     * an exception from func will not be obscured by a subsequent exception
+       from a cleanup.
+     * an exception from a cleanup will not prevent other cleanups from
+       running (but the first exception encountered is still the one
+       propagated).
+
+    Unike `run_cleanup`, `do_with_cleanups` can propagate an exception from a
+    cleanup, but only if there is no exception from func.
+    """
+    # As correct as Python 2.4 allows.
+    try:
+        result = func()
+    except:
+        # We have an exception from func already, so suppress cleanup errors.
+        run_cleanups(cleanup_funcs)
+        raise
+    else:
+        # No exception from func, so allow the first exception from
+        # cleanup_funcs to propagate if one occurs (but only after running all
+        # of them).
+        exc_info = None
+        for cleanup in cleanup_funcs:
+            # XXX: Hmm, if KeyboardInterrupt arrives at exactly this line, we
+            # won't run all cleanups... perhaps we should temporarily install a
+            # SIGINT handler?
+            if exc_info is None:
+                try:
+                    cleanup()
+                except:
+                    # This is the first cleanup to fail, so remember its
+                    # details.
+                    exc_info = sys.exc_info()
+            else:
+                # We already have an exception to propagate, so log any errors
+                # but don't propagate them.
+                run_cleanup(cleanup)
+        if exc_info is not None:
+            raise exc_info[0], exc_info[1], exc_info[2]
+        # No error, so we can return the result
+        return result
+
+

=== modified file 'bzrlib/decorators.py'
--- bzrlib/decorators.py	2009-03-23 14:59:43 +0000
+++ bzrlib/decorators.py	2009-09-25 02:11:12 +0000
@@ -24,6 +24,9 @@
 
 import sys
 
+from bzrlib.cleanup import run_cleanup
+from bzrlib import trace
+
 
 def _get_parameters(func):
     """Recreate the parameters for a function using introspection.
@@ -204,6 +207,31 @@
     return write_locked
 
 
+def cleanup_method(unbound):
+    """Decorate unbound...    """
+    def cleanup_wrapper(*args, **kwargs):
+        run_cleanup(unbound, *args, **kwargs)
+    cleanup_wrapper.__doc__ = unbound.__doc__
+    cleanup_wrapper.__name__ = unbound.__name__
+    return cleanup_wrapper
+
+
+def only_raises(*errors):
+    def decorator(unbound):
+        def wrapped(*args, **kwargs):
+            try:
+                return unbound(*args, **kwargs)
+            except errors:
+                raise
+            except:
+                trace.mutter('Error suppressed by only_raises:')
+                trace.log_exception_quietly()
+        wrapped.__doc__ = unbound.__doc__
+        wrapped.__name__ = unbound.__name__
+        return wrapped
+    return decorator
+
+
 # Default is more functionality, 'bzr' the commandline will request fast
 # versions.
 needs_read_lock = _pretty_needs_read_lock

=== modified file 'bzrlib/lockable_files.py'
--- bzrlib/lockable_files.py	2009-07-27 05:39:01 +0000
+++ bzrlib/lockable_files.py	2009-09-25 02:11:12 +0000
@@ -32,8 +32,7 @@
 """)
 
 from bzrlib.decorators import (
-    needs_read_lock,
-    needs_write_lock,
+    only_raises,
     )
 from bzrlib.symbol_versioning import (
     deprecated_in,
@@ -221,6 +220,7 @@
         """Setup a write transaction."""
         self._set_transaction(transactions.WriteTransaction())
 
+    @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
         if not self._lock_mode:
             return lock.cant_unlock_not_held(self)

=== modified file 'bzrlib/lockdir.py'
--- bzrlib/lockdir.py	2009-07-27 05:24:02 +0000
+++ bzrlib/lockdir.py	2009-09-25 02:11:12 +0000
@@ -112,6 +112,7 @@
     lock,
     )
 import bzrlib.config
+from bzrlib.decorators import only_raises
 from bzrlib.errors import (
         DirectoryNotEmpty,
         FileExists,
@@ -286,6 +287,7 @@
                                             info_bytes)
         return tmpname
 
+    @only_raises(LockNotHeld, LockBroken)
     def unlock(self):
         """Release a held lock
         """

=== modified file 'bzrlib/progress.py'
--- bzrlib/progress.py	2009-09-24 04:55:10 +0000
+++ bzrlib/progress.py	2009-09-25 02:11:12 +0000
@@ -30,6 +30,7 @@
 from bzrlib import (
     errors,
     )
+from bzrlib.decorators import cleanup_method
 from bzrlib.trace import mutter
 from bzrlib.symbol_versioning import (
     deprecated_function,
@@ -130,6 +131,7 @@
     def tick(self):
         self.update(self.msg)
 
+    @cleanup_method
     def finished(self):
         if self.progress_view:
             self.progress_view.task_finished(self)
@@ -247,6 +249,7 @@
         # next update should not throttle
         self.last_update = now - self.MIN_PAUSE - 1
 
+    @cleanup_method
     def finished(self):
         """Return this bar to its progress stack."""
         self.clear()

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2009-09-24 05:31:23 +0000
+++ bzrlib/remote.py	2009-09-25 02:11:12 +0000
@@ -33,7 +33,7 @@
 )
 from bzrlib.branch import BranchReferenceFormat
 from bzrlib.bzrdir import BzrDir, RemoteBzrDirFormat
-from bzrlib.decorators import needs_read_lock, needs_write_lock
+from bzrlib.decorators import needs_read_lock, needs_write_lock, only_raises
 from bzrlib.errors import (
     NoSuchRevision,
     SmartProtocolError,
@@ -1082,6 +1082,7 @@
         else:
             raise errors.UnexpectedSmartServerResponse(response)
 
+    @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
         if not self._lock_count:
             return lock.cant_unlock_not_held(self)
@@ -2382,6 +2383,7 @@
             return
         raise errors.UnexpectedSmartServerResponse(response)
 
+    @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
         try:
             self._lock_count -= 1

=== modified file 'bzrlib/revisiontree.py'
--- bzrlib/revisiontree.py	2009-08-28 05:00:33 +0000
+++ bzrlib/revisiontree.py	2009-09-25 02:11:12 +0000
@@ -25,6 +25,7 @@
     symbol_versioning,
     tree,
     )
+from bzrlib.decorators import cleanup_method
 
 
 class RevisionTree(tree.Tree):
@@ -180,6 +181,7 @@
         return '<%s instance at %x, rev_id=%r>' % (
             self.__class__.__name__, id(self), self._revision_id)
 
+    @cleanup_method
     def unlock(self):
         self._repository.unlock()
 

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2009-09-24 04:54:19 +0000
+++ bzrlib/tests/__init__.py	2009-09-25 02:11:12 +0000
@@ -3854,6 +3854,188 @@
     This function can be replaced if you need to change the default test
     suite on a global basis, but it is not encouraged.
     """
+<<<<<<< TREE
+=======
+    testmod_names = [
+                   'bzrlib.doc',
+                   'bzrlib.tests.blackbox',
+                   'bzrlib.tests.commands',
+                   'bzrlib.tests.per_branch',
+                   'bzrlib.tests.per_bzrdir',
+                   'bzrlib.tests.per_interrepository',
+                   'bzrlib.tests.per_intertree',
+                   'bzrlib.tests.per_inventory',
+                   'bzrlib.tests.per_interbranch',
+                   'bzrlib.tests.per_lock',
+                   'bzrlib.tests.per_transport',
+                   'bzrlib.tests.per_tree',
+                   'bzrlib.tests.per_pack_repository',
+                   'bzrlib.tests.per_repository',
+                   'bzrlib.tests.per_repository_chk',
+                   'bzrlib.tests.per_repository_reference',
+                   'bzrlib.tests.per_versionedfile',
+                   'bzrlib.tests.per_workingtree',
+                   'bzrlib.tests.test__annotator',
+                   'bzrlib.tests.test__chk_map',
+                   'bzrlib.tests.test__dirstate_helpers',
+                   'bzrlib.tests.test__groupcompress',
+                   'bzrlib.tests.test__known_graph',
+                   'bzrlib.tests.test__rio',
+                   'bzrlib.tests.test__walkdirs_win32',
+                   'bzrlib.tests.test_ancestry',
+                   'bzrlib.tests.test_annotate',
+                   'bzrlib.tests.test_api',
+                   'bzrlib.tests.test_atomicfile',
+                   'bzrlib.tests.test_bad_files',
+                   'bzrlib.tests.test_bencode',
+                   'bzrlib.tests.test_bisect_multi',
+                   'bzrlib.tests.test_branch',
+                   'bzrlib.tests.test_branchbuilder',
+                   'bzrlib.tests.test_btree_index',
+                   'bzrlib.tests.test_bugtracker',
+                   'bzrlib.tests.test_bundle',
+                   'bzrlib.tests.test_bzrdir',
+                   'bzrlib.tests.test__chunks_to_lines',
+                   'bzrlib.tests.test_cache_utf8',
+                   'bzrlib.tests.test_chk_map',
+                   'bzrlib.tests.test_chk_serializer',
+                   'bzrlib.tests.test_chunk_writer',
+                   'bzrlib.tests.test_clean_tree',
+                   'bzrlib.tests.test_cleanup',
+                   'bzrlib.tests.test_commands',
+                   'bzrlib.tests.test_commit',
+                   'bzrlib.tests.test_commit_merge',
+                   'bzrlib.tests.test_config',
+                   'bzrlib.tests.test_conflicts',
+                   'bzrlib.tests.test_counted_lock',
+                   'bzrlib.tests.test_crash',
+                   'bzrlib.tests.test_decorators',
+                   'bzrlib.tests.test_delta',
+                   'bzrlib.tests.test_debug',
+                   'bzrlib.tests.test_deprecated_graph',
+                   'bzrlib.tests.test_diff',
+                   'bzrlib.tests.test_directory_service',
+                   'bzrlib.tests.test_dirstate',
+                   'bzrlib.tests.test_email_message',
+                   'bzrlib.tests.test_eol_filters',
+                   'bzrlib.tests.test_errors',
+                   'bzrlib.tests.test_export',
+                   'bzrlib.tests.test_extract',
+                   'bzrlib.tests.test_fetch',
+                   'bzrlib.tests.test_fifo_cache',
+                   'bzrlib.tests.test_filters',
+                   'bzrlib.tests.test_ftp_transport',
+                   'bzrlib.tests.test_foreign',
+                   'bzrlib.tests.test_generate_docs',
+                   'bzrlib.tests.test_generate_ids',
+                   'bzrlib.tests.test_globbing',
+                   'bzrlib.tests.test_gpg',
+                   'bzrlib.tests.test_graph',
+                   'bzrlib.tests.test_groupcompress',
+                   'bzrlib.tests.test_hashcache',
+                   'bzrlib.tests.test_help',
+                   'bzrlib.tests.test_hooks',
+                   'bzrlib.tests.test_http',
+                   'bzrlib.tests.test_http_response',
+                   'bzrlib.tests.test_https_ca_bundle',
+                   'bzrlib.tests.test_identitymap',
+                   'bzrlib.tests.test_ignores',
+                   'bzrlib.tests.test_index',
+                   'bzrlib.tests.test_info',
+                   'bzrlib.tests.test_inv',
+                   'bzrlib.tests.test_inventory_delta',
+                   'bzrlib.tests.test_knit',
+                   'bzrlib.tests.test_lazy_import',
+                   'bzrlib.tests.test_lazy_regex',
+                   'bzrlib.tests.test_lock',
+                   'bzrlib.tests.test_lockable_files',
+                   'bzrlib.tests.test_lockdir',
+                   'bzrlib.tests.test_log',
+                   'bzrlib.tests.test_lru_cache',
+                   'bzrlib.tests.test_lsprof',
+                   'bzrlib.tests.test_mail_client',
+                   'bzrlib.tests.test_memorytree',
+                   'bzrlib.tests.test_merge',
+                   'bzrlib.tests.test_merge3',
+                   'bzrlib.tests.test_merge_core',
+                   'bzrlib.tests.test_merge_directive',
+                   'bzrlib.tests.test_missing',
+                   'bzrlib.tests.test_msgeditor',
+                   'bzrlib.tests.test_multiparent',
+                   'bzrlib.tests.test_mutabletree',
+                   'bzrlib.tests.test_nonascii',
+                   'bzrlib.tests.test_options',
+                   'bzrlib.tests.test_osutils',
+                   'bzrlib.tests.test_osutils_encodings',
+                   'bzrlib.tests.test_pack',
+                   'bzrlib.tests.test_patch',
+                   'bzrlib.tests.test_patches',
+                   'bzrlib.tests.test_permissions',
+                   'bzrlib.tests.test_plugins',
+                   'bzrlib.tests.test_progress',
+                   'bzrlib.tests.test_read_bundle',
+                   'bzrlib.tests.test_reconcile',
+                   'bzrlib.tests.test_reconfigure',
+                   'bzrlib.tests.test_registry',
+                   'bzrlib.tests.test_remote',
+                   'bzrlib.tests.test_rename_map',
+                   'bzrlib.tests.test_repository',
+                   'bzrlib.tests.test_revert',
+                   'bzrlib.tests.test_revision',
+                   'bzrlib.tests.test_revisionspec',
+                   'bzrlib.tests.test_revisiontree',
+                   'bzrlib.tests.test_rio',
+                   'bzrlib.tests.test_rules',
+                   'bzrlib.tests.test_sampler',
+                   'bzrlib.tests.test_selftest',
+                   'bzrlib.tests.test_serializer',
+                   'bzrlib.tests.test_setup',
+                   'bzrlib.tests.test_sftp_transport',
+                   'bzrlib.tests.test_shelf',
+                   'bzrlib.tests.test_shelf_ui',
+                   'bzrlib.tests.test_smart',
+                   'bzrlib.tests.test_smart_add',
+                   'bzrlib.tests.test_smart_request',
+                   'bzrlib.tests.test_smart_transport',
+                   'bzrlib.tests.test_smtp_connection',
+                   'bzrlib.tests.test_source',
+                   'bzrlib.tests.test_ssh_transport',
+                   'bzrlib.tests.test_status',
+                   'bzrlib.tests.test_store',
+                   'bzrlib.tests.test_strace',
+                   'bzrlib.tests.test_subsume',
+                   'bzrlib.tests.test_switch',
+                   'bzrlib.tests.test_symbol_versioning',
+                   'bzrlib.tests.test_tag',
+                   'bzrlib.tests.test_testament',
+                   'bzrlib.tests.test_textfile',
+                   'bzrlib.tests.test_textmerge',
+                   'bzrlib.tests.test_timestamp',
+                   'bzrlib.tests.test_trace',
+                   'bzrlib.tests.test_transactions',
+                   'bzrlib.tests.test_transform',
+                   'bzrlib.tests.test_transport',
+                   'bzrlib.tests.test_transport_log',
+                   'bzrlib.tests.test_tree',
+                   'bzrlib.tests.test_treebuilder',
+                   'bzrlib.tests.test_tsort',
+                   'bzrlib.tests.test_tuned_gzip',
+                   'bzrlib.tests.test_ui',
+                   'bzrlib.tests.test_uncommit',
+                   'bzrlib.tests.test_upgrade',
+                   'bzrlib.tests.test_upgrade_stacked',
+                   'bzrlib.tests.test_urlutils',
+                   'bzrlib.tests.test_version',
+                   'bzrlib.tests.test_version_info',
+                   'bzrlib.tests.test_weave',
+                   'bzrlib.tests.test_whitebox',
+                   'bzrlib.tests.test_win32utils',
+                   'bzrlib.tests.test_workingtree',
+                   'bzrlib.tests.test_workingtree_4',
+                   'bzrlib.tests.test_wsgi',
+                   'bzrlib.tests.test_xml',
+                   ]
+>>>>>>> MERGE-SOURCE
 
     loader = TestUtil.TestLoader()
 

=== added file 'bzrlib/tests/test_cleanup.py'
--- bzrlib/tests/test_cleanup.py	1970-01-01 00:00:00 +0000
+++ bzrlib/tests/test_cleanup.py	2009-09-25 02:11:12 +0000
@@ -0,0 +1,259 @@
+# Copyright (C) 2009 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+from cStringIO import StringIO
+import re
+
+from bzrlib.cleanup import (
+    do_with_cleanups,
+    run_cleanup,
+    )
+from bzrlib.tests import TestCase
+from bzrlib import (
+    debug,
+    trace,
+    )
+
+
+class CleanupsTestCase(TestCase):
+
+    def setUp(self):
+        super(CleanupsTestCase, self).setUp()
+        self.call_log = []
+
+    def no_op_cleanup(self):
+        self.call_log.append('no_op_cleanup')
+
+    def assertLogContains(self, regex):
+        log = self._get_log(keep_log_file=True)
+        self.assertContainsRe(log, regex, re.DOTALL)
+
+    def failing_cleanup(self):
+        self.call_log.append('failing_cleanup')
+        raise Exception("failing_cleanup goes boom!")
+
+
+class TestRunCleanup(CleanupsTestCase):
+
+    def test_no_errors(self):
+        """The function passed to run_cleanup is run."""
+        self.assertTrue(run_cleanup(self.no_op_cleanup))
+        self.assertEqual(['no_op_cleanup'], self.call_log)
+
+    def test_cleanup_with_args_kwargs(self):
+        def func_taking_args_kwargs(*args, **kwargs):
+            self.call_log.append(('func', args, kwargs))
+        run_cleanup(func_taking_args_kwargs, 'an arg', kwarg='foo')
+        self.assertEqual(
+            [('func', ('an arg',), {'kwarg': 'foo'})], self.call_log)
+
+    def test_cleanup_error(self):
+        """An error from the cleanup function is logged by run_cleanup, but not
+        propagated.
+
+        This is there's no way for run_cleanup to know if there's an existing
+        exception in this situation::
+            try:
+              some_func()
+            finally:
+              run_cleanup(cleanup_func)
+        So, the best run_cleanup can do is always log errors but never raise
+        them.
+        """
+        self.assertFalse(run_cleanup(self.failing_cleanup))
+        self.assertLogContains('Cleanup failed:.*failing_cleanup goes boom')
+
+    def test_cleanup_error_debug_flag(self):
+        """The -Dcleanup debug flag causes cleanup errors to be reported to the
+        user.
+        """
+        log = StringIO()
+        trace.push_log_file(log)
+        debug.debug_flags.add('cleanup')
+        self.assertFalse(run_cleanup(self.failing_cleanup))
+        self.assertContainsRe(
+            log.getvalue(),
+            "bzr: warning: Cleanup failed:.*failing_cleanup goes boom")
+
+    def test_prior_error_cleanup_succeeds(self):
+        """Calling run_cleanup from a finally block will not interfere with an
+        exception from the try block.
+        """
+        def failing_operation():
+            try:
+                1/0
+            finally:
+                run_cleanup(self.no_op_cleanup)
+        self.assertRaises(ZeroDivisionError, failing_operation)
+        self.assertEqual(['no_op_cleanup'], self.call_log)
+
+    def test_prior_error_cleanup_fails(self):
+        """Calling run_cleanup from a finally block will not interfere with an
+        exception from the try block even when the cleanup itself raises an
+        exception.
+
+        The cleanup exception will be logged.
+        """
+        def failing_operation():
+            try:
+                1/0
+            finally:
+                run_cleanup(self.failing_cleanup)
+        self.assertRaises(ZeroDivisionError, failing_operation)
+        self.assertLogContains('Cleanup failed:.*failing_cleanup goes boom')
+
+
+#class TestRunCleanupReportingErrors(CleanupsTestCase):
+#
+#    def test_cleanup_error_reported(self):
+#        xxx
+
+
+class TestDoWithCleanups(CleanupsTestCase):
+
+    def trivial_func(self):
+        self.call_log.append('trivial_func')
+        return 'trivial result'
+
+    def test_runs_func(self):
+        """do_with_cleanups runs the function it is given, and returns the
+        result.
+        """
+        result = do_with_cleanups(self.trivial_func, [])
+        self.assertEqual('trivial result', result)
+
+    def test_runs_cleanups(self):
+        """Cleanup functions are run (in the given order)."""
+        cleanup_func_1 = lambda: self.call_log.append('cleanup 1')
+        cleanup_func_2 = lambda: self.call_log.append('cleanup 2')
+        do_with_cleanups(self.trivial_func, [cleanup_func_1, cleanup_func_2])
+        self.assertEqual(
+            ['trivial_func', 'cleanup 1', 'cleanup 2'], self.call_log)
+
+    def failing_func(self):
+        self.call_log.append('failing_func')
+        1/0
+
+    def test_func_error_propagates(self):
+        """Errors from the main function are propagated (after running
+        cleanups).
+        """
+        self.assertRaises(
+            ZeroDivisionError, do_with_cleanups, self.failing_func,
+            [self.no_op_cleanup])
+        self.assertEqual(['failing_func', 'no_op_cleanup'], self.call_log)
+
+    def test_func_error_trumps_cleanup_error(self):
+        """Errors from the main function a propagated even if a cleanup raises
+        an error.
+
+        The cleanup error is be logged.
+        """
+        self.assertRaises(
+            ZeroDivisionError, do_with_cleanups, self.failing_func,
+            [self.failing_cleanup])
+        self.assertLogContains('Cleanup failed:.*failing_cleanup goes boom')
+
+    def test_func_passes_and_error_from_cleanup(self):
+        """An error from a cleanup is propagated when the main function doesn't
+        raise an error.  Later cleanups are still executed.
+        """
+        exc = self.assertRaises(
+            Exception, do_with_cleanups, self.trivial_func,
+            [self.failing_cleanup, self.no_op_cleanup])
+        self.assertEqual('failing_cleanup goes boom!', exc.args[0])
+        self.assertEqual(
+            ['trivial_func', 'failing_cleanup', 'no_op_cleanup'],
+            self.call_log)
+
+    def test_multiple_cleanup_failures(self):
+        """When multiple cleanups fail (as tends to happen when something has
+        gone wrong), the first error is propagated, and subsequent errors are
+        logged.
+        """
+        cleanups = self.make_two_failing_cleanup_funcs()
+        self.assertRaises(ErrorA, do_with_cleanups, self.trivial_func,
+            cleanups)
+        self.assertLogContains('Cleanup failed:.*ErrorB')
+        log = self._get_log(keep_log_file=True)
+        self.assertFalse('ErrorA' in log)
+
+    def make_two_failing_cleanup_funcs(self):
+        def raise_a():
+            raise ErrorA('Error A')
+        def raise_b():
+            raise ErrorB('Error B')
+        return [raise_a, raise_b]
+
+    def test_multiple_cleanup_failures_debug_flag(self):
+        log = StringIO()
+        trace.push_log_file(log)
+        debug.debug_flags.add('cleanup')
+        cleanups = self.make_two_failing_cleanup_funcs()
+        self.assertRaises(ErrorA, do_with_cleanups, self.trivial_func, cleanups)
+        self.assertContainsRe(
+            log.getvalue(), "bzr: warning: Cleanup failed:.*Error B\n")
+        self.assertEqual(1, log.getvalue().count('bzr: warning:'),
+                log.getvalue())
+
+    def test_func_and_cleanup_errors_debug_flag(self):
+        log = StringIO()
+        trace.push_log_file(log)
+        debug.debug_flags.add('cleanup')
+        cleanups = self.make_two_failing_cleanup_funcs()
+        self.assertRaises(ZeroDivisionError, do_with_cleanups,
+            self.failing_func, cleanups)
+        self.assertContainsRe(
+            log.getvalue(), "bzr: warning: Cleanup failed:.*Error A\n")
+        self.assertContainsRe(
+            log.getvalue(), "bzr: warning: Cleanup failed:.*Error B\n")
+        self.assertEqual(2, log.getvalue().count('bzr: warning:'))
+
+    def test_func_may_mutate_cleanups(self):
+        """The main func may mutate the cleanups before it returns.
+        
+        This allows a function to gradually add cleanups as it acquires
+        resources, rather than planning all the cleanups up-front.
+        """
+        # XXX: this is cute, but an object with an 'add_cleanup' method may
+        # make a better API?
+        cleanups_list = []
+        def func_that_adds_cleanups():
+            self.call_log.append('func_that_adds_cleanups')
+            cleanups_list.append(self.no_op_cleanup)
+            return 'result'
+        result = do_with_cleanups(func_that_adds_cleanups, cleanups_list)
+        self.assertEqual('result', result)
+        self.assertEqual(
+            ['func_that_adds_cleanups', 'no_op_cleanup'], self.call_log)
+
+    def test_cleanup_error_debug_flag(self):
+        """The -Dcleanup debug flag causes cleanup errors to be reported to the
+        user.
+        """
+        log = StringIO()
+        trace.push_log_file(log)
+        debug.debug_flags.add('cleanup')
+        self.assertRaises(ZeroDivisionError, do_with_cleanups,
+            self.failing_func, [self.failing_cleanup])
+        self.assertContainsRe(
+            log.getvalue(),
+            "bzr: warning: Cleanup failed:.*failing_cleanup goes boom")
+        self.assertEqual(1, log.getvalue().count('bzr: warning:'))
+
+
+class ErrorA(Exception): pass
+class ErrorB(Exception): pass