gh-108851: Fix tomllib recursion tests by vstinner · Pull Request #108853 · python/cpython

vstinner · 2023-09-03T20:39:34Z

sys.setrecursionlimit() now allows setting the limit to the current recursion depth.
Add get_recursion_available() and get_recursion_depth() functions to the test.support module.
Change infinite_recursion() default max_depth from 75 to 100.
Fix test_tomllib recursion tests for WASI buildbots: reduce the recursion limit and compute the maximum nested array/dict depending on the current available recursion limit.

Issue: test_tomllib.test_inline_array_recursion_limit() failed on wasm32-wasi 3.11 buildbot #108851

vstinner · 2023-09-03T20:40:16Z

This PR combines 3 changes at once. IMO it's better to change all of them at the same time to ease backports, rather than having 3 commits (more work for little benefits).

cc @serhiy-storchaka

vstinner · 2023-09-03T20:50:35Z

The test_tomllib failure can be reproduced on Linux by reducing the recursion limit. Use this patch:

diff --git a/Lib/test/test_tomllib/test_misc.py b/Lib/test/test_tomllib/test_misc.py
index a477a219fd..43357f2064 100644
--- a/Lib/test/test_tomllib/test_misc.py
+++ b/Lib/test/test_tomllib/test_misc.py
@@ -10,6 +10,8 @@
import tempfile
import unittest
+sys.setrecursionlimit(250)
+
from . import tomllib

On the main branch, I reproduce the issue:

$ ./python -m test test_tomllib -v
(...)
======================================================================
ERROR: test_inline_array_recursion_limit (test.test_tomllib.test_misc.TestMiscellaneous.test_inline_array_recursion_limit)
----------------------------------------------------------------------
(...)
RecursionError: maximum recursion depth exceeded
======================================================================
ERROR: test_inline_table_recursion_limit (test.test_tomllib.test_misc.TestMiscellaneous.test_inline_table_recursion_limit)
----------------------------------------------------------------------
(...)
RecursionError: maximum recursion depth exceeded
(...)

With this PR and the patch to trigger the bug: test_tomllib pass successfully.

brettcannon · 2023-09-05T19:55:56Z

+ # it can raise RecursionError
+ return
+ get_depth = support.get_recursion_depth()
+ print(f"test_recursive: {depth}/{limit}: "


Should subTest() used instead of printing out the values?

This is a script run by test_get_recursion_depth(). You should not see the output unless the test crash or fails. It's not written with unittest.

* Add get_recursion_available() and get_recursion_depth() functions to the test.support module. * Change infinite_recursion() default max_depth from 75 to 100. * Fix test_tomllib recursion tests for WASI buildbots: reduce the recursion limit and compute the maximum nested array/dict depending on the current available recursion limit. * test.pythoninfo logs sys.getrecursionlimit() * Enhance test_sys tests on sys.getrecursionlimit() and sys.setrecursionlimit().

vstinner · 2023-09-05T20:28:55Z

I rebased my PR and I revert the sys.setrecursionlimit().

The sys.setrecursionlimit() change was fun to do, play with the bare minimum. But it introduces a weird behavior (you can no longer call any Python function, not great.) Instead of I changed the support.infinite_recursion() minimum to 3, instead of 2. In practice, this function should be called at least with 50 (default is 100 with my change).

vstinner · 2023-09-05T20:29:11Z

@brettcannon: Would you mind to review the updated PR?

serhiy-storchaka

LGTM.

You may also add simple docstrings for new functions. Just one line would be enough.

vstinner · 2023-09-06T15:33:28Z

You may also add simple docstrings for new functions. Just one line would be enough.

vstinner · 2023-09-06T15:33:45Z

You may also add simple docstrings for new functions. Just one line would be enough.

I added docstrings to the 2 newly added test.support functions.

bedevere-bot · 2023-09-06T15:34:37Z

There's a new commit after the PR has been approved.

@serhiy-storchaka: please review the changes made to this pull request.

miss-islington · 2023-09-06T15:34:43Z

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington · 2023-09-06T15:34:44Z

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington · 2023-09-06T15:34:50Z

Sorry, @vstinner, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 8ff11425783806f8cb78e99f667546b1f7f3428e 3.11

* Add get_recursion_available() and get_recursion_depth() functions to the test.support module. * Change infinite_recursion() default max_depth from 75 to 100. * Fix test_tomllib recursion tests for WASI buildbots: reduce the recursion limit and compute the maximum nested array/dict depending on the current available recursion limit. * test.pythoninfo logs sys.getrecursionlimit(). * Enhance test_sys tests on sys.getrecursionlimit() and sys.setrecursionlimit(). (cherry picked from commit 8ff1142) Co-authored-by: Victor Stinner <vstinner@python.org>

bedevere-bot · 2023-09-06T15:34:56Z

GH-109012 is a backport of this pull request to the 3.12 branch.

* Add get_recursion_available() and get_recursion_depth() functions to the test.support module. * Change infinite_recursion() default max_depth from 75 to 100. * Fix test_tomllib recursion tests for WASI buildbots: reduce the recursion limit and compute the maximum nested array/dict depending on the current available recursion limit. * test.pythoninfo logs sys.getrecursionlimit(). * Enhance test_sys tests on sys.getrecursionlimit() and sys.setrecursionlimit(). Backport notes: * Set support.infinite_recursion() minimumum to 4 frames. * test_support.test_get_recursion_depth() uses limit-2, apparently f-string counts for 2 frames in Python 3.11. * test_sys.test_setrecursionlimit_to_depth() tests depth+2 instead of depth+1. (cherry picked from commit 8ff1142)

* Add get_recursion_available() and get_recursion_depth() functions to the test.support module. * Change infinite_recursion() default max_depth from 75 to 100. * Fix test_tomllib recursion tests for WASI buildbots: reduce the recursion limit and compute the maximum nested array/dict depending on the current available recursion limit. * test.pythoninfo logs sys.getrecursionlimit(). * Enhance test_sys tests on sys.getrecursionlimit() and sys.setrecursionlimit(). Backport notes: * Set support.infinite_recursion() minimum to 4 frames. * test_support.test_get_recursion_depth() uses limit-2, apparently f-string counts for 2 frames in Python 3.11. * test_sys.test_setrecursionlimit_to_depth() tests depth+2 instead of depth+1. (cherry picked from commit 8ff1142)

bedevere-bot · 2023-09-06T16:00:22Z

GH-109013 is a backport of this pull request to the 3.11 branch.

vstinner · 2023-09-06T16:01:49Z

Thanks for the review @brettcannon and @serhiy-storchaka. I will try to keep an eye on the WASI buildbots ;-)

gh-108851: Fix tomllib recursion tests (#108853) * Add get_recursion_available() and get_recursion_depth() functions to the test.support module. * Change infinite_recursion() default max_depth from 75 to 100. * Fix test_tomllib recursion tests for WASI buildbots: reduce the recursion limit and compute the maximum nested array/dict depending on the current available recursion limit. * test.pythoninfo logs sys.getrecursionlimit(). * Enhance test_sys tests on sys.getrecursionlimit() and sys.setrecursionlimit(). Backport notes: * Set support.infinite_recursion() minimum to 4 frames. * test_support.test_get_recursion_depth() uses limit-2, apparently f-string counts for 2 frames in Python 3.11. * test_sys.test_setrecursionlimit_to_depth() tests depth+2 instead of depth+1. (cherry picked from commit 8ff1142)

brettcannon · 2023-09-06T18:21:25Z

Thanks for the fix!

vstinner · 2023-09-07T05:12:43Z

The fix works as expected: wasm32-wasi 3.11917 buildbot is back to green (success).

first successful build: https://buildbot.python.org/all/#/builders/1047/builds/908

By the way, I added sys.getrecursionlimit to test.pythoninfo. On WASI, the limit is lower, as you can now see on test.pythoninfo:

sys.getrecursionlimit: 600

On other platforms, the default is 1,000 frames:

$ make pythoninfo|grep getrecursionlimit
sys.getrecursionlimit: 1000

vstinner · 2023-09-08T08:43:51Z

Oh, I was sure that I removed #self.assertEqual(available, 2) commented code from test_support.py but it back from death in the merged commit! Well, I will remove it in a following change :-)

gh-108851: Fix tomllib recursion tests (GH-108853) * Add get_recursion_available() and get_recursion_depth() functions to the test.support module. * Change infinite_recursion() default max_depth from 75 to 100. * Fix test_tomllib recursion tests for WASI buildbots: reduce the recursion limit and compute the maximum nested array/dict depending on the current available recursion limit. * test.pythoninfo logs sys.getrecursionlimit(). * Enhance test_sys tests on sys.getrecursionlimit() and sys.setrecursionlimit(). (cherry picked from commit 8ff1142) Co-authored-by: Victor Stinner <vstinner@python.org>

vstinner requested review from encukou and hauntsaninja as code owners September 3, 2023 20:39

bedevere-bot added the awaiting core review label Sep 3, 2023

bedevere-bot mentioned this pull request Sep 3, 2023
test_tomllib.test_inline_array_recursion_limit() failed on wasm32-wasi 3.11 buildbot #108851
Closed

brettcannon reviewed Sep 5, 2023
View reviewed changes

vstinner force-pushed the support_recursive branch from b0104a7 to a1b3414Compare September 5, 2023 20:27

brettcannon self-requested a review September 6, 2023 00:22

serhiy-storchaka self-requested a review September 6, 2023 10:36

serhiy-storchaka approved these changes Sep 6, 2023
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 6, 2023

Add docstrings to new test.support functions
d2a77b3

vstinner merged commit 8ff1142 into python:mainSep 6, 2023

vstinner deleted the support_recursive branch September 6, 2023 15:34

bedevere-bot added awaiting core review and removed awaiting merge labels Sep 6, 2023

bedevere-bot requested a review from serhiy-storchaka September 6, 2023 15:34

vstinner added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Sep 6, 2023

miss-islington assigned vstinner Sep 6, 2023

bedevere-bot removed the needs backport to 3.12 only security fixes label Sep 6, 2023

bedevere-bot removed the needs backport to 3.11 only security fixes label Sep 6, 2023

Uh oh!

Uh oh!

Conversation

vstinner commented Sep 3, 2023• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Sep 3, 2023

Uh oh!

vstinner commented Sep 3, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brettcannonSep 5, 2023

Choose a reason for hiding this comment

Uh oh!

vstinnerSep 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner commented Sep 5, 2023

Uh oh!

vstinner commented Sep 5, 2023

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 6, 2023

Uh oh!

vstinner commented Sep 6, 2023

Uh oh!

bedevere-bot commented Sep 6, 2023

Uh oh!

miss-islington commented Sep 6, 2023

Uh oh!

miss-islington commented Sep 6, 2023

Uh oh!

miss-islington commented Sep 6, 2023

Uh oh!

bedevere-bot commented Sep 6, 2023

Uh oh!

bedevere-bot commented Sep 6, 2023

Uh oh!

vstinner commented Sep 6, 2023

Uh oh!

brettcannon commented Sep 6, 2023

Uh oh!

vstinner commented Sep 7, 2023

Uh oh!

vstinner commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vstinner commented Sep 3, 2023•
edited by bedevere-bot
Loading