KEMBAR78
gh-116767: fix crash on 'async with' with many context managers by iritkatriel · Pull Request #118348 · python/cpython · GitHub
Skip to content

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
@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 only 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

Development

Successfully merging this pull request may close these issues.

Python/flowgraph.c:701: basicblock *push_except_block(struct _PyCfgExceptStack *, cfg_instr *): Assertion `stack->depth <= CO_MAXBLOCKS' failed

3 participants