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-116767: fix crash on 'async with' with many context managers #118348

Merged
merged 7 commits into from May 1, 2024

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Apr 27, 2024

@iritkatriel iritkatriel marked this pull request as draft April 27, 2024 18:43
@iritkatriel iritkatriel marked this pull request as ready for review April 27, 2024 21:12
@encukou
Copy link
Member

encukou commented Apr 29, 2024

Thank you! I can't say I follow all of what's happening here, but I have a better picture now :)

This is technically backwards incompatible -- the limit is decreased even for plain with (or while, etc.) in a generator or async function. For example, these worked in Python 3.12:

async def t():
    with c,c,c,c,c,c,c,c,c,c,c,c,c,c,c,c,c,c,c,c: c

def g():
    with c,c,c,c,c,c,c,c,c,c,c,c,c,c,c,c,c,c,c,c: yield

Should CO_MAXBLOCKS be bumped up to 21 to allow this?

@iritkatriel
Copy link
Member Author

Should CO_MAXBLOCKS be bumped up to 21 to allow this?

We could, though this maximum static depth is not documented anywhere and I seriously doubt anyone hits it.

@iritkatriel
Copy link
Member Author

Should CO_MAXBLOCKS be bumped up to 21 to allow this?

We could, though this maximum static depth is not documented anywhere and I seriously doubt anyone hits it.

I made the change, it changes semantics the other way (allowing one more nested level in the non-generator case). I think that's ok. (The compiler could distinguish the cases and use a different limit for each, but I don't think it's worth the added complexity).

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

Thank you!

I guess the kind of use-case that would hit this is auto-generated code. Bumping to 21 is easier than researching if someone does that.

@encukou encukou merged commit c1bf487 into python:main May 1, 2024
37 checks passed
@miss-islington-app
Copy link

Thanks @iritkatriel for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @iritkatriel and @encukou, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker c1bf4874c1e9db2beda1d62c8c241229783c789b 3.12

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Ubuntu NoGIL 3.x has failed when building commit c1bf487.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1225/builds/2170) and take a look at the build logs.
  4. Check if the failure is related to this commit (c1bf487) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1225/builds/2170

Failed tests:

  • test_eintr
  • test.test_concurrent_futures.test_process_pool

Failed subtests:

  • test_flock - main.FNTLEINTRTest.test_flock
  • test_map_timeout - test.test_concurrent_futures.test_process_pool.ProcessPoolSpawnProcessPoolExecutorTest.test_map_timeout
  • test_all - test.test_eintr.EINTRTests.test_all
  • test_lockf - main.FNTLEINTRTest.test_lockf
  • test_map_timeout - test.test_concurrent_futures.test_process_pool.ProcessPoolForkserverProcessPoolExecutorTest.test_map_timeout

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.nogil/build/Lib/test/_test_eintr.py", line 535, in test_flock
    self._lock(fcntl.flock, "flock")
    ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.nogil/build/Lib/test/_test_eintr.py", line 517, in _lock
    raise Exception("failed to sync child in %.1f sec" % dt)
Exception: failed to sync child in 300.0 sec


Traceback (most recent call last):
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.nogil/build/Lib/test/test_eintr.py", line 17, in test_all
    script_helper.run_test_script(script)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.nogil/build/Lib/test/support/script_helper.py", line 316, in run_test_script
    raise AssertionError(f"{name} failed")
AssertionError: script _test_eintr.py failed


Traceback (most recent call last):
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.nogil/build/Lib/test/_test_eintr.py", line 532, in test_lockf
    self._lock(fcntl.lockf, "lockf")
    ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.nogil/build/Lib/test/_test_eintr.py", line 517, in _lock
    raise Exception("failed to sync child in %.1f sec" % dt)
Exception: failed to sync child in 300.7 sec


Traceback (most recent call last):
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.nogil/build/Lib/test/test_concurrent_futures/executor.py", line 71, in test_map_timeout
    self.assertEqual([None, None], results)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: Lists differ: [None, None] != []


Traceback (most recent call last):
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.nogil/build/Lib/test/_test_eintr.py", line 532, in test_lockf
    self._lock(fcntl.lockf, "lockf")
    ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.nogil/build/Lib/test/_test_eintr.py", line 517, in _lock
    raise Exception("failed to sync child in %.1f sec" % dt)
Exception: failed to sync child in 300.0 sec

iritkatriel added a commit to iritkatriel/cpython that referenced this pull request May 1, 2024
…pythonGH-118348)

Account for `add_stopiteration_handler` pushing a block for `async with`.
To allow generator functions that previously almost hit the `CO_MAXBLOCKS`
limit by nesting non-async blocks, the limit is increased by 1.
This increase allows one more block in non-generator functions.

(cherry picked from commit c1bf487)
@bedevere-app
Copy link

bedevere-app bot commented May 1, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label May 1, 2024
iritkatriel added a commit that referenced this pull request May 1, 2024
GH-118348) (#118477)

gh-116767: fix crash on 'async with' with many context managers (GH-118348)

Account for `add_stopiteration_handler` pushing a block for `async with`.
To allow generator functions that previously almost hit the `CO_MAXBLOCKS`
limit by nesting non-async blocks, the limit is increased by 1.
This increase allows one more block in non-generator functions.

(cherry picked from commit c1bf487)
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
…pythonGH-118348)

Account for `add_stopiteration_handler` pushing a block for `async with`.
To allow generator functions that previously almost hit the `CO_MAXBLOCKS`
limit by nesting non-async blocks, the limit is increased by 1.
This increase allows one more block in non-generator functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
3 participants