gh-153852: Assert thread list non-empty only after stopping the world (TSAN-0034)#154716
Closed
Vamsi-klu wants to merge 1 commit into
Closed
gh-153852: Assert thread list non-empty only after stopping the world (TSAN-0034)#154716Vamsi-klu wants to merge 1 commit into
Vamsi-klu wants to merge 1 commit into
Conversation
… world Move the threads.head assert in handle_thread_shutdown_exception to after _PyEval_StopTheWorld so it does not race tstate_delete_common (TSAN-0034).
Vamsi-klu
requested review from
FFY00,
ZeroIntensity and
ericsnowcurrently
as code owners
July 26, 2026 07:13
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Vamsi-klu
marked this pull request as draft
July 26, 2026 08:01
Author
|
Closing this PR. Thank you for the review attention. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix TSAN-0034 from the free-threading data-race umbrella gh-153852: a debug-only race on
interp->threads.headduring finalization.What the issue is
In
handle_thread_shutdown_exception(Python/pylifecycle.c):The comment below correctly says the world-stopped loop does not need locking — but the assert runs before STW. A racing exiting thread in
tstate_delete_commoncan mutate the list underHEAD_LOCK, and TSan reports a data race on the assert's load. Severity is low (debug assert only;NDEBUGstrips it), but it is a real race on a free-threaded--with-pydebugbuild and pollutes TSan signal.Why I solved it that way
_PyEval_StopTheWorld, do not delete it. The invariant ("finalizing thread is still registered") remains checked; the read becomes race-free by construction.HEAD_LOCKfor the assert — that would introduce a lock ordered before STW on the finalization path for no benefit and a plausible lock-ordering hazard.halt_on_error. The fix is a pure ordering change with no release-build codegen impact (assertvanishes underNDEBUG; STW is unconditional in both modes).How I did it
NEWS under Core and Builtins referencing gh-153852 (TSAN-0034).
Impact
Testing plan
./python -m test test_sys test_threadingsmoke — OK.--disable-gil --with-thread-sanitizer --with-pydebugrebuild + the fusil/TSan repro from the TSAN-0034 gist — not run in this environment (no FT+TSan build). Requesting FT SMEs to confirm on their TSan setup.Everything else
Requested reviewers (subject-matter experts)
Could the following SMEs take a look when convenient (I cannot formally request reviews from this fork account):
Thank you!