gh-134935: Use RecursionError to check for circular references #134937
Conversation
Other fast JSON encoders rely on RecursionError to detect circular references: - ultrajson -- OverflowError: Maximum recursion level reached - orjson -- TypeError: Recursion limit reached Python's json module kept a dictionary with visited objects. It is too much work for a nicer error message. Instead raise a ValueError on RecursionError to keep the API but do not track the objects.
|
It's not guaranteed that Python will recover:
And I would argue it's not backwards compatible, because it no longer checks for circular references, simply relying on the recursion limit. Does this improve the performance in any way compared to |
| @@ -22,7 +22,7 @@ def test_listrecursion(self): | |||
| try: | |||
| self.dumps(x) | |||
| except ValueError as exc: | |||
| self.assertEqual(exc.__notes__, ["when serializing list item 0"]*2) | |||
| self.assertEqual(exc.__notes__[:2], ["when serializing list item 0"]*2) | |||
There was a problem hiding this comment.
what consequences of this change led to the need to limit how many notes we assert on?
what additional notes are now added?
|
The old approach of custom reference tracking detected a cycle with a stack
depth of 2 and there were only 2 messages. The one based on recursion error
exhausts the stack so it gets more messages. I don't know what is the best
approach here: to check if message is in list or limit the size of notes.
Before 3.11 it worked without notes so I don't think preserving the same
content of notes is a big thing.
…On Fri, 30 May 2025, 18:49 Gregory P. Smith, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Lib/test/test_json/test_recursion.py
<#134937 (comment)>:
> @@ -22,7 +22,7 @@ def test_listrecursion(self):
try:
self.dumps(x)
except ValueError as exc:
- self.assertEqual(exc.__notes__, ["when serializing list item 0"]*2)
+ self.assertEqual(exc.__notes__[:2], ["when serializing list item 0"]*2)
what consequences of this change led to the need to limit how many notes
we assert on?
what additional notes are now added?
—
Reply to this email directly, view it on GitHub
<#134937 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAZGSDAC2ZS2LW5DC3Q35T3BCD25AVCNFSM6AAAAAB6IK6DYKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQOBRHA4DQOBVGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
having notes grow huge with the stack depth destroys the value of the notes as the error message produced when the exception is seen will be huge. perhaps something like this to constrain the notes growth: 971abd2 |
|
I do not think this is correct. See reasons on the issue. |
Other fast JSON encoders rely on RecursionError to detect circular references:
Python's json module kept a dictionary with visited objects. It is too much work for a nicer error message. Instead raise a ValueError on RecursionError to keep the API but do not track the objects.