Repository navigation
gh-151518: Avoid STW starvation of attaching threads - #152826
tpn merged 8 commits into
Conversation
Sorry, something went wrong.
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 _PyThreadStateImpl flag (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.
Sorry, something went wrong.
|
@kumaraditya303 can you add this to your PR review queue? |
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
|
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. |
Sorry, something went wrong.
Or even better: deterministic synchronization. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
@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? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Confirmed 👍 Should this be backported to 3.14 and 3.15? |
Sorry, something went wrong.
colesbury
left a comment
There was a problem hiding this comment.
The implementation looks good.
Please consider @kumaraditya303's recent PR #157839. I don't think it needs any changes, but the behaviors overlap
I don't fully understand the justification for this new mechanism. In particular:
- How common is starvation caused by repeated manual gc.collect() calls?
- It’s not clear to me that adding this mechanism is better than fixing the gc.collect() calls in a tight loop.
- Avoiding starvation here only guarantees an opportunity to make a small amount of progress, until the next eval breaker check. I’m not sure how much practical benefit that provides. This seems qualitatively different from ordinary lock fairness mechanisms, which allow progress until the next contended lock acquisition.
That said, the implementation is small and easy to understand, so if you’re confident in the motivation, I’m comfortable with it.
Sorry, something went wrong.
Avoid a parking-lot lookup when resuming a suspended thread that has not tried to reattach. Use the old state from the successful CAS so concurrent waiter registration still receives a wakeup.
|
@colesbury I checked #157839: BRC and STW already share the waiter-aware resume helper after the rebase, and neither re-suspends DETACHED_WAITING. This came from intentional numba-cuda GC stress; I don't know how common it is in ordinary workloads. Throttling gc.collect() avoids the reproducer. This fix instead gives an active attach waiter a chance to run between successive pauses. You're right about the eval-breaker limit. In this case, that opportunity lets the waiting thread return from sleep and stop the collector, instead of timing out.
@dpdani Thanks for confirming. Yes, I think both should get it: 3.14 and 3.15 have the same attachment/resumption logic. The backports will need a small adjustment because those branches don't have the newer resume helper. |
Sorry, something went wrong.
Okay, that makes sense to me! |
Sorry, something went wrong.
Sorry, something went wrong.
|
GH-158186 is a backport of this pull request to the 3.15 branch. |
Sorry, something went wrong.
|
GH-158187 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
… (GH-158186) Preserve active attach waiters across successive stop-the-world pauses so a tight collection loop cannot keep them from making progress. Keep the waiter-aware resume operation local to pystate.c because this branch does not have the newer BRC suspend and resume helpers. (cherry picked from commit f156510)
Fixes #151518.
Repeated stop-the-world requests can starve a thread returning from a detached operation. After start_the_world() restores its state to DETACHED, 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 _PyThreadStateImpl padding. Existing state numbers are preserved, and both suspended states resume directly to DETACHED.
The subprocess regression explicitly uses -X gil=0, waits for the collector to start, and repeatedly returns from sleep while another thread calls gc.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: