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

gh-113655: Reduce recursion limit to a safe number for Windows #113668

Closed
wants to merge 2 commits into from

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Jan 2, 2024

#113397 recently increased the C recursion limit from 1,500 to 8,000. Unfortunately, this has caused the Windows release buildbots as well as PGO builds to fail with a stack overflow.

The reason, as far as I can tell, is that while there was a set of experiments to determine the largest number that would work for Windows (#113328) (which passed the buildbots), that didn't include the new test that was added in the merged PR #113397 (test_functools:test_lru_recursion) and that new test pushes the stack farther than any of the other tests.

Unfortunately, it doesn't seem to be enough to just limit the test, Py_C_RECURSION_LIMIT also has to be reduced to prevent a C stack overflow.

@mdboom mdboom added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 2, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mdboom for commit 5d5ab9f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 2, 2024
@mdboom mdboom added OS-windows 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Jan 2, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mdboom for commit 5d5ab9f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 2, 2024
@mdboom mdboom added needs backport to 3.12 bug and security fixes skip news labels Jan 2, 2024
@gvanrossum
Copy link
Member

I expect this isn't going to make many people happy -- on other platforms, the stack has way more space (I found that 80,000 worked fine on Mac). I recall around Xmas doing an experiment showing that -- but I cannot find the discussion now (maybe it was on Discourse?).

@mdboom
Copy link
Contributor Author

mdboom commented Jan 3, 2024

I expect this isn't going to make many people happy

Agreed. However, the 4000 here is still more than the 1500 that it was prior to #113397 (two weeks ago). And if the goal is to not make it easily possibly to smash the C stack, I think we need to merge something like this. I'd suggest we merge something simple like this for now to restore safety and then consider other options later.

Perhaps we can find a way to increase the stack on Windows, or we just have different hard limits on different platforms (but maybe the same defaults). But all that seems to warrant further discussion.

@@ -1876,8 +1876,7 @@ def fib(n):

if not support.Py_DEBUG:
with support.infinite_recursion():
fib(1250)

self.assertRaises(RecursionError, fib, 2000)
Copy link
Member

Choose a reason for hiding this comment

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

I believe this violates the intention of the test -- it wants to ensure that we can recurse at least a minimum given number of times. Maybe we need both?

Copy link
Member

Choose a reason for hiding this comment

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

We already have tests for maxing out the recursion limit, both for C and Python recursion, so we probably don't need to add any. But there is no harm in having more tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we already have those tests, I wonder why they aren't likewise failing... Do you know which those are?

@gvanrossum
Copy link
Member

Trying to repro this, I still get the same crash in test_functools, after switching to your code. Maybe there's something I should clean up first? I honestly don't know how to get back to a pristine repo (deleting old .obj files etc.) on Windows (on UNIX I'd just use make clobber or make distclean).

@mdboom
Copy link
Contributor Author

mdboom commented Jan 5, 2024

Trying to repro this, I still get the same crash in test_functools

Strange, though I admit I don't develop on Windows much either so I could be doing something wrong here.

@mdboom
Copy link
Contributor Author

mdboom commented Jan 19, 2024

Closing as obsolete.

@mdboom mdboom closed this Jan 19, 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.

4 participants