From 4f57f14954549c619fda0ebb553ffea7800fe807 Mon Sep 17 00:00:00 2001 From: edward_xu Date: Sun, 24 May 2026 22:06:53 +0800 Subject: [PATCH 1/4] fix the gc_set_threshold_impl race --- Modules/gcmodule.c | 12 ++++++------ Python/gc_free_threading.c | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Modules/gcmodule.c b/Modules/gcmodule.c index 12f93ac0fdea14b..f10c4a2025bac20 100644 --- a/Modules/gcmodule.c +++ b/Modules/gcmodule.c @@ -167,12 +167,12 @@ gc_set_threshold_impl(PyObject *module, int threshold0, int group_right_1, gcstate->generations[2].threshold = threshold2; } #else - gcstate->young.threshold = threshold0; + _Py_atomic_exchange_int(&gcstate->young.threshold, threshold0); if (group_right_1) { - gcstate->old[0].threshold = threshold1; + _Py_atomic_exchange_int(&gcstate->old[0].threshold, threshold1); } if (group_right_2) { - gcstate->old[1].threshold = threshold2; + _Py_atomic_exchange_int(&gcstate->old[1].threshold, threshold2); } #endif Py_RETURN_NONE; @@ -196,9 +196,9 @@ gc_get_threshold_impl(PyObject *module) gcstate->generations[2].threshold); #else return Py_BuildValue("(iii)", - gcstate->young.threshold, - gcstate->old[0].threshold, - gcstate->old[1].threshold); + _Py_atomic_load_int(&gcstate->young.threshold), + _Py_atomic_load_int(&gcstate->old[0].threshold), + _Py_atomic_load_int(&gcstate->old[1].threshold)); #endif } diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index 4e36189580bbf87..2574463e8e8a10d 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -1665,7 +1665,7 @@ void _PyGC_InitState(GCState *gcstate) { // TODO: move to pycore_runtime_init.h once the incremental GC lands. - gcstate->young.threshold = 2000; + _Py_atomic_exchange_int(&gcstate->young.threshold, 2000); } @@ -1996,12 +1996,12 @@ static bool gc_should_collect(GCState *gcstate) { int count = _Py_atomic_load_int_relaxed(&gcstate->young.count); - int threshold = gcstate->young.threshold; + int threshold = _Py_atomic_load_int_relaxed(&gcstate->young.threshold); int gc_enabled = _Py_atomic_load_int_relaxed(&gcstate->enabled); if (count <= threshold || threshold == 0 || !gc_enabled) { return false; } - if (gcstate->old[0].threshold == 0) { + if (_Py_atomic_load_int_relaxed(&gcstate->old[0].threshold) == 0) { // A few tests rely on immediate scheduling of the GC so we ignore the // extra conditions if generations[1].threshold is set to zero. return true; From 104b4e6f8b4f024d129ad79de73c7b76562c0272 Mon Sep 17 00:00:00 2001 From: edward_xu Date: Sun, 24 May 2026 22:40:48 +0800 Subject: [PATCH 2/4] add unit test --- Lib/test/test_free_threading/test_gc.py | 30 ++++++++++++++++++++++++- Modules/gcmodule.c | 6 ++--- Python/gc_free_threading.c | 2 +- 3 files changed, 33 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_free_threading/test_gc.py b/Lib/test/test_free_threading/test_gc.py index 8b45b6e2150c288..78509ba9034bc79 100644 --- a/Lib/test/test_free_threading/test_gc.py +++ b/Lib/test/test_free_threading/test_gc.py @@ -1,7 +1,7 @@ import unittest import threading -from threading import Thread +from threading import Barrier, Thread from unittest import TestCase import gc @@ -94,6 +94,34 @@ def evil(): thread.start() thread.join() + def test_set_threshold(self): + # GH-148613: Setting the GC threshold from another thread could causes + # race between the `gc_should_collect` and `gc_set_threshold` functions. + NUM_THREADS = 8 + NUM_ITERS = 100_000 + barrier = Barrier(NUM_THREADS) + + class CyclicReference: + def __init__(self): + self.r = self + + def allocator(): + barrier.wait() + for _ in range(NUM_ITERS): + CyclicReference() + + def setter(): + barrier.wait() + for i in range(NUM_ITERS): + gc.set_threshold(100 + (i % 100), 10 + (i % 10), 10 + (i % 10)) + + threads = [Thread(target=allocator) for _ in range(NUM_THREADS - 1)] + threads.append(Thread(target=setter)) + for t in threads: + t.start() + for t in threads: + t.join() + if __name__ == "__main__": unittest.main() diff --git a/Modules/gcmodule.c b/Modules/gcmodule.c index f10c4a2025bac20..87dba53d742aa0b 100644 --- a/Modules/gcmodule.c +++ b/Modules/gcmodule.c @@ -167,12 +167,12 @@ gc_set_threshold_impl(PyObject *module, int threshold0, int group_right_1, gcstate->generations[2].threshold = threshold2; } #else - _Py_atomic_exchange_int(&gcstate->young.threshold, threshold0); + _Py_atomic_store_int(&gcstate->young.threshold, threshold0); if (group_right_1) { - _Py_atomic_exchange_int(&gcstate->old[0].threshold, threshold1); + _Py_atomic_store_int(&gcstate->old[0].threshold, threshold1); } if (group_right_2) { - _Py_atomic_exchange_int(&gcstate->old[1].threshold, threshold2); + _Py_atomic_store_int(&gcstate->old[1].threshold, threshold2); } #endif Py_RETURN_NONE; diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index 2574463e8e8a10d..bd1c2833942832d 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -1665,7 +1665,7 @@ void _PyGC_InitState(GCState *gcstate) { // TODO: move to pycore_runtime_init.h once the incremental GC lands. - _Py_atomic_exchange_int(&gcstate->young.threshold, 2000); + gcstate->young.threshold = 2000; } From c00b96579bad775d1b35b033adb4fceacfe729b8 Mon Sep 17 00:00:00 2001 From: edward_xu Date: Sun, 24 May 2026 22:47:25 +0800 Subject: [PATCH 3/4] add blurb file --- Lib/test/test_free_threading/test_gc.py | 16 +++++++++------- ...026-05-24-22-46-49.gh-issue-148613.PLpmyd.rst | 2 ++ 2 files changed, 11 insertions(+), 7 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2026-05-24-22-46-49.gh-issue-148613.PLpmyd.rst diff --git a/Lib/test/test_free_threading/test_gc.py b/Lib/test/test_free_threading/test_gc.py index 78509ba9034bc79..62a389ccf52f084 100644 --- a/Lib/test/test_free_threading/test_gc.py +++ b/Lib/test/test_free_threading/test_gc.py @@ -95,7 +95,7 @@ def evil(): thread.join() def test_set_threshold(self): - # GH-148613: Setting the GC threshold from another thread could causes + # GH-148613: Setting the GC threshold from another thread could cause a # race between the `gc_should_collect` and `gc_set_threshold` functions. NUM_THREADS = 8 NUM_ITERS = 100_000 @@ -115,12 +115,14 @@ def setter(): for i in range(NUM_ITERS): gc.set_threshold(100 + (i % 100), 10 + (i % 10), 10 + (i % 10)) - threads = [Thread(target=allocator) for _ in range(NUM_THREADS - 1)] - threads.append(Thread(target=setter)) - for t in threads: - t.start() - for t in threads: - t.join() + old_threshold = gc.get_threshold() + try: + threads = [Thread(target=allocator) for _ in range(NUM_THREADS - 1)] + threads.append(Thread(target=setter)) + with threading_helper.start_threads(threads): + pass + finally: + gc.set_threshold(*old_threshold) if __name__ == "__main__": diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-05-24-22-46-49.gh-issue-148613.PLpmyd.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-05-24-22-46-49.gh-issue-148613.PLpmyd.rst new file mode 100644 index 000000000000000..71a701bf3eb3551 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-05-24-22-46-49.gh-issue-148613.PLpmyd.rst @@ -0,0 +1,2 @@ +Fix a data race in the free-threaded build between :func:`gc.set_threshold` +and garbage collection scheduling during object allocation. From 77f72d92a607163d7a2fc3f75b44d487489d9f31 Mon Sep 17 00:00:00 2001 From: edward_xu Date: Sun, 24 May 2026 23:38:27 +0800 Subject: [PATCH 4/4] use relaxed for load --- Lib/test/test_free_threading/test_gc.py | 4 ++-- Modules/gcmodule.c | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_free_threading/test_gc.py b/Lib/test/test_free_threading/test_gc.py index 62a389ccf52f084..a4a3b265a0181b7 100644 --- a/Lib/test/test_free_threading/test_gc.py +++ b/Lib/test/test_free_threading/test_gc.py @@ -115,14 +115,14 @@ def setter(): for i in range(NUM_ITERS): gc.set_threshold(100 + (i % 100), 10 + (i % 10), 10 + (i % 10)) - old_threshold = gc.get_threshold() + current_threshold = gc.get_threshold() try: threads = [Thread(target=allocator) for _ in range(NUM_THREADS - 1)] threads.append(Thread(target=setter)) with threading_helper.start_threads(threads): pass finally: - gc.set_threshold(*old_threshold) + gc.set_threshold(*current_threshold) if __name__ == "__main__": diff --git a/Modules/gcmodule.c b/Modules/gcmodule.c index 87dba53d742aa0b..3ca6a0a6a205a94 100644 --- a/Modules/gcmodule.c +++ b/Modules/gcmodule.c @@ -167,12 +167,12 @@ gc_set_threshold_impl(PyObject *module, int threshold0, int group_right_1, gcstate->generations[2].threshold = threshold2; } #else - _Py_atomic_store_int(&gcstate->young.threshold, threshold0); + _Py_atomic_store_int_relaxed(&gcstate->young.threshold, threshold0); if (group_right_1) { - _Py_atomic_store_int(&gcstate->old[0].threshold, threshold1); + _Py_atomic_store_int_relaxed(&gcstate->old[0].threshold, threshold1); } if (group_right_2) { - _Py_atomic_store_int(&gcstate->old[1].threshold, threshold2); + _Py_atomic_store_int_relaxed(&gcstate->old[1].threshold, threshold2); } #endif Py_RETURN_NONE; @@ -196,9 +196,9 @@ gc_get_threshold_impl(PyObject *module) gcstate->generations[2].threshold); #else return Py_BuildValue("(iii)", - _Py_atomic_load_int(&gcstate->young.threshold), - _Py_atomic_load_int(&gcstate->old[0].threshold), - _Py_atomic_load_int(&gcstate->old[1].threshold)); + _Py_atomic_load_int_relaxed(&gcstate->young.threshold), + _Py_atomic_load_int_relaxed(&gcstate->old[0].threshold), + _Py_atomic_load_int_relaxed(&gcstate->old[1].threshold)); #endif }