agronholm / exceptiongroup

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

Bizarre error in `sys.excepthook` with recursive calls #33

Closed Zac-HD closed 2 years ago

Zac-HD commented 2 years ago
Error in sys.excepthook:
Traceback (most recent call last):
  File "/root/.pyenv/versions/3.10.4/lib/python3.10/site-packages/exceptiongroup/_formatting.py", line 71, in exceptiongroup_excepthook
    sys.stderr.write("".join(traceback.format_exception(etype, value, tb)))
  File "/root/.pyenv/versions/3.10.4/lib/python3.10/traceback.py", line 136, in format_exception
    return list(te.format(chain=chain))
  File "/root/.pyenv/versions/3.10.4/lib/python3.10/site-packages/exceptiongroup/_formatting.py", line 206, in format
    yield from exc.exceptions[i].format(chain=chain, _ctx=_ctx)
  File "/root/.pyenv/versions/3.10.4/lib/python3.10/site-packages/exceptiongroup/_formatting.py", line 142, in format
    if exc.__cause__ is not None:
AttributeError: 'PatchedTracebackException' object has no attribute '__cause__'. Did you mean: '__class__'?

I'm pretty sure this is because the upstream __init__ method doesn't assign __cause__ for recursive calls, and so if you happen to hit this (on exceptiongroup == 1.0.0rc9) you subsequently crash with an AttributeError.

I think the obvious getattr(exc, "__cause__", None) patch is probably the best way forward; happy to write that if you'd like. Unfortunately I don't have a good reproducing case or indeed any way to reproduce this locally - it's consistently crashing only in a particular build and CI system, and at time of writing I've worked out why but not constructed a repro.

agronholm commented 2 years ago

The behavior seems to have changed in Python 3.10. But even that seems to unconditionally assign both __cause__ and __context__ to every TE. So a reproducing snippet would be really welcome. I'm not comfortable patching the code until I understand what's going on.

agronholm commented 2 years ago

Python 3.11 was just released. I've said that I wanted to release the v1.0.0 final of this backport when that happens, but with a problem like this lurking in the code, I don't want to do that yet. I would appreciate at least some hints. You said you had worked out why it happens, so please share with the class?

Zac-HD commented 2 years ago

Based on reading the implementation I thought that this could happen if (something weird happened and) we created a __cause__-free TE, but I haven't yet managed to get a reproducer smaller than "an end-to-end stresstest". Getting something debuggable and then sharable is my top priority for up to the next few days though - sorry to report this so close to the release date!

gschaffner commented 2 years ago

I just came across a reproducer for this while testing against the dev branch of anyio. With the anyio bits pulled out, it simplifies to

raise Exception() from ExceptionGroup("", (Exception(),))

on CPython 3.10.

Bisecting against CPython (after applying patch

diff --git a/src/exceptiongroup/_formatting.py b/src/exceptiongroup/_formatting.py
index 2c74162..6d47e20 100644
--- a/src/exceptiongroup/_formatting.py
+++ b/src/exceptiongroup/_formatting.py
@@ -85,8 +85,6 @@ class PatchedTracebackException(traceback.TracebackException):
         _seen: set[int] | None = None,
     ) -> None:
         kwargs: dict[str, Any] = {}
-        if sys.version_info >= (3, 10):
-            kwargs["compact"] = compact

         # Capture the original exception and its cause and context as
         # TracebackExceptions

for compat.) found https://github.com/python/cpython/commit/6dfd1734f5b230bb8fbd2a9df806c1333b6652a8 as the commit that this broke due to.

I think the problem is the following: when PatchedTracebackException.__init__ instantiates a new PatchedTracebackException te for each grouped exception, it passes along (a copy of) _seen, so te.__init__ thinks it's being called by the recursion unroller (root_te.__init__) and assumes its caller will generate and set te.__cause__ and te.__context__ for it. But the caller PatchedTracebackException.__init__ does not do this.

Passing _seen=None to construct the PatchedTracebackException for each group member would fix this. However doing so might cause a different bug—I don't currently understand why a copy of _seen needs to get passed to each grouped PatchedTracebackException on Python < 3.10 (per the code comment).

agronholm commented 2 years ago

Thanks, that should help me get started on the fix.

agronholm commented 2 years ago

Yeah, I can confirm that the above code fails only on Python 3.10. Works fine on 3.9 and 3.11.

agronholm commented 2 years ago

I've now turned this into a test that only fails on py3.10.

agronholm commented 2 years ago

I'm currently pursuing a few possible avenues for fixing this. Straight up copying code from the stdlib would be dubious considering the amount of private API calls it makes, and the fact that this has to work on older Pythons as well.

agronholm commented 2 years ago

Uhm...my first attempt ended up with Python 3.10 working and the rest failing. Back to the drawing board :sweat_smile:

agronholm commented 2 years ago

Ok, I finally got it working across the board. Please review the PR so we can get the final release out. Thanks!

agronholm commented 2 years ago

Ok, v1.0.0 final is out now!