New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is our current strategy for handling instrument errors optimal? #47

Open
njsmith opened this Issue Feb 12, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@njsmith
Member

njsmith commented Feb 12, 2017

Right now, if an instrumentation callback raises an exception, we (a) print log (see #306) the traceback to stderr, (b) disable that instrument, (c) carry on.

This is the only place in trio that discards exceptions instead of handling or propagating them. Is this the right choice?

The motivation is that instrumentation gets called at all kinds of weird times where it's quite difficult to propagate an error, and it's not clear where to propagate to in any case, and while we in general prefer to crash early and noisily in general it's still probably true that no-one wants their instrumentation to take down their server (I think).

The downside are the obvious ones: it's easy to miss stuff dumped to stderr, if some tool is trying to automatically collect instrumentation then it could get wedged in unexpected ways if the instrument just disappears, etc.

So, I'm not entirely sure this is the best approach, and would be interested to hear what others think.

@smurfix

This comment has been minimized.

Contributor

smurfix commented Sep 26, 2017

Why not add a .detached(exc=None) method to the instrument? That'd allow it to clean up after itself. Of course if that also dies all bets are off, but at least you'd have a chance to exit gracefully.

@njsmith

This comment has been minimized.

Member

njsmith commented Sep 26, 2017

Hmm, so the idea would be that unhandled exceptions still automatically get printed and kick-out the instrument, but there would be a general life-cycle hook (or hooks, attached and detached) that an instrument could define, both to do any general setup and also as a last-chance to do something not-terrible when it crashes? That does sound reasonable. My initial instrument design didn't have any way to detect when an instrument was added/removed (you were just allowed to bodge around with the internal list directly), so this wasn't on my radar, but we're already switching to a more structured system (#257) so this could fit nicely after that transition finishes (which will be immediately after I finish up the 0.2.0 release).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment