Skip to content

Commit f4e012c

Browse files
colesburycolesbury
committed
pythongh-117657: Fix race involving immortalizing objects (python#119927)
The free-threaded build currently immortalizes objects that use deferred reference counting (see pythongh-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. (cherry picked from commit 47fb432)
1 parent ca37034 commit f4e012c

9 files changed

Lines changed: 30 additions & 44 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
@@ -1965,32 +1965,26 @@ get_py_thread_id(PyObject *self, PyObject *Py_UNUSED(ignored))
19651965
#endif
19661966

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

19881982
static PyObject *
19891983
get_immortalize_deferred(PyObject *self, PyObject *Py_UNUSED(ignored))
19901984
{
19911985
#ifdef Py_GIL_DISABLED
19921986
PyInterpreterState *interp = PyInterpreterState_Get();
1993-
return PyBool_FromLong(interp->gc.immortalize.enable_on_thread_created);
1987+
return PyBool_FromLong(_Py_atomic_load_int(&interp->gc.immortalize) >= 0);
19941988
#else
19951989
Py_RETURN_FALSE;
19961990
#endif
@@ -2110,7 +2104,7 @@ static PyMethodDef module_functions[] = {
21102104
#ifdef Py_GIL_DISABLED
21112105
{"py_thread_id", get_py_thread_id, METH_NOARGS},
21122106
#endif
2113-
{"set_immortalize_deferred", set_immortalize_deferred, METH_VARARGS},
2107+
{"suppress_immortalization", suppress_immortalization, METH_O},
21142108
{"get_immortalize_deferred", get_immortalize_deferred, METH_NOARGS},
21152109
#ifdef _Py_TIER2
21162110
{"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
@@ -2433,7 +2433,7 @@ _PyObject_SetDeferredRefcount(PyObject *op)
24332433
assert(op->ob_ref_shared == 0);
24342434
_PyObject_SET_GC_BITS(op, _PyGC_BITS_DEFERRED);
24352435
PyInterpreterState *interp = _PyInterpreterState_GET();
2436-
if (interp->gc.immortalize.enabled) {
2436+
if (_Py_atomic_load_int_relaxed(&interp->gc.immortalize) == 1) {
24372437
// gh-117696: immortalize objects instead of using deferred reference
24382438
// counting for now.
24392439
_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 & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1583,9 +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)
1588-
{
1586+
if (_Py_atomic_load_int(&interp->gc.immortalize) == 0) {
15891587
// Immortalize objects marked as using deferred reference counting
15901588
// the first time a non-main thread is created.
15911589
_PyGC_ImmortalizeDeferredObjects(interp);

‎Tools/tsan/suppressions_free_threading.txt‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ race_top:_PyImport_AcquireLock
4747
race_top:_Py_dict_lookup_threadsafe
4848
race_top:_imp_release_lock
4949
race_top:_multiprocessing_SemLock_acquire_impl
50-
race_top:builtin_compile_impl
5150
race_top:count_next
5251
race_top:dictiter_new
5352
race_top:dictresize
@@ -56,7 +55,6 @@ race_top:insertdict
5655
race_top:list_get_item_ref
5756
race_top:make_pending_calls
5857
race_top:set_add_entry
59-
race_top:should_intern_string
6058
race_top:_Py_slot_tp_getattr_hook
6159
race_top:add_threadstate
6260
race_top:dump_traceback

0 commit comments

Comments
 (0)