agronholm / exceptiongroup

Backport of PEP 654 (exception groups)
Other
42 stars 20 forks source link

exceptiongroup override of sys.excepthook changes stdlib code module behavior #26

Closed exarkun closed 1 year ago

exarkun commented 2 years ago

stdlib code.InteractiveInterpreter has two methods that include a check of whether sys.excepthook is the same object as sys.__excepthook__.

If the exceptiongroup formatting override is applied then these conditionals take their alternate branch. For applications which overrode the write method, the error content is now sent to (probably) the real process stderr instead of handled by whatever logic the write method implements.

For example, Twisted Manhole subclasses InteractiveInterpreter and overrides write to let it send output over telnet or ssh connections.

The possible failure here went unnoticed for many years but recently I tried to add some Hypothesis-based tests to Twisted. Hypothesis apparently has a dependency on exceptiongroup and in some environments (this part is still a mystery to me!) the result is that some Twisted Manhole unit tests fail because they fail to capture the exception output because the modified sys.excepthook now causes code.InteractiveInterpreter.showtraceback to call the exceptiongroup hook instead of the instance's own write method.

agronholm commented 2 years ago

I don't think I understand. Are you trying to report a bug in exceptiongroup? To me it sounds like the Manhole code would fail if any library anywhere set its own exception hook. If this is a bug report, can you suggest an alternate approach to making exception groups display properly by default?

exarkun commented 2 years ago

I don't think I understand. Are you trying to report a bug in exceptiongroup?

Maybe, I don't know. I might just be sharing an experience I had with exceptiongroup.

To me it sounds like the Manhole code would fail if any library anywhere set its own exception hook.

I think that's right but also the manhole code is around 15 years old and no one ever set an exception hook (at least, such that the test suite noticed) until exceptiongroup did. This also doesn't mean I think exceptiongroup is necessarily at fault but it seems like overriding excepthook is pretty uncommon.

Maybe this is also just a search result for Google to churn up for anyone else who runs into the same interaction. Or it could also be a place to collect ideas about why the stdlib code module shouldn't have this conditional.

If this is a bug report, can you suggest an alternate approach to making exception groups display properly by default?

I actually have no idea what exception groups are yet :) I only ran into them because exceptiongroup is a dependency of a dependency. I'll try to learn a bit more about them before I try to suggest any potential changes here.

agronholm commented 2 years ago

I actually have no idea what exception groups are yet :)

Here's some reading about that: https://docs.python.org/3.11/library/exceptions.html#exception-groups

cfbolz commented 1 year ago

hm, just noticed this too, because it affects the PyPy REPL (which is just using the code module). Like @exarkun I am not sure what should be done about it, or whether anything should be done about it at all :sweat_smile:

$ pypy3.9
>>>> x
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'x' is not defined
>>>> import exceptiongroup
>>>> x
Traceback (most recent call last):
  File "/home/cfbolz/bin/pypy-c-jit-105829-51afa45d7c16-linux64/lib/pypy3.9/code.py", line 90, in runcode
    exec(code, self.locals)
  File "<stdin>", line 1, in <module>
NameError: name 'x' is not defined

note the extra line in the traceback.

this is the function in the code module that causes it:

    def showtraceback(self):
        """Display the exception that just occurred.

        We remove the first stack item because it is our own code.

        The output is written by self.write(), below.

        """
        sys.last_type, sys.last_value, last_tb = ei = sys.exc_info()
        sys.last_traceback = last_tb
        try:
            lines = traceback.format_exception(ei[0], ei[1], last_tb.tb_next)
            if sys.excepthook is sys.__excepthook__:
                self.write(''.join(lines))
            else:
                # If someone has set sys.excepthook, we let that take precedence
                # over self.write
                sys.excepthook(ei[0], ei[1], last_tb)
        finally:
            last_tb = ei = None

in the "default" code path exception printing starts at last_tb.tb_next, to get rid of the frame that comes from the code module. But if the excepthook is overridden, it gets passed last_tb.

agronholm commented 1 year ago

It's been a while now, and I've even improved the implementation to follow the built-in implementations more closely. If any practical issues arise, then those should be submitted as new issues.