Exception mangling in hooks?

Andrew Bennetts andrew.bennetts at canonical.com
Fri Feb 27 10:50:21 GMT 2009


Robert Collins wrote:
> On Thu, 2009-02-26 at 08:31 -0600, John Arbash Meinel wrote:
[...]
> > History has shown that plugins do tend to be sloppy about error
> > handling, and we have gotten a fair number of bug reports because of
> > things that plugins do. (I don't have a count, but as an example just
> > bzr-loom's handling of 'status' caused 3-4 repeat bugs to be raised in
> > bzr core, even though the last line clearly showed it was occurring in
> > bzr-loom.)
> 
> This is true, but loom was failing in command argument handling, not in
> a hook. If the general principle were a good idea, we'd be wrapping
> every exception that a plugin might generate, no?

That's hardly a convincing argument.  There are plenty of good ideas we
haven't implemented yet...

FWIW, I agree with John — and I suspect custom hooks are going to be more
likely to be buggy than general plugins, because they are/will be a more
common sort of plugin to write, to meet the specific needs of a particular
project.  They're more likely to be written on an ad hoc basis and not
rigorously tested.

There's a limit to how much we can protect bzrlib against plugins.  New
branch formats like bzr-loom's integrate at a very deep and pervasive level,
so the best we can realistically do for that case is something like tweak
bzrlib.trace.report_bug to examine the traceback we're about to show the
user and modify our “Please report a bzr bug” message to be a bit more
ambivalent if “bzrlib/plugins” appears in the traceback.

But when it comes to hooks, they are clear, well-defined points where we're
handing off to 3rd-party code.  It's pretty easy to intercept their errors
so that we report them more clearly for users (and insulate bzrlib from
accidentally misinterpreting a hook's exception as one of its own), and I
think the downside is minimal.  (And if a particular hook wants to
explicitly allow some exceptions, as pre_change_branch_tip does with
TipChangeRejected, that's easily accomodated.)

I'm not sure why you found the HookFailed exception so much harder to work
with when diagnosing test failures: the full traceback should still be
displayed in a test failure, because Python shows "traceback +
str(exception)", and str(exception) here will include the traceback captured
by HookFailed.  The captured traceback is still accessible in the HookFailed
object if you want to use pdb on it.  We could add a .post_mortem method to
HookFailed invoke pdb on it if that's what you need, but I honestly don't
follow why you found it harder to work with.  I originally added it because
I found it clearer and easier!

Even if there is some degree of developer inconvenience that can't be
improved, that should be weighed against the better user experience.

-Andrew.




More information about the bazaar mailing list