Repository navigation
gh-153014: Fix data race on the GC debug flag in free-threading builds - #153015
nascheme merged 10 commits into
Conversation
Sorry, something went wrong.
… builds In free-threading builds gc.set_debug() stored gcstate->debug with a plain write while gc.get_debug() and the collector read it without synchronisation, which ThreadSanitizer reports as a data race. Access the flag with FT_ATOMIC_STORE/LOAD_INT_RELAXED, consistent with how gc.enable()/disable() already handle gcstate->enabled.
|
This looks worth fixing. Some suggested improvements:
|
Sorry, something went wrong.
|
This PR is stale because it has been open for 90 days with no activity. |
Sorry, something went wrong.
- Read gcstate->debug once into a local in delete_garbage, handle_legacy_finalizers, gc_collect_main and _PyGC_DumpShutdownStats. - Simplify the regression test to one gc.set_debug() writer racing one gc.get_debug() reader via run_concurrently(), dropping the collector thread.
|
Done: gcstate->debug is now read once into a local in the four collector functions, and the test is one gc.set_debug() writer racing one gc.get_debug() reader via run_concurrently() (no collector thread). Verified under --disable-gil --with-thread-sanitizer: TSAN flags the race without the fix and is clean with it. Also merged main to clear the conflict. I have made the requested changes; please review again |
Sorry, something went wrong.
|
Thanks for making the requested changes! : please review the changes made to this pull request. |
Sorry, something went wrong.
|
Hi tonghuaroot, thanks for making the changes requested. This looks good to merge now. After reconsidering the unit test, I think it's better we just remove it (I pushed a change). In terms of CI cost vs what this test buys us, I think it's not worth it. This race is not very serious. Fixing the code so we don't get a TSAN warning is good. Sorry for asking you to fix the test on the first review, normally we do want unit tests when bugs are fixed. |
Sorry, something went wrong.
|
looks like this PR needs your approval @nascheme |
Sorry, something went wrong.
|
@nascheme you'll have to approve the PR for the bot to change the label, or change it manually. |
Sorry, something went wrong.
nascheme
left a comment
There was a problem hiding this comment.
LGTM!
Sorry, something went wrong.
In a free-threading build, gc.set_debug() stored gcstate->debug with a
plain write while gc.get_debug() and the collector read it without
synchronisation. gc.set_debug() takes no lock and, unlike
gc_set_threshold_impl(), does not run under _PyEval_StopTheWorld(), so
ThreadSanitizer reports a write/write race on gcstate->debug.
This accesses the flag with FT_ATOMIC_STORE_INT_RELAXED /
FT_ATOMIC_LOAD_INT_RELAXED in gc_set_debug_impl() / gc_get_debug_impl()
and in the collector reads in Python/gc_free_threading.c. Four of those
collector reads run outside stop-the-world (in delete_garbage(),
handle_legacy_finalizers(), and two stats reads in gc_collect_main()) and
race the write directly; the read in gc_collect_internal() runs under
stop-the-world and is made atomic only for consistency. Relaxed ordering is
correct for an independent int flag, matching how gcstate->enabled and
sys dlopenflags (gh-151644) are already handled.
Python/gc.c (the default GIL build) is left unchanged: it has no concurrent
access, and FT_ATOMIC_* compiles to a plain load/store there anyway.
Verified with a --with-thread-sanitizer --disable-gil build stressing
concurrent gc.set_debug() / gc.get_debug() plus a thread churning cyclic
garbage:
A regression test is added under Lib/test/test_free_threading/.