Format MultiError without sys.excepthook

[rotu]

I think this may be a better approach to MultiError. It doesn't involve clobbering sys.excepthook.

Initial thoughts?

Fixes #1065

[rotu]

Obviously a little more massaging is necessary, to make the printing nicer and to adjust tests to match. Please let me know if I should spend the time.

Here's the after error message from 'test_simple_excepthook' after this change:

MultiError.__cause__: 
Details of cause 1:


  Traceback (most recent call last):

    File "/home/dan/.config/JetBrains/PyCharm2020.1/scratches/scratch.py", line 14, in <module>
      raise ValueError

  ValueError


Details of cause 2:


  Traceback (most recent call last):

    File "/home/dan/.config/JetBrains/PyCharm2020.1/scratches/scratch.py", line 19, in <module>
      raise TypeError

  TypeError


The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/dan/.config/JetBrains/PyCharm2020.1/scratches/scratch.py", line 24, in <module>
    raise MultiError([e1, e2])
trio.MultiError: ValueError(), TypeError()

[rotu]

@njsmith is this viable (assuming I fix all the tests)? Or is there some deep reason it wasn’t done this way in the first place?

[oremanj]

Unfortunately I'm not sure this approach will be workable (though I appreciate you taking the time to prototype it!). Using __cause__ for this prevents a MultiError from having a different cause or context displayed. (The same exception can have both a cause and a context, but if both are set then by default only the cause will be shown.) Also, semantically the MultiError is intended to represent multiple exceptions at once, rather than being "caused by" the combination of multiple exceptions at once. See #611 for much more discussion on this.

I think the best solution for #1065 is to directly detect and disable the apport hook.

[rotu]

Hmmm... I thought cause and context were not mutually exclusive. I can certainly see an example where a MultiError has a context, but not where it should have a cause that’s not in the list of exceptions it rounds up.

As for the multierror being a conjunction of errors versus caused by a conjunction of errors, I’m on the fence. Certainly there is a subtle change of semantics, but when errors X and Y fail in a nursery, I think it does make sense to say failures of X and Y mutually caused a single failure I’m the nursery.

I’ll meditate on this and read over #611 a few more times

[oremanj]

They’re not mutually exclusive (you can set both attributes) but they sort of are (if you set a cause, the context won’t be shown when printing the traceback).

Another issue with using the cause to represent the source exceptions: the cause is shown before the main exception, but the tracebacks look nicer if the MultiError’s constituent exceptions are shown after the main exception. That way you can read the tree straightforwardly from top to bottom, following whichever branch of the MultiError you’re interested in.

[njsmith]

Sorry for being slow to reply... I've been trying to figure out what I wanted to say. This is tricky!

I'm not certain, but I'm leaning towards rejecting this. Here's my reasoning:

I feel like this works better in some ways (avoiding monkeypatching) but worse in others (messes with __cause__). Either way we'll be in a place where MultiError... mostly kinda works, but is clunky, and will ultimately have to be replaced by a better system (probably built into the interpreter).

The one unambiguous issue this is fixing is #1065, but that's also pretty easy to fix with the current system (see #1528), and that's a much less disruptive change.

So, given that this isn't an obvious clear-cut win, I think it's probably better to leave it be, and put that energy towards the long-term fix instead.

Does that sound right to you? Let me know what you think.

[rotu]

@oremanj

They’re not mutually exclusive (you can set both attributes) but they sort of are (if you set a cause, the context won’t be shown when printing the traceback).

Wow! You're right and that seems like a huge problem for this PR! It seems a built-in feature of PEP 3134, with the underlying assumption that the exception chain is linear: "The chain of exceptions is traversed by following the __cause__ and __context__ attributes, with __cause__ taking priority."

Another issue with using the cause to represent the source exceptions: the cause is shown before the main exception, but the tracebacks look nicer if the MultiError’s constituent exceptions are shown after the main exception. That way you can read the tree straightforwardly from top to bottom, following whichever branch of the MultiError you’re interested in.

I thought of that, and I don't think it's undesirable. The order of a stack and of intermediate errors in Python is deliberate (as per PEP 3134 "In keeping with the chronological order of tracebacks, the most recently raised exception is displayed last"). MultiError breaks this convention.

@njsmith

Sorry for being slow to reply... I've been trying to figure out what I wanted to say. This is tricky!

No worries. It is indeed tricky!!!

I'm not certain, but I'm leaning towards rejecting this. Here's my reasoning:

I feel like this works better in some ways (avoiding monkeypatching) but worse in others (messes with __cause__). Either way we'll be in a place where MultiError... mostly kinda works, but is clunky, and will ultimately have to be replaced by a better system (probably built into the interpreter).

I wouldn't call it "messing with __cause__" rather than "using __cause__". The proposal in #611 seems to me a reimplementation (that essentially decays to __cause__ when there's one exception).

But yeah, as @oremanj points out, the assumptions in the existing framework (linear error chain, either explicit or implicit chaining, i.e. __cause__ vs. __context__) show that this is not really compatible with the error chain abstraction.

The one unambiguous issue this is fixing is #1065, but that's also pretty easy to fix with the current system (see #1528), and that's a much less disruptive change.

I'm really glad to see #1528! This PR was born of the frustration with #1065.

So, given that this isn't an obvious clear-cut win, I think it's probably better to leave it be, and put that energy towards the long-term fix instead.

Does that sound right to you? Let me know what you think.

Basically it comes down to: is this PR a stepping stone towards a better long-term fix of MultiError or a change that makes it harder to evolve MultiError in the future? It seems this is the latter, so closing.

Maaaybe it's possible to achieve the same aims by modifying the traceback object, which is something I hope to explore.

[pquentin]

I'm really glad to see #1528! This PR was born of the frustration with #1065.

#1528 is now on PyPI as part of 0.15.0, which also removes Python 3.5 support: https://pypi.org/project/trio/0.15.0/

@rotu Is there anything else you'd like to work on? We have lots of other things to fix :)

[rotu]

@pquentin unfortunately I can’t hack on trio much right now. The warning about error handlers became high priority, as it was displayed every time the user would start our robot utility program on Ubuntu https://pypi.org/project/openrover/

I’m sure I can dig into other issues if/when they get in my way or when I have the bandwidth

[oremanj]

I thought of that, and I don't think it's undesirable. The order of a stack and of intermediate errors in Python is deliberate (as per PEP 3134 "In keeping with the chronological order of tracebacks, the most recently raised exception is displayed last"). MultiError breaks this convention.

If you think about the individual exceptions as the cause of the MultiError, then you're right. But Trio generally doesn't do that - it thinks of the MultiError as simply a bundle of exceptions, which did some propagating on their own and then some more propagating together. The traceback frames on the MultiError correspond to locations further up the call tree than the traceback frames on the individual exceptions, so they should be further up the traceback printout.

Another way to think about it: In all of the following examples, outer() calls inner(), so the Python convention is for the inner traceback frame to be displayed lower on the screen when printing the traceback ("most recent call last").

def outer():
    inner()
def inner():
    raise RuntimeError

async def outer():
    async with trio.open_nursery() as nursery:
        nursery.start_soon(inner)
async def inner():
    raise RuntimeError

async def outer():
    async with trio.open_nursery() as nursery:
        nursery.start_soon(inner)
        nursery.start_soon(inner)
async def inner():
    raise RuntimeError

But in your PR, the last of these would have the inner frames above the outer frame.