[RFC] Add run_cleanup, do_with_cleanup

[Martin Pool]

2009/9/25 Andrew Bennetts <andrew.bennetts at canonical.com>:
> 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/>