[3.9] bpo-43048: RecursionError traceback RecursionError bugfix for cpy3.9 (GH-24460) #24460
Conversation
When traceback is formatting a RecursionError with a long __context__ chain it runs the risk of itself raising a RecursionError. This patch catches the corresponding recursive call's RecursionError, truncates the traceback and emits a warning instead.
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
Lib/test/test_traceback.py
Outdated
| f() | ||
| except Exception: | ||
| exc_info = sys.exc_info() | ||
| traceback.format_exception(exc_info[0], exc_info[1], exc_info[2]) |
There was a problem hiding this comment.
For the purpose of testing the traceback methods it would be ok to unindent the traceback.format_exception(exc_info[0], exc_info[1], exc_info[2]) line so that it's not happening in the except block.
Lib/traceback.py
Outdated
| @@ -465,6 +465,9 @@ class TracebackException: | |||
| - :attr:`msg` For syntax errors - the compiler error message. | |||
| """ | |||
|
|
|||
| def hello(self, exc_value): | |||
There was a problem hiding this comment.
Per my comment here, the surprising fact that this seemingly no-op function is necessary for tests to pass is what tells me there's a bug in whatever mechanism is used to clear the tstate->recursion_headroom flag.
Solving this seems well out of scope for this PR. (I just pushed this so that you could see what was going on.) I'll remove this no-op hello as well as the problematic test case.
There was a problem hiding this comment.
can you create a standalone script to show what happened, and report that as a separate issue?
I think what happened here was that when format_exception is called in the except block, the recursion limit was already reached, so things go haywire. I don't think it's an exponential blowup in traversing the context and cause links.
There was a problem hiding this comment.
Maybe I got it wrong - it was in the except after the recursion returned. I haven't tried running it yet, will try to look into it tomorrow.
There was a problem hiding this comment.
Yes, if you check out 8550dbdd53b87dde1f32288f250507bc43b68130 then without the call to hello() there's a fatal error regardless of the indentation of format_exception() in the test case.
I tried for a bit to make a minimum example script but wasn't able to replicate. I agree there's no exponential blowup (I was mistaken in my long comment in the other thread) because the seen set prevents revisiting nodes in the implicit binary tree I was imagining. Will try some more later.
I still have no explanation as to why the headroom flag isn't getting cleared when we pop out of the recursion error that happens inside the TracebackException constructor (i.e., the second recursion error in the unit test overall in time).
Lib/traceback.py
Outdated
| 'Warning: the above exception was the cause of other ' | ||
| 'exceptions, but those were truncated to avoid ' | ||
| 'stack overflow in the exception handler. ' | ||
| 'See https://bugs.python.org/issue43048 for details.\n') |
There was a problem hiding this comment.
I don't think this is correct - you truncated the cause of what you emit, not the other way around.
Also, I think this should be more concise, something like:
yield "(chained exceptions have been truncated to avoid deep recursion):"
(and certainly not a link to the bpo issue).
There was a problem hiding this comment.
Great, was looking for a more concise way of putting it. I did something along those lines.
…ssages There seems to be another bug that's out-of-scope for this PR, which is handling the tstate->recursion_headroom clearing that's needed for the cause_context test case to pass. I fixed this temporarily by calling a no-op function which I think allows cpython to notice that the stack has been unwound, but it's a convoluted hack and is better addressed head-on by a fix in the ceval code.
|
I pushed up a version that reuses `__suppress_context__` to avoid the
context print. Is that what you had in mind?
…On Mon, Feb 8, 2021 at 3:39 AM Irit Katriel ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Lib/traceback.py
<#24460 (comment)>:
> @@ -620,6 +634,14 @@ def format(self, *, chain=True):
not self.__suppress_context__):
yield from self.__context__.format(chain=chain)
yield _context_message
+ if self._truncated_cause:
+ yield (
+ 'Explicitly chained exceptions have been truncated to avoid '
+ 'stack overflow in traceback formatting:\n')
+ if self._truncated_context:
+ yield (
+ 'Implicitly chained exceptions have been truncated to avoid '
+ 'stack overflow in traceback formatting:\n')
Talking about "my way" and "your way" tends to create an uncollaborative
atmosphere. We're trying find find "our way".
And I don't think we have yet - the code as is it will not work correctly
in the case where cause was truncated: We are supposed to skip emitting
context in that case, but since we set self.*cause* to None and
self._truncated to True, we will print context and then say that cause was
truncated.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#24460 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABMQRVB2QLKWQHWNR276S53S57EQJANCNFSM4XF5GZGQ>
.
|
Lib/traceback.py
Outdated
| @@ -476,34 +476,50 @@ def __init__(self, exc_type, exc_value, exc_traceback, *, limit=None, | |||
| _seen.add(id(exc_value)) | |||
| # Gracefully handle (the way Python 2.4 and earlier did) the case of | |||
| # being called with no type or value (None, None, None). | |||
| self.__suppress_context__ = False | |||
| self._truncated = False | |||
| if (exc_value and exc_value.__cause__ is not None | |||
| and id(exc_value.__cause__) not in _seen): | |||
There was a problem hiding this comment.
If recursion into constructing cause exceeded the recursion limit then surely the call to construct context will too. Why do we need to try blocks?
|
I don't think that's strictly true, because differing exceptions could have
varying numbers of additional stack frames used up in the non-recursive
part of the constructor.
However, you could be getting at the fact that I might as well ignore the
above and just have a generic catch-all except block. Works for me.
…On Mon, Feb 8, 2021 at 8:29 AM Irit Katriel ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Lib/traceback.py
<#24460 (comment)>:
> @@ -476,34 +476,50 @@ def __init__(self, exc_type, exc_value, exc_traceback, *, limit=None,
_seen.add(id(exc_value))
# Gracefully handle (the way Python 2.4 and earlier did) the case of
# being called with no type or value (None, None, None).
+ self.__suppress_context__ = False
+ self._truncated = False
if (exc_value and exc_value.__cause__ is not None
and id(exc_value.__cause__) not in _seen):
If recursion into constructing cause exceeded the recursion limit then
surely the call to construct context will too. Why do we need to try blocks?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#24460 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABMQRVHNTLDKZ4PWBZ2L7RTS6AGPFANCNFSM4XF5GZGQ>
.
|
| @@ -0,0 +1 @@ | |||
| When traceback is formatting a RecursionError with a long __context__ chain it runs the risk of itself raising a RecursionError. This patch catches the corresponding recursive call's RecursionError, truncates the traceback and emits a warning instead. | |||
There was a problem hiding this comment.
This is ambiguous because there is also traceback formatting code in the interpreter. Also probably too long, and mentions context but not cause. This is probably enough to indicate the impact (and there are always more details in the bpo):
Handle RecursionErrors in :class:~traceback.TracebackException's constructor, so that long exceptions chains are truncated instead of causing traceback formatting to fail.
|
Thanks. You'll need to sign a CLA as well. |
|
@iritkatriel should this PR have "awaiting core review" as a label? |
Yes, it needs to be approved by a core developer. |
|
This PR is stale because it has been open for 30 days with no activity. |
|
Closed and re-opened to trigger CI. |
|
@ambv: Please replace |
https://bugs.python.org/issue43048