Skip to content
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

Bizarre error in sys.excepthook with recursive calls #33

Closed
Zac-HD opened this issue Oct 20, 2022 · 11 comments · Fixed by #34
Closed

Bizarre error in sys.excepthook with recursive calls #33

Zac-HD opened this issue Oct 20, 2022 · 11 comments · Fixed by #34
Labels
bug Something isn't working

Comments

@Zac-HD
Copy link
Contributor

Zac-HD commented Oct 20, 2022

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
Copy link
Owner

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 agronholm added the bug Something isn't working label Oct 24, 2022
@agronholm
Copy link
Owner

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
Copy link
Contributor Author

Zac-HD commented Oct 24, 2022

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
Copy link

gschaffner commented Oct 26, 2022

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 python/cpython@6dfd173 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
Copy link
Owner

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

@agronholm
Copy link
Owner

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

@agronholm
Copy link
Owner

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

@agronholm
Copy link
Owner

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
Copy link
Owner

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

@agronholm
Copy link
Owner

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

agronholm added a commit that referenced this issue Oct 27, 2022
* Fixed AttributeError when rendering an excgroup as a cause for another exception

Fixes #33.

* Added an extra test for EG loop rendering

Closes #35.
@agronholm
Copy link
Owner

Ok, v1.0.0 final is out now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants