[RFC] Add run_cleanup, do_with_cleanup

[Andrew Bennetts]

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

> === added file 'bzrlib/cleanup.py'
> --- bzrlib/cleanup.py	1970-01-01 00:00:00 +0000
> +++ bzrlib/cleanup.py	2009-09-24 01:05:22 +0000
> 
> +"""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.
> 
> I wonder if generally this should be going inside the function run
> from cleanups - but we can change that later.

Yeah, me too...

[...]
> 
> Nice docstring.

Thanks!

I found writing it really helped me decide what code I was trying to write.

> These could perhaps become with blocks when we require python2.5 (or 2.6?).

Yeah, probably.  I haven't thought about that too hard.

[...]
> Do you need to handle KeyboardInterrupt specially?  Isn't it outside
> of Exception, or is that only from 2.5 on?

Only from 2.5 IIRC.  I'll double-check.

[...]
> Ok, I can see this is a bit different but it probably wants a
> docstring to say so.

Oops, yes.

[...]
> +    by func or any of the cleanup_funcs is the one that will be propagted by
> +    this function (subsequent errors are caught and logged).
> 
> typo

Ta.

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

My browser is filled with tabs of those bugs as I write...

Thanks for the review!

-Andrew.