Repository navigation
Better ergonomics for exceptions #56
Description
Our error-reporting story is tolerable at this point (see #49 for some history), but could definitely still be improved. This collects some misc ideas:
- Our tracebacks are very cluttered and hard to read (lots of internal helper functions etc. all over the place) – can we cut this down?
- Rewrite nursery context manager without
@asynccontextmanagerand@async_generatorto reduce extraneous frames - Avoid raising internally to communicate gathered exceptions up to nursery exit (see implement open_nursery with explicit context manager #612 (comment) and Better ergonomics for exceptions #56 (comment))
- Maybe elide trio-internal stuff with [...4 frames skipped...] to help users focus on their code? May be at odds with pushing our traceback logic down to the interpreter.
- Rewrite nursery context manager without
- Tracebacks should include task context, like: as an exception propagates in (
Error.unwrap) and out (_task_finished) of tasks, update a record likeexc.__trio_task_context__with entries like(len(tb), "exit", task), and use them to add context to printed tracebacks. (The need to computelenis a bit unfortunate though... I guess ideally these should be attributes on the tb objects themselves? But tb objects don't have__dict__s. Or we could just stash the tb object.)MultiError.filterneeds to be careful to propagate these correctly when propagating tracebacks. -
MultiError.catchis OK, but could be better- We don't propagate
__context__onMultiErrors - We should really have an async version... but bpo29600 makes this difficult!
- The filter interface is maybe a bit awkward and error prone?
- If people try to work around the awkwardness by doing some sort of
try: raise exc except: ...thing, then they get corrupt tracebacks (we assume that the filter callback doesn't touch the traceback). We could potentially detect new tb frames appearing and fix them up.
- We don't propagate
- Better IPython integration: right now we override the printing of
MultiErrors and use our regularMultiErrorprinting code. This is good b/c it gives full information, but bad b/c it loses out on IPython's enhanced exception formatting. Better ergonomics for working with MultiErrors #49 has some notes on how this might be possible to get both. (Note that this becomes more of an issue if we do as suggested above and start customizing all exception output and not justMultiError!) Possibly should chat with the IPython devs about this.
Activity
bluetech commentedon Mar 23, 2018
Are you familiar with C#'s
AggregateException?Some notes compared to
MultiError:(Subjective) The name is a bit more memorable, to me at least.
Trio follows the rule "any unhandled exceptions are re-raised inside the parent task. If there are multiple exceptions, then they’re collected up into a single MultiError exception".
C# however follows the rule that an
AggregateExceptionis thrown even in the single-exception case. That is, if the operation is such that it needs to aggregate exceptions, thenAggregateExceptionis what it throws always (example).I think the C# way is better:
In order to write correct code, the caller must handle the
MultiErrorcase. If usingMultiError.catch, this handles the single-error case already. If doing owntry-except(the way I prefer), must handle the two cases separately, which is redundant and annoying.It is impossible to be lazy/ignorant of the multi-error case and only handle the single-error case.
Simpler documentation, simpler static typing.
AggregateExceptionhas a flatten method which recursively merges nestedAggregateExceptions. I thinkMutliError.filterdoes some of that, however I personally find it simpler to just catchMultiErrorand iterate over the exceptions, so aflatten(self: MultiError) -> MultiErrorcan be useful.belm0 commentedon Aug 11, 2018
+1 for an option to elide stack frames. Here I'll consider how this might behave and show a concrete example.
Proposed behavior:
Program specimen is (roughly) https://gist.github.com/nonsleepr/43420a0ce659f1b870544fffb9e5cda4
Original trace (with paths shortened):
Trace with trio frames elided (including async_generator, contextvars, and contextlib dependencies) using MIN_FRAMES_ELIDED=2:
belm0 commentedon Aug 18, 2018
notes from @njsmith:
belm0 commentedon Aug 18, 2018
From #393 :
belm0 commentedon Aug 22, 2018
I've seen some recent developer articles complaining about unintelligible exception traces from RxJava and Kotlin. Granted it presents future obstacles, but traces with elided frames would be a big differentiator for Trio.
https://proandroiddev.com/forget-rxjava-kotlin-coroutines-are-all-you-need-part-1-2-4f62ecc4f99b
njsmith commentedon Aug 22, 2018
@belm0 Oh yeah, it's absolutely a real problem, and we need to find a solution one way or another.
The thing about traceback elision that makes me hesitate is, in the long run, we have to find a way to get out of the business of monkeypatching every traceback printer in the world. It's just not maintainable. (See the discussion at the beginning of #611.) And that means that if we want traceback elision to work long-term, we need to come up with a way to move the whole Python ecosystem in that direction. This is doable, and should probably be done: the pytest folks have their own hack for it (
__traceback_hide__, and since Python switched implementing most of the core import machinery in Python those frames have been causing various problems too (see bpo-24305 and linked bugs)... but pushing something like this through python-dev is a ton of work. Like "plan to spend multiple full-time person-months on it" level of work.OTOH, keeping the tracebacks simple in the first place is something we can do with relatively small, straightforward changes today. In many cases it's probably simpler than elision (e.g., it's not totally obvious how to write/tune/test the heuristics, and you avoid needing to create/test/document tuning knobs like
MIN_FRAMES_ELIDED). And, if we run out of low-hanging fruit and the tracebacks are still bad, then when we propose adding elision features to Python we can say we've done our due diligence and proved that the problem really does require interpreter changes.Let's take a look at a concrete case. I'll use your example from #612 (comment):
User code, this is what we want to see.
This is... a lot of junk, mostly specific to entering trio. I'm not going to squint at it too closely right now, because it's the bottom of the traceback where it's most important to make things readable, plus I think a lot of this clutter will go away if we fix the things below. Maybe not all of it, but it'll be easier to revisit after dealing with the higher priority stuff.
User code; this is what we want to see.
Ick. This is what I was referring to here when I mentioned making the whole nursery
__aexit__pathraise-free. I think that's pretty straightforward actually, and will reduce this to a single frame.Also, when we stop compressing
MultiErrors (see #611), this will be the point where we switch indentation levels and say "switched to a new task".This is noise... when we catch a user exception in
run_impl, we should unconditionally discard the top frame in the traceback (which will be this one).This line will disappear on 3.7, where
contextvars.Context.runis implemented in C, so maybe not worth worrying about. Or we could discard it at the same time we fix therun_implframe above.User code, this is what we want to see :-).
We could remove a line here by inlining
unwrapintowait_task_rescheduled. Not a huge win, butunwrapis trivial and these lines probably show up in a lot of tracebacks? Specifically they show up whenever someone usesreschedule(task, Error(...))to throw an exception into a task. That's not the most common way for an exception to be created, but it certainly happens.So to-do list for now, ordered by bang-for-buck:
run_implraisefrom the nursery__aexit__pathunwrapintowait_task_rescheduledrunsetup code and see what's left and what can be done about itnjsmith commentedon Sep 2, 2018
The main cluttered tracebacks problem was fixed by #612, #631, #640, and for all the other complaints at the top of this thread the current plan for fixing them is in #611, so I'm going to close this.
There are probably still a few places where we can fine-tune exception tracebacks, but we can open a new issue for that. Or possibly even claim one of the reserved issues if we want to have a single place for people to report little nits as they notice them.
Added link to blog entry about python-trio#56 to history