From 59a43f52ac7a0bb66f3f7b41b749e2d44e3f0e36 Mon Sep 17 00:00:00 2001 From: edward_xu Date: Sun, 24 May 2026 22:06:53 +0800 Subject: [PATCH 1/5] 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 12f93ac0fdea14..f10c4a2025bac2 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 4e36189580bbf8..2574463e8e8a10 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 c8cca6c43e818a45a521ecb914451410f9754830 Mon Sep 17 00:00:00 2001 From: edward_xu Date: Sun, 24 May 2026 22:40:48 +0800 Subject: [PATCH 2/5] 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 8b45b6e2150c28..78509ba9034bc7 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 f10c4a2025bac2..87dba53d742aa0 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 2574463e8e8a10..bd1c2833942832 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 dcb2d714b81ff1fbda0215e244748368e7ab003e Mon Sep 17 00:00:00 2001 From: edward_xu Date: Sun, 24 May 2026 22:47:25 +0800 Subject: [PATCH 3/5] 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 78509ba9034bc7..62a389ccf52f08 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 00000000000000..71a701bf3eb355 --- /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 8be444036935721463bf3444b3283938843ab7c6 Mon Sep 17 00:00:00 2001 From: edward_xu Date: Sun, 24 May 2026 23:38:27 +0800 Subject: [PATCH 4/5] 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 62a389ccf52f08..a4a3b265a0181b 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 87dba53d742aa0..3ca6a0a6a205a9 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 } From 8b997a94644b9f8f7feb10cd379886c5093d7b14 Mon Sep 17 00:00:00 2001 From: edward_xu Date: Wed, 27 May 2026 23:08:14 +0800 Subject: [PATCH 5/5] use mutex to guard threshold --- Include/internal/pycore_interp_structs.h | 1 + Lib/test/test_free_threading/test_gc.py | 4 ++-- Modules/gcmodule.c | 19 +++++++++++++------ Python/gc_free_threading.c | 17 +++++++++++++---- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/Include/internal/pycore_interp_structs.h b/Include/internal/pycore_interp_structs.h index f13bc2178b1e7e..aa35d7c1f14cdb 100644 --- a/Include/internal/pycore_interp_structs.h +++ b/Include/internal/pycore_interp_structs.h @@ -229,6 +229,7 @@ struct _gc_runtime_state { #ifndef Py_GIL_DISABLED struct gc_generation generations[NUM_GENERATIONS]; #else + PyMutex generations_mutex; // Mutex to guard threshold access to the generations below. struct gc_generation young; struct gc_generation old[2]; #endif diff --git a/Lib/test/test_free_threading/test_gc.py b/Lib/test/test_free_threading/test_gc.py index a4a3b265a0181b..cc1888dae48bc0 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 Barrier, Thread +from threading import Thread from unittest import TestCase import gc @@ -99,7 +99,7 @@ def test_set_threshold(self): # race between the `gc_should_collect` and `gc_set_threshold` functions. NUM_THREADS = 8 NUM_ITERS = 100_000 - barrier = Barrier(NUM_THREADS) + barrier = threading.Barrier(NUM_THREADS) class CyclicReference: def __init__(self): diff --git a/Modules/gcmodule.c b/Modules/gcmodule.c index 3ca6a0a6a205a9..dd82893951b9ec 100644 --- a/Modules/gcmodule.c +++ b/Modules/gcmodule.c @@ -167,13 +167,15 @@ gc_set_threshold_impl(PyObject *module, int threshold0, int group_right_1, gcstate->generations[2].threshold = threshold2; } #else - _Py_atomic_store_int_relaxed(&gcstate->young.threshold, threshold0); + PyMutex_Lock(&gcstate->generations_mutex); + gcstate->young.threshold = threshold0; if (group_right_1) { - _Py_atomic_store_int_relaxed(&gcstate->old[0].threshold, threshold1); + gcstate->old[0].threshold = threshold1; } if (group_right_2) { - _Py_atomic_store_int_relaxed(&gcstate->old[1].threshold, threshold2); + gcstate->old[1].threshold = threshold2; } + PyMutex_Unlock(&gcstate->generations_mutex); #endif Py_RETURN_NONE; } @@ -195,10 +197,15 @@ gc_get_threshold_impl(PyObject *module) gcstate->generations[1].threshold, gcstate->generations[2].threshold); #else + PyMutex_Lock(&gcstate->generations_mutex); + int young_threshold = gcstate->young.threshold; + int old_0_threshold = gcstate->old[0].threshold; + int old_1_threshold = gcstate->old[1].threshold; + PyMutex_Unlock(&gcstate->generations_mutex); return Py_BuildValue("(iii)", - _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)); + young_threshold, + old_0_threshold, + old_1_threshold); #endif } diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index bd1c2833942832..52a22b26781aed 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -1995,13 +1995,22 @@ cleanup_worklist(struct worklist *worklist) static bool gc_should_collect(GCState *gcstate) { - int count = _Py_atomic_load_int_relaxed(&gcstate->young.count); - 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) { + if (!gc_enabled) { + return false; + } + + int count = _Py_atomic_load_int_relaxed(&gcstate->young.count); + + PyMutex_Lock(&gcstate->generations_mutex); + int threshold = gcstate->young.threshold; + int old_0_threshold = gcstate->old[0].threshold; + PyMutex_Unlock(&gcstate->generations_mutex); + + if (count <= threshold || threshold == 0) { return false; } - if (_Py_atomic_load_int_relaxed(&gcstate->old[0].threshold) == 0) { + if (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;