gh-120321: Make gi_frame_state transitions atomic in FT build #142599

colesbury · 2025-12-11T20:47:03Z

This makes generator frame state transitions atomic in the free threading build, which avoids segfaults when trying to execute a generator from multiple threads concurrently.

There are still a few operations that aren't thread-safe and may crash if performed concurrently on the same generator/coroutine:

Accessing gi_yieldfrom/cr_await/ag_await
Accessing gi_frame/cr_frame/ag_frame
Async generator operations

Issue: SIGSEGV with generators in free-threaded build #120321

This makes generator frame state transitions atomic in the free threading build, which avoids segfaults when trying to execute a generator from multiple threads concurrently. There are still a few operations that aren't thread-safe and may crash if performed concurrently on the same generator/coroutine: * Accessing gi_yieldfrom/cr_await/ag_await * Accessing gi_frame/cr_frame/ag_frame * Async generator operations

mpage · 2025-12-12T19:29:15Z

Python/bytecodes.c

+            FT_ATOMIC_STORE_INT8_RELEASE(gen->gi_frame_state, FRAME_SUSPENDED + oparg);
+#ifdef Py_GIL_DISABLED
+            ((_PyThreadStateImpl *)tstate)->gen_last_frame_state = FRAME_SUSPENDED + oparg;
+#endif


Would it make sense to factor this out into a helper function? It's duplicated in ceval.c. Also, having helper to update both would reduce the chance that we update one without the other.

I've refactored it out. I also moved the functions to ceval_macros.h from pycore_genobject.h because:

they are only used in ceval.c and bytecodes.c

I want to avoid adding additional includes to pycore_genobject.h because I think it's pulled in by internal headers that some third-party projects tend to include.

Include/internal/pycore_genobject.h

kumaraditya303 · 2025-12-14T17:32:18Z

Objects/genobject.c


-    gen->gi_frame_state = FRAME_EXECUTING;
+#if defined(Py_GIL_DISABLED) && defined(Py_DEBUG)
+    ((_PyThreadStateImpl *)tstate)->gen_last_frame_state = FRAME_EXECUTING;


why is this set only in debug builds?

I changed this to #if defined(Py_GIL_DISABLED). It's not really needed at all, but it'll help catch bugs in case some ceval code forgets to set gen_last_frame_state before returning.

In other words, when _PyEval_EvalFrame returns, gen_last_frame_state must be either FRAME_SUSPENDED[_YIELD_FROM] or FRAME_CLEARED and this helps differentiate this call to _PyEval_EvalFrame from earlier calls in the same thread.

Objects/genobject.c

Include/internal/pycore_genobject.h

Co-authored-by: Kumar Aditya <[email protected]>

Objects/genobject.c

markshannon

The changes to bytecodes.c and ceval/cevalmacros look good.

I'm puzzled by the gen_last_frame_state field on the thread state.
It seems unnecessary and, if it is necessary, broken in case of reentrancy.

markshannon · 2025-12-16T08:42:14Z

Objects/genobject.c

+    // Grab the last frame state from the thread state instead of the
+    // generator, as it may have changed if another thread resumed this
+    // generator.
+    int8_t frame_state = ((_PyThreadStateImpl *)tstate)->gen_last_frame_state;


How can another thread resume this generator? This thread is executing it, so no other thread can run it.

This thread is no longer executing the generator once it sets the frame state to suspended in YIELD_VALUE. Between that line in bytecodes.c and here, another thread can immediately start executing the same generator. That isn't true with the GIL because there are no opportunities for reentrancy or GIL release between YIELD_VALUE and the return from _PyEval_Frame. That lack of opportunities for reentrancy is also why it's safe to store it on the thread state.

OK, I see what this issue is.
What you want is an additional return value, to distinguish between a yield and a return.

I think the code would be clearer, if gen_last_frame_state field were renamed to something like generator_return_kind. Set it to RETURN before the call, and set it to YIELD in YIELD_VALUE.
Instead of testing for FRAME_STATE_SUSPENDED(frame_state) test for generator_return_kind != RETURN

Also add a comment about why it is needed.

I've renamed the field and adjusted the condition.

However, we need to set gen_last_frame_state in gen_clear_frame (or elsewhere in RETURN_VALUE). It's not enough to set it to RETURN before the _PyEval_EvalFrame call because generators can send to other generator. The "inner" generator will overwrite the gen_last_frame_state. (See the test failures on 4b82b8c)

markshannon · 2025-12-16T08:44:02Z

Objects/genobject.c

+           FRAME_STATE_SUSPENDED(frame_state));
+
+    if (!_Py_GEN_SET_FRAME_STATE(gen, frame_state, FRAME_EXECUTING)) {
+        goto retry;


No need for a goto. You can use do ... while (!_Py_GEN_SET_FRAME_STATE(gen, frame_state, FRAME_EXECUTING));

I've switched from gotos to do/while loops

Also use it in both the default and FT builds.

Generator calls may be nested so it's not sufficient to set the field before the call to PyEval_Frame.

markshannon · 2025-12-18T12:07:32Z

The changes look good.

I'm concerned about the complexity of this for such an obscure use case, but I can't suggest a better way of doing it.

mpage

LGTM!

Objects/genobject.c

Co-authored-by: Kumar Aditya <[email protected]>

cdce8p · 2025-12-20T03:34:09Z

Objects/genobject.c

+            if (!_Py_GEN_TRY_SET_FRAME_STATE(gen, frame_state, FRAME_CLEARED)) {
+                continue;
+            }


This check seems to trigger a new compiler warning during build.

Objects/genobject.c:426:17: warning: code will never be executed [-Wunreachable-code] continue;

Tested on MacOS with gcc.

colesbury added skip news topic-free-threading labels Dec 11, 2025

bedevere-app bot mentioned this pull request Dec 11, 2025

SIGSEGV with generators in free-threaded build #120321

Open

colesbury added 2 commits December 12, 2025 11:43

Add some tests

77fee35

Merge branch 'main' into pythongh-120321-gen-atomic

1236692

colesbury marked this pull request as ready for review December 12, 2025 16:44

colesbury requested review from ericsnowcurrently and markshannon as code owners December 12, 2025 16:44

bedevere-app bot added the awaiting core review label Dec 12, 2025

colesbury requested a review from mpage December 12, 2025 16:44

Remove unused import

ac3974d

colesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Dec 12, 2025

This comment was marked as outdated.

Sign in to view

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Dec 12, 2025

mpage reviewed Dec 12, 2025

View reviewed changes

kumaraditya303 reviewed Dec 14, 2025

View reviewed changes

Objects/genobject.c Outdated Show resolved Hide resolved

kumaraditya303 reviewed Dec 14, 2025

View reviewed changes

Objects/genobject.c Outdated Show resolved Hide resolved

kumaraditya303 reviewed Dec 14, 2025

View reviewed changes

Include/internal/pycore_genobject.h Outdated Show resolved Hide resolved

Update Objects/genobject.c

b285594

Co-authored-by: Kumar Aditya <[email protected]>

colesbury requested a review from pablogsal as a code owner December 15, 2025 16:43

colesbury and others added 3 commits December 15, 2025 11:44

Update Objects/genobject.c

9c1c0fe

Co-authored-by: Kumar Aditya <[email protected]>

Changes from review

3876dd0

Merge branch 'main' into pythongh-120321-gen-atomic

46a8699

colesbury requested review from kumaraditya303 and mpage December 15, 2025 16:53

Remove extra whitespace

46a8e84

kumaraditya303 reviewed Dec 16, 2025

View reviewed changes

Objects/genobject.c Show resolved Hide resolved

kumaraditya303 reviewed Dec 16, 2025

View reviewed changes

Objects/genobject.c Outdated Show resolved Hide resolved

markshannon reviewed Dec 16, 2025

View reviewed changes

colesbury added 7 commits December 16, 2025 12:04

Merge branch 'main' into pythongh-120321-gen-atomic

15c2604

Rename _Py_GEN_SET_FRAME_STATE to _Py_GEN_TRY_SET_FRAME_STATE

64155ab

Switch from goto to do/while

f135b64

Merge branch 'main' into pythongh-120321-gen-atomic

f532899

Rename field to generator_return_kind

4b82b8c

Also use it in both the default and FT builds.

Set generator_return_kind in clear_gen_frame

832e413

Generator calls may be nested so it's not sufficient to set the field before the call to PyEval_Frame.

Fix stray typo

f5c10f9

colesbury requested a review from markshannon December 17, 2025 19:34

mpage approved these changes Dec 18, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Dec 18, 2025

kumaraditya303 reviewed Dec 19, 2025

View reviewed changes

Objects/genobject.c Outdated Show resolved Hide resolved

kumaraditya303 approved these changes Dec 19, 2025

View reviewed changes

colesbury and others added 2 commits December 19, 2025 13:44

Update Objects/genobject.c

8a86ce0

Co-authored-by: Kumar Aditya <[email protected]>

Merge branch 'main' into pythongh-120321-gen-atomic

a874397

colesbury enabled auto-merge (squash) December 19, 2025 18:45

colesbury merged commit 08bc03f into python:main Dec 19, 2025
77 checks passed

bedevere-app bot removed the awaiting merge label Dec 19, 2025

cdce8p reviewed Dec 20, 2025

View reviewed changes

Uh oh!

gh-120321: Make gi_frame_state transitions atomic in FT build #142599

gh-120321: Make gi_frame_state transitions atomic in FT build #142599

Conversation

colesbury commented Dec 11, 2025 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markshannon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markshannon Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markshannon commented Dec 18, 2025

Uh oh!

mpage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

colesbury commented Dec 11, 2025 •

edited by bedevere-app bot

Loading

markshannon left a comment •

edited

Loading

markshannon Dec 17, 2025 •

edited

Loading