Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a free-threaded stop-the-world (STW) fairness issue where repeated STW requests could repeatedly re-park a thread that is trying to reattach, leading to starvation (notably after returning from detached operations like time.sleep()).
Changes:
- Add a distinct thread-state for threads suspended while detached (
_Py_THREAD_SUSPENDED_DETACHED) and update STW parking/unparking logic accordingly. - Track “attach-waiters” across STW passes via a new cold
_PyThreadStateImplflag (stw_attach_waiting) reused from existing padding, so subsequent STW requests avoid re-parking those threads until they attach. - Add a regression test exercising a tight
gc.collect()loop vs. a sleeping thread, plus a NEWS entry and updated state documentation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Python/pystate.c | Implements the new suspended-detached state and attach-waiter logic to prevent STW starvation. |
| Include/internal/pycore_pystate.h | Documents the new suspended-detached state and updates state constants/diagram. |
| Include/internal/pycore_tstate.h | Adds stw_attach_waiting in reused tail padding for free-threaded builds. |
| Lib/test/test_free_threading/test_gc.py | Adds a subprocess regression test for STW starvation during tight GC loops. |
| Misc/NEWS.d/next/Core_and_Builtins/2026-06-16-19-20-00.gh-issue-151518.e6v0Js.rst | Announces the fix in NEWS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b1889c7 to
b3b504c
Compare
|
@kumaraditya303 can you add this to your PR review queue? |
dpdani
left a comment
dpdani
left a comment
There was a problem hiding this comment.
Thanks this looks much better now.
I would also like @colesbury or @mpage to have a look at this.
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.
Represent active attach waiters with suspended-waiting and detached-waiting states. Preserve waiter registration when the world resumes, and keep passive detached threads immediately parkable. Restore the thread-state padding and retain the single-CAS uncontended attach path.
Use explicit warmup imports and a joined non-daemon collector. Run the child through script_helper with a faulthandler watchdog so a stalled attachment still fails with a traceback. Describe the bug as a fairness issue in the NEWS entry.
Share the waiter-aware resume transition between stop-the-world pauses and biased reference count merging so a concurrently registering waiter keeps its opportunity to attach. Restore the support import needed by the new upstream GC regression and align the thread-state constants.
bc20719 to
c34abd4
Compare
|
Thanks for your changes. Unfortunately the test doesn't fail for me on main. The added C testing function should probably have some waiting time between starting the STW pause and ending it. |
Or even better: deterministic synchronization. |
Hold each test pause for 10 ms and repeat pauses in C to reduce the opportunities for a waiting thread to run between pause requests. Clarify that parking rechecks the thread state before sleeping.
|
Added 100 consecutive 10 ms pauses in C in 15c4b6d. With the same test changes, the unfixed PR base now times out with 4 or 64 CPUs; the fixed build passes with 1, 4, and 64 CPUs, and under TSan. On deterministic synchronization: an event before reattachment does not prove the thread has entered the attach wait. That would need a hook inside the attach path. This remains a stress test, and the base still passes on one CPU. |
@dpdani thanks, adding the wait helped. I originally hit this while stress-testing on B200 machines, and the earlier regression wasn't reliably catching it on my local Threadripper either. The helper now runs 100 consecutive STW pauses in C, holding each for 10 ms. With just the test changes applied to the unfixed base, it now times out locally with either 4 or 64 CPUs available; the fixed build passes. Could you try the latest version and see whether it reproduces for you? |
STW is a no-op in GIL builds. Avoid waiting in the helper there, since WASI cannot perform the blocking futex operation.
|
@colesbury or @mpage (or both): would you like time to review this as well? Looks like everything is green and all feedback has been addressed, with at least one core approval. |
Fixes #151518.
Repeated stop-the-world requests can starve a thread returning from a detached operation. After
start_the_world()restores its state toDETACHED, the next requester can suspend it again before its OS thread completes_PyThreadState_Attach().This change distinguishes thread states suspended while detached from those suspended while attached. An active attach waiter records that it encountered the detached-origin suspension; subsequent stop-the-world scans let that waiter attach before suspending it again. The waiter remains in the stop-the-world countdown until it actually stops. Threads detached for sleep or I/O without an active attach attempt remain immediately parkable, and the uncontended attach path remains a single CAS.
The waiting flag reuses trailing
_PyThreadStateImplpadding. Existing state numbers are preserved, and both suspended states resume directly toDETACHED.The subprocess regression explicitly uses
-X gil=0, waits for the collector to start, and repeatedly returns from sleep while another thread callsgc.collect()in a tight loop. It performs 50 reattachments with one second of total intentional sleep and relies on the test framework's outer timeout for stalled progress or shutdown.Validation on Linux x86-64 after rebasing onto
89c67a98aee:--with-pydebug --enable-safety --enable-slower-safety --disable-gil):test_free_threading test_gc test_threading test_capi test_embedpassed, 2,211 tests run.test_interpretersis unsupported in this build and skipped.test_free_threading test_gc test_threading test_embedpassed, 647 tests run.-X gil=1: GC, threading, embedding, and free-threaded GC tests passed, 403 tests run. The final regression also passed separately with this parent setting.test_gc test_threading test_capi test_embed test_interpreterspassed, 2,132 tests run.test_free_threadingskipped as expected.-R 3:3reference-leak checking. Seven additional stress trials passed with mixed global/local pauses, repeated detach/reattach, and workers blocked waiting for collector completion._PyThreadStateImplremains 18,200 bytes, and the new flag overlays padding at offset 18,136.make patchcheckandgit diff --checkpassed.