Skip to content

Commit b3b504c

Browse files
committed
gh-151518: Avoid STW starvation of attaching threads
Free-threaded stop-the-world pauses can otherwise starve a thread trying to reattach after it was suspended while detached. A tight manual gc.collect() loop can release and immediately request the next stop-the-world pause, repeatedly parking the detached thread before it can attach and make progress. Add a distinct _Py_THREAD_SUSPENDED_DETACHED state for tstates parked from DETACHED. tstate_wait_attach() marks an attach waiter only after observing that detached-origin suspended state, and park_detached_threads() skips only those active waiters on later stop-the-world passes. The ordinary successful tstate_try_attach() path remains the baseline CAS-only path. Teach the related stop-the-world paths about both suspended states, including start_the_world() and tstate_delete_common(). Keep the new wait flag after the existing hot free-threaded _PyThreadStateImpl fields so their offsets do not move. Add a free-threaded GC regression test that runs a subprocess with a tight gc.collect() worker and verifies the main thread can reattach after sleeping and stop the worker.
1 parent 89c67a9 commit b3b504c

6 files changed

Lines changed: 116 additions & 30 deletions

File tree

Include/cpython/pystate.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,7 @@ struct _ts {
118118

119119
int _whence;
120120

121-
/* Thread state (_Py_THREAD_ATTACHED, _Py_THREAD_DETACHED, _Py_THREAD_SUSPENDED).
122-
See Include/internal/pycore_pystate.h for more details. */
121+
/* Thread state. See Include/internal/pycore_pystate.h for details. */
123122
int state;
124123

125124
int py_recursion_remaining;

Include/internal/pycore_pystate.h

Lines changed: 17 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -21,32 +21,28 @@ extern "C" {
2121
// interpreter at the same time. Only the "bound" thread may perform the
2222
// transitions between "attached" and "detached" on its own PyThreadState.
2323
//
24-
// The "suspended" state is used to implement stop-the-world pauses, such as
25-
// for cyclic garbage collection. It is only used in `--disable-gil` builds.
26-
// The "suspended" state is similar to the "detached" state in that in both
27-
// states the thread is not allowed to call most Python APIs. However, unlike
28-
// the "detached" state, a thread may not transition itself out from the
29-
// "suspended" state. Only the thread performing a stop-the-world pause may
30-
// transition a thread from the "suspended" state back to the "detached" state.
24+
// The "suspended" states are used to implement stop-the-world pauses, such as
25+
// for cyclic garbage collection. They are only used in `--disable-gil` builds.
26+
// They are similar to the "detached" state in that the thread is not allowed
27+
// to call most Python APIs. However, unlike the "detached" state, a thread may
28+
// not transition itself out from a "suspended" state. Only the thread
29+
// performing a stop-the-world pause may transition a thread from a "suspended"
30+
// state back to the "detached" state.
3131
//
3232
// The "shutting down" state is used when the interpreter is being finalized.
3333
// Threads in this state can't do anything other than block the OS thread.
3434
// (See _PyThreadState_HangThread).
3535
//
36-
// State transition diagram:
37-
//
38-
// (bound thread) (stop-the-world thread)
39-
// [attached] <-> [detached] <-> [suspended]
40-
// | ^
41-
// +---------------------------->---------------------------+
42-
// (bound thread)
43-
//
44-
// The (bound thread) and (stop-the-world thread) labels indicate which thread
45-
// is allowed to perform the transition.
46-
#define _Py_THREAD_DETACHED 0
47-
#define _Py_THREAD_ATTACHED 1
48-
#define _Py_THREAD_SUSPENDED 2
49-
#define _Py_THREAD_SHUTTING_DOWN 3
36+
// State transitions:
37+
// Bound thread: attached <-> detached
38+
// attached -> suspended
39+
// Stop-the-world thread: detached <-> suspended-detached
40+
// suspended -> detached
41+
#define _Py_THREAD_DETACHED 0
42+
#define _Py_THREAD_ATTACHED 1
43+
#define _Py_THREAD_SUSPENDED 2
44+
#define _Py_THREAD_SHUTTING_DOWN 3
45+
#define _Py_THREAD_SUSPENDED_DETACHED 4
5046

5147

5248
/* Check if the current thread is the main thread.

Include/internal/pycore_tstate.h

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,14 @@ typedef struct _PyThreadStateImpl {
105105

106106
#ifdef Py_GIL_DISABLED
107107
// gh-144438: Add padding to ensure that the fields above don't share a
108-
// cache line with other allocations.
109-
char __padding[64];
108+
// cache line with other allocations. Reuse the first bytes of the padding
109+
// for a cold stop-the-world flag without growing the thread state.
110+
union {
111+
// Set while the thread is waiting to attach after a
112+
// stop-the-world pause suspended it while detached.
113+
int stw_attach_waiting;
114+
char __padding[64];
115+
};
110116
#endif
111117
} _PyThreadStateImpl;
112118

Lib/test/test_free_threading/test_gc.py

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,15 @@
11
import unittest
22

3+
import subprocess
4+
import sys
5+
import textwrap
36
import threading
47
from threading import Thread
58
import time
69
from unittest import TestCase
710
import gc
811

12+
from test import support
913
from test.support import threading_helper
1014

1115

@@ -174,6 +178,55 @@ def reader():
174178
with threading_helper.start_threads(threads):
175179
pass
176180

181+
@support.requires_subprocess()
182+
def test_tight_gc_loop_does_not_starve_attach(self):
183+
script = textwrap.dedent("""
184+
import gc
185+
import importlib
186+
import threading
187+
import time
188+
189+
modules = (
190+
"abc", "argparse", "collections", "contextlib",
191+
"decimal", "enum", "functools", "heapq",
192+
"importlib", "inspect", "itertools", "json",
193+
"math", "operator", "random", "re",
194+
)
195+
for name in modules:
196+
importlib.import_module(name)
197+
198+
started = threading.Event()
199+
stop = threading.Event()
200+
201+
def collect():
202+
gc.collect()
203+
started.set()
204+
while not stop.is_set():
205+
gc.collect()
206+
207+
thread = threading.Thread(target=collect, daemon=True)
208+
thread.start()
209+
started.wait()
210+
# Each reattachment must make progress between consecutive pauses.
211+
for _ in range(50):
212+
time.sleep(0.02)
213+
stop.set()
214+
thread.join()
215+
""")
216+
proc = subprocess.run(
217+
[sys.executable, "-I", "-X", "gil=0", "-X", "faulthandler",
218+
"-c", script],
219+
stdout=subprocess.PIPE,
220+
stderr=subprocess.PIPE,
221+
text=True,
222+
timeout=support.SHORT_TIMEOUT,
223+
)
224+
self.assertEqual(
225+
proc.returncode,
226+
0,
227+
f"stdout:\n{proc.stdout}\nstderr:\n{proc.stderr}",
228+
)
229+
177230

178231
if __name__ == "__main__":
179232
unittest.main()
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fix a free-threaded stop-the-world race that could starve a thread reattaching
2+
after being suspended while detached.

Python/pystate.c

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1939,7 +1939,9 @@ tstate_delete_common(PyThreadState *tstate, int release_gil)
19391939
if (tstate->next) {
19401940
tstate->next->prev = tstate->prev;
19411941
}
1942-
if (tstate->state != _Py_THREAD_SUSPENDED) {
1942+
if (tstate->state != _Py_THREAD_SUSPENDED &&
1943+
tstate->state != _Py_THREAD_SUSPENDED_DETACHED)
1944+
{
19431945
// Any ongoing stop-the-world request should not wait for us because
19441946
// our thread is getting deleted.
19451947
if (interp->stoptheworld.requested) {
@@ -2237,9 +2239,22 @@ tstate_set_detached(PyThreadState *tstate, int detached_state)
22372239
static void
22382240
tstate_wait_attach(PyThreadState *tstate)
22392241
{
2242+
#ifdef Py_GIL_DISABLED
2243+
_PyThreadStateImpl *tstate_impl = (_PyThreadStateImpl *)tstate;
2244+
int stw_attach_waiting = 0;
2245+
#endif
22402246
do {
22412247
int state = _Py_atomic_load_int_relaxed(&tstate->state);
2242-
if (state == _Py_THREAD_SUSPENDED) {
2248+
if (state == _Py_THREAD_SUSPENDED ||
2249+
state == _Py_THREAD_SUSPENDED_DETACHED)
2250+
{
2251+
#ifdef Py_GIL_DISABLED
2252+
if (state == _Py_THREAD_SUSPENDED_DETACHED) {
2253+
stw_attach_waiting = 1;
2254+
_Py_atomic_store_int_relaxed(
2255+
&tstate_impl->stw_attach_waiting, 1);
2256+
}
2257+
#endif
22432258
// Wait until we're switched out of SUSPENDED to DETACHED.
22442259
_PyParkingLot_Park(&tstate->state, &state, sizeof(tstate->state),
22452260
/*timeout=*/-1, NULL, /*detach=*/0);
@@ -2253,6 +2268,11 @@ tstate_wait_attach(PyThreadState *tstate)
22532268
}
22542269
// Once we're back in DETACHED we can re-attach
22552270
} while (!tstate_try_attach(tstate));
2271+
#ifdef Py_GIL_DISABLED
2272+
if (stw_attach_waiting) {
2273+
_Py_atomic_store_int_relaxed(&tstate_impl->stw_attach_waiting, 0);
2274+
}
2275+
#endif
22562276
}
22572277

22582278
void
@@ -2422,9 +2442,16 @@ park_detached_threads(struct _stoptheworld_state *stw)
24222442
_Py_FOR_EACH_TSTATE_UNLOCKED(i, t) {
24232443
int state = _Py_atomic_load_int_relaxed(&t->state);
24242444
if (state == _Py_THREAD_DETACHED) {
2445+
_PyThreadStateImpl *tstate_impl = (_PyThreadStateImpl *)t;
2446+
if (_Py_atomic_load_int_relaxed(
2447+
&tstate_impl->stw_attach_waiting))
2448+
{
2449+
continue;
2450+
}
24252451
// Atomically transition to "suspended" if in "detached" state.
24262452
if (_Py_atomic_compare_exchange_int(
2427-
&t->state, &state, _Py_THREAD_SUSPENDED)) {
2453+
&t->state, &state,
2454+
_Py_THREAD_SUSPENDED_DETACHED)) {
24282455
num_parked++;
24292456
}
24302457
}
@@ -2509,8 +2536,11 @@ start_the_world(struct _stoptheworld_state *stw)
25092536
_Py_FOR_EACH_STW_INTERP(stw, i) {
25102537
_Py_FOR_EACH_TSTATE_UNLOCKED(i, t) {
25112538
if (t != stw->requester) {
2512-
assert(_Py_atomic_load_int_relaxed(&t->state) ==
2513-
_Py_THREAD_SUSPENDED);
2539+
#ifndef NDEBUG
2540+
int state = _Py_atomic_load_int_relaxed(&t->state);
2541+
assert(state == _Py_THREAD_SUSPENDED ||
2542+
state == _Py_THREAD_SUSPENDED_DETACHED);
2543+
#endif
25142544
_Py_atomic_store_int(&t->state, _Py_THREAD_DETACHED);
25152545
_PyParkingLot_UnparkAll(&t->state);
25162546
}

0 commit comments

Comments
 (0)