Skip to content

gh-123596: Fixed return None when getattr the sys.tracebacklimit. #125777

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

Closed
wants to merge 2 commits into from

Conversation

rruuaanng
Copy link
Contributor

@rruuaanng rruuaanng commented Oct 21, 2024

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests for this change.

@rruuaanng
Copy link
Contributor Author

Please let me finish the work at hand. I'm about to add everything :)

@rruuaanng rruuaanng changed the title gh-123596: Fixed the problem of returning None when getting the sys.tracebacklimit. gh-123596: Fixed return None when getattr the sys.tracebacklimit. Oct 21, 2024
@vstinner
Copy link
Member

Please let me finish the work at hand. I'm about to add everything :)

Please don't create a pull request until it's ready according to you. Otherwise people might want to review it without knowing that it's incomplete.

@rruuaanng
Copy link
Contributor Author

I see. Maybe I should respond to your request quickly. So I created a draft pull. I hope no one will misunderstand. I'm working this.

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Oct 21, 2024

I'm not sure it's worth adding the test. It seems like already exists. And his handling of tracebacklimit is really only 1000 when it's undefined.

def test_extract_stack(self):
frame = self.last_returns_frame5()
def extract(**kwargs):
return traceback.extract_stack(frame, **kwargs)
def assertEqualExcept(actual, expected, ignore):
self.assertEqual(actual[:ignore], expected[:ignore])
self.assertEqual(actual[ignore+1:], expected[ignore+1:])
self.assertEqual(len(actual), len(expected))
with support.swap_attr(sys, 'tracebacklimit', 1000):

Edit:
And in many tests, the same method is used for the use of tracebacklimit.

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Oct 21, 2024

And this change passes the existing test for tracebacklimit. To ensure accuracy, I verified that frame_gen is empty by inserting a print into _extract_from_extended_frame_gen, and it actually behaves in accordance with the original logic of the program.

test code:

def f():
    f()

f()

changed diff:

--- a/Lib/traceback.py
+++ b/Lib/traceback.py
@@ -455,7 +455,7 @@ def _extract_from_extended_frame_gen(klass, frame_gen, *, limit=None,
         # information is not available.
         builtin_limit = limit is BUILTIN_EXCEPTION_LIMIT
         if limit is None or builtin_limit:
-            limit = getattr(sys, 'tracebacklimit', None)
+            limit = getattr(sys, 'tracebacklimit', 1_000)
             if limit is not None and limit < 0:
                 limit = 0
         if limit is not None:
@@ -466,7 +466,8 @@ def _extract_from_extended_frame_gen(klass, frame_gen, *, limit=None,
                 frame_gen = itertools.islice(frame_gen, limit)
             else:
                 frame_gen = collections.deque(frame_gen, maxlen=-limit)
-
+        for frame in frame_gen:
+            print(frame)
         result = klass()
         fnames = set()

changed output:

(<frame at 0x000001D8F6E1AF00, file 'E:\\code\\github\\cpython\\alpha\\cpython-main\\Users\\test.py', line 4, code f>, (4, 4, 4, 7))
Traceback (most recent call last):
  File "E:\code\github\cpython\alpha\cpython-main\Users\test.py", line 6, in <module>
    f()
    ~^^
  File "E:\code\github\cpython\alpha\cpython-main\Users\test.py", line 4, in f
    f()
    ~^^
  File "E:\code\github\cpython\alpha\cpython-main\Users\test.py", line 4, in f
    f()
    ~^^
  File "E:\code\github\cpython\alpha\cpython-main\Users\test.py", line 4, in f
    f()
    ~^^
  [Previous line repeated 996 more times]
RecursionError: maximum recursion depth exceeded

When I remove 1_000

index 0fe7187..1b5f4eb 100644
--- a/Lib/traceback.py
+++ b/Lib/traceback.py
@@ -466,7 +466,8 @@ def _extract_from_extended_frame_gen(klass, frame_gen, *, limit=None,
                 frame_gen = itertools.islice(frame_gen, limit)
             else:
                 frame_gen = collections.deque(frame_gen, maxlen=-limit)
-
+        for frame in frame_gen:
+            print(frame)
         result = klass()
         fnames = set()
         for f, (lineno, end_lineno, colno, end_colno) in frame_gen:

For the above test code

(<frame at 0x000001D8A732B1C0, file 'E:\\code\\github\\cpython\\alpha\\cpython-main\\Users\\test.py', line 4, code f>, (4, 4, 4, 7))
(<frame at 0x000001D8A732B110, file 'E:\\code\\github\\cpython\\alpha\\cpython-main\\Users\\test.py', line 4, code f>, (4, 4, 4, 7))
(<frame at 0x000001D8A732B060, file 'E:\\code\\github\\cpython\\alpha\\cpython-main\\Users\\test.py', line 4, code f>, (4, 4, 4, 7))
(<frame at 0x000001D8A732AFB0, file 'E:\\code\\github\\cpython\\alpha\\cpython-main\\Users\\test.py', line 4, code f>, (4, 4, 4, 7))
(<frame at 0x000001D8A732AF00, file 'E:\\code\\github\\cpython\\alpha\\cpython-main\\Users\\test.py', line 4, code f>, (4, 4, 4, 7))
RecursionError: maximum recursion depth exceeded

@rruuaanng rruuaanng requested a review from vstinner October 21, 2024 11:03
@rruuaanng
Copy link
Contributor Author

I actually feel like this change could probably be merged with the PR that adds tracebacklimit, since it seems like changing traceback.py separately is a bit abrupt (mainly because I'm confused about the test, I'm not sure if I need to add a duplicate test, because changing it to 1000 doesn't seem to have any effect on the test, because when tracebacklimit doesn't exist in the test code, its value will be 1000).

@rruuaanng rruuaanng marked this pull request as ready for review October 21, 2024 12:57
@rruuaanng
Copy link
Contributor Author

I have added a test that checks if the call stack is 1000. Can it skip NEWS? It seems that this change is implicit.

@rruuaanng
Copy link
Contributor Author

I'll close this PR and hand the change over to the original author for fix.

@rruuaanng rruuaanng closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants