Skip to content

Commit 45afd9d

Browse files
colesburycolesbury
committed
gh-117657: Fix race involving immortalizing objects
The free-threaded build currently immortalizes objects that use deferred reference counting (see gh-117783). This typically happens once the first non-main thread is created, but the behavior can be suppressed for tests, in subinterpreters, or during a compile() call. This fixes a race condition involving the tracking of whether the behavior is suppressed.
1 parent 53b1981 commit 45afd9d

9 files changed

Lines changed: 30 additions & 43 deletions

File tree

‎Include/internal/pycore_gc.h‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -347,15 +347,11 @@ struct _gc_runtime_state {
347347

348348
/* gh-117783: Deferred reference counting is not fully implemented yet, so
349349
as a temporary measure we treat objects using deferred referenence
350-
counting as immortal. */
351-
struct {
352-
/* Immortalize objects instead of marking them as using deferred
353-
reference counting. */
354-
int enabled;
355-
356-
/* Set enabled=1 when the first background thread is created. */
357-
int enable_on_thread_created;
358-
} immortalize;
350+
counting as immortal. The value may be zero, one, or a negative number:
351+
0: immortalize deferred RC objects once the first thread is created
352+
1: immortalize all deferred RC objects immediately
353+
<0: suppressed; don't immortalize objects */
354+
int immortalize;
359355
#endif
360356
};
361357

‎Lib/test/support/__init__.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -529,11 +529,11 @@ def suppress_immortalization(suppress=True):
529529
yield
530530
return
531531

532-
old_values = _testinternalcapi.set_immortalize_deferred(False)
532+
_testinternalcapi.suppress_immortalization(True)
533533
try:
534534
yield
535535
finally:
536-
_testinternalcapi.set_immortalize_deferred(*old_values)
536+
_testinternalcapi.suppress_immortalization(False)
537537

538538
def skip_if_suppress_immortalization():
539539
try:

‎Modules/_testinternalcapi.c‎

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1966,32 +1966,26 @@ get_py_thread_id(PyObject *self, PyObject *Py_UNUSED(ignored))
19661966
#endif
19671967

19681968
static PyObject *
1969-
set_immortalize_deferred(PyObject *self, PyObject *value)
1969+
suppress_immortalization(PyObject *self, PyObject *value)
19701970
{
19711971
#ifdef Py_GIL_DISABLED
1972-
PyInterpreterState *interp = PyInterpreterState_Get();
1973-
int old_enabled = interp->gc.immortalize.enabled;
1974-
int old_enabled_on_thread = interp->gc.immortalize.enable_on_thread_created;
1975-
int enabled_on_thread = 0;
1976-
if (!PyArg_ParseTuple(value, "i|i",
1977-
&interp->gc.immortalize.enabled,
1978-
&enabled_on_thread))
1979-
{
1972+
int suppress = PyObject_IsTrue(value);
1973+
if (suppress < 0) {
19801974
return NULL;
19811975
}
1982-
interp->gc.immortalize.enable_on_thread_created = enabled_on_thread;
1983-
return Py_BuildValue("ii", old_enabled, old_enabled_on_thread);
1984-
#else
1985-
return Py_BuildValue("OO", Py_False, Py_False);
1976+
PyInterpreterState *interp = PyInterpreterState_Get();
1977+
// Subtract two to suppress immortalization (so that 1 -> -1)
1978+
_Py_atomic_add_int(&interp->gc.immortalize, suppress ? -2 : 2);
19861979
#endif
1980+
Py_RETURN_NONE;
19871981
}
19881982

19891983
static PyObject *
19901984
get_immortalize_deferred(PyObject *self, PyObject *Py_UNUSED(ignored))
19911985
{
19921986
#ifdef Py_GIL_DISABLED
19931987
PyInterpreterState *interp = PyInterpreterState_Get();
1994-
return PyBool_FromLong(interp->gc.immortalize.enable_on_thread_created);
1988+
return PyBool_FromLong(_Py_atomic_load_int(&interp->gc.immortalize) >= 0);
19951989
#else
19961990
Py_RETURN_FALSE;
19971991
#endif
@@ -2111,7 +2105,7 @@ static PyMethodDef module_functions[] = {
21112105
#ifdef Py_GIL_DISABLED
21122106
{"py_thread_id", get_py_thread_id, METH_NOARGS},
21132107
#endif
2114-
{"set_immortalize_deferred", set_immortalize_deferred, METH_VARARGS},
2108+
{"suppress_immortalization", suppress_immortalization, METH_O},
21152109
{"get_immortalize_deferred", get_immortalize_deferred, METH_NOARGS},
21162110
#ifdef _Py_TIER2
21172111
{"uop_symbols_test", _Py_uop_symbols_test, METH_NOARGS},

‎Objects/codeobject.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ should_intern_string(PyObject *o)
110110
// unless we've disabled immortalizing objects that use deferred reference
111111
// counting.
112112
PyInterpreterState *interp = _PyInterpreterState_GET();
113-
if (interp->gc.immortalize.enable_on_thread_created) {
113+
if (_Py_atomic_load_int(&interp->gc.immortalize) < 0) {
114114
return 1;
115115
}
116116
#endif
@@ -240,7 +240,7 @@ intern_constants(PyObject *tuple, int *modified)
240240
PyThreadState *tstate = PyThreadState_GET();
241241
if (!_Py_IsImmortal(v) && !PyCode_Check(v) &&
242242
!PyUnicode_CheckExact(v) &&
243-
tstate->interp->gc.immortalize.enable_on_thread_created)
243+
_Py_atomic_load_int(&tstate->interp->gc.immortalize) >= 0)
244244
{
245245
PyObject *interned = intern_one_constant(v);
246246
if (interned == NULL) {

‎Objects/object.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2429,7 +2429,7 @@ _PyObject_SetDeferredRefcount(PyObject *op)
24292429
assert(op->ob_ref_shared == 0);
24302430
_PyObject_SET_GC_BITS(op, _PyGC_BITS_DEFERRED);
24312431
PyInterpreterState *interp = _PyInterpreterState_GET();
2432-
if (interp->gc.immortalize.enabled) {
2432+
if (_Py_atomic_load_int_relaxed(&interp->gc.immortalize) == 1) {
24332433
// gh-117696: immortalize objects instead of using deferred reference
24342434
// counting for now.
24352435
_Py_SetImmortal(op);

‎Python/bltinmodule.c‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -870,15 +870,15 @@ builtin_compile_impl(PyObject *module, PyObject *source, PyObject *filename,
870870
// gh-118527: Disable immortalization of code constants for explicit
871871
// compile() calls to get consistent frozen outputs between the default
872872
// and free-threaded builds.
873+
// Subtract two to suppress immortalization (so that 1 -> -1)
873874
PyInterpreterState *interp = _PyInterpreterState_GET();
874-
int old_value = interp->gc.immortalize.enable_on_thread_created;
875-
interp->gc.immortalize.enable_on_thread_created = 0;
875+
_Py_atomic_add_int(&interp->gc.immortalize, -2);
876876
#endif
877877

878878
result = Py_CompileStringObject(str, filename, start[compile_mode], &cf, optimize);
879879

880880
#ifdef Py_GIL_DISABLED
881-
interp->gc.immortalize.enable_on_thread_created = old_value;
881+
_Py_atomic_add_int(&interp->gc.immortalize, 2);
882882
#endif
883883

884884
Py_XDECREF(source_copy);

‎Python/gc_free_threading.c‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -703,11 +703,9 @@ _PyGC_Init(PyInterpreterState *interp)
703703
{
704704
GCState *gcstate = &interp->gc;
705705

706-
if (_Py_IsMainInterpreter(interp)) {
707-
// gh-117783: immortalize objects that would use deferred refcounting
708-
// once the first non-main thread is created.
709-
gcstate->immortalize.enable_on_thread_created = 1;
710-
}
706+
// gh-117783: immortalize objects that would use deferred refcounting
707+
// once the first non-main thread is created (but not in subinterpreters).
708+
gcstate->immortalize = _Py_IsMainInterpreter(interp) ? 0 : -1;
711709

712710
gcstate->garbage = PyList_New(0);
713711
if (gcstate->garbage == NULL) {
@@ -1808,8 +1806,10 @@ _PyGC_ImmortalizeDeferredObjects(PyInterpreterState *interp)
18081806
{
18091807
struct visitor_args args;
18101808
_PyEval_StopTheWorld(interp);
1811-
gc_visit_heaps(interp, &immortalize_visitor, &args);
1812-
interp->gc.immortalize.enabled = 1;
1809+
if (interp->gc.immortalize == 0) {
1810+
gc_visit_heaps(interp, &immortalize_visitor, &args);
1811+
interp->gc.immortalize = 1;
1812+
}
18131813
_PyEval_StartTheWorld(interp);
18141814
}
18151815

‎Python/pystate.c‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1583,8 +1583,7 @@ new_threadstate(PyInterpreterState *interp, int whence)
15831583
}
15841584
else {
15851585
#ifdef Py_GIL_DISABLED
1586-
if (interp->gc.immortalize.enable_on_thread_created &&
1587-
!interp->gc.immortalize.enabled)
1586+
if (_Py_atomic_load_int(&interp->gc.immortalize) == 0)
15881587
{
15891588
// Immortalize objects marked as using deferred reference counting
15901589
// the first time a non-main thread is created.

‎Tools/tsan/suppressions_free_threading.txt‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,15 +56,13 @@ race_top:_PyImport_AcquireLock
5656
race_top:_Py_dict_lookup_threadsafe
5757
race_top:_imp_release_lock
5858
race_top:_multiprocessing_SemLock_acquire_impl
59-
race_top:builtin_compile_impl
6059
race_top:dictiter_new
6160
race_top:dictresize
6261
race_top:insert_to_emptydict
6362
race_top:insertdict
6463
race_top:list_get_item_ref
6564
race_top:make_pending_calls
6665
race_top:set_add_entry
67-
race_top:should_intern_string
6866
race_top:_PyEval_IsGILEnabled
6967
race_top:llist_insert_tail
7068
race_top:_Py_slot_tp_getattr_hook

0 commit comments

Comments
 (0)