diff --git a/Include/internal/pycore_interp_structs.h b/Include/internal/pycore_interp_structs.h index a9ad61f458944a..2236b158803bae 100644 --- a/Include/internal/pycore_interp_structs.h +++ b/Include/internal/pycore_interp_structs.h @@ -588,6 +588,12 @@ struct types_state { where all those lower numbers are used for core static types. */ unsigned int next_version_tag; +#ifdef Py_GIL_DISABLED + /* Incremented on changes that can invalidate a type update that is + prepared under the type lock and published with the world stopped. */ + uint64_t publish_gen; +#endif + /* Every static builtin type is initialized for each interpreter during its own initialization, including for the main interpreter during global runtime initialization. This is done by calling diff --git a/Lib/test/test_free_threading/test_threading.py b/Lib/test/test_free_threading/test_threading.py index ecc42db7b44f3a..670d66e1d0ebb2 100644 --- a/Lib/test/test_free_threading/test_threading.py +++ b/Lib/test/test_free_threading/test_threading.py @@ -26,6 +26,30 @@ def mutate_thread(): class TestThreadState(unittest.TestCase): + def test_stop_the_world(self): + import _testinternalcapi + + _testinternalcapi.stop_the_world() + _testinternalcapi.stop_the_world(0) + + def pause(): + for _ in range(10): + _testinternalcapi.stop_the_world(0.0001) + + threading_helper.run_concurrently(pause, nthreads=2) + for duration in (-1, -0.1, float("nan")): + with self.subTest(duration=duration): + with self.assertRaises(ValueError): + _testinternalcapi.stop_the_world(duration) + for duration in (float("inf"), 10**100): + with self.subTest(duration=duration): + with self.assertRaises(OverflowError): + _testinternalcapi.stop_the_world(duration) + for duration in (None, "0"): + with self.subTest(duration=duration): + with self.assertRaises(TypeError): + _testinternalcapi.stop_the_world(duration) + @support.requires_subprocess() def test_tight_stw_loop_does_not_starve_attach(self): script = textwrap.dedent(f""" diff --git a/Lib/test/test_free_threading/test_type.py b/Lib/test/test_free_threading/test_type.py index a55c4815a03837..a19638b083be5d 100644 --- a/Lib/test/test_free_threading/test_type.py +++ b/Lib/test/test_free_threading/test_type.py @@ -1,5 +1,7 @@ +import gc import threading import unittest +import weakref from concurrent.futures import ThreadPoolExecutor from threading import Barrier, Thread @@ -19,6 +21,24 @@ class A: attr = 1 +def world_stoppers(): + """Finite workloads that contend for the stop-the-world mutex. + + Waiting for the mutex releases the type lock, allowing queued type + updates to go stale. Use a short pause rather than full collections + to prolong the contention without depending on the size of the heap. + """ + def stopper(): + for _ in range(100): + _testinternalcapi.stop_the_world() + + def pauser(): + for _ in range(100): + _testinternalcapi.stop_the_world(0.0001) + + return [stopper, pauser] + + @threading_helper.requires_working_threading() class TestType(TestCase): def test_attr_cache(self): @@ -344,6 +364,12 @@ def custom_repr(self): self.assertTrue(all(repr(cls()) != "custom repr" for cls in subclasses)) + # The slot update queue must release its references to the types, + # including those stored in heap-allocated chunks. + del subclasses + gc.collect() + self.assertEqual(Base.__subclasses__(), []) + def test_concurrent_setattr_deadlock(self): # gh-155400: two threads assigning to a special method of the same # class could deadlock. One thread held the type lock and waited for @@ -401,6 +427,164 @@ class B(A): with threading_helper.start_threads(threads): pass + def check_stop_the_world_under_type_lock(self, setter): + # gh-158195: Python code that runs with the type lock held and stops + # the world could deadlock with a thread that held the type lock + # pinned while it waited for the stop-the-world mutex (__bases__ + # assignment). The first thread released the type lock while it was + # blocked on the stop-the-world mutex, was handed the mutex, and then + # waited for the type lock while owning the mutex. + def basesetter(): + class A: + pass + class B: + pass + class C(A): + pass + for _ in range(100): + C.__bases__ = (B,) + C.__bases__ = (A,) + + def stopper(): + for _ in range(100): + _testinternalcapi.stop_the_world() + + # The stopper keeps the stop-the-world mutex contended, which makes + # the setters block on it. All workers have finite workloads so + # that repeated invalidation cannot indefinitely delay a setter. + threading_helper.run_concurrently( + [setter, setter, basesetter, stopper]) + + def test_stop_the_world_in_mro_deadlock(self): + # type_ready() calls a metaclass mro() with the type lock held. + N = 50 + + class Meta(type): + def mro(cls): + _testinternalcapi.stop_the_world() + return type.mro(cls) + + def setter(): + for _ in range(N): + Meta("K", (), {}) + + self.check_stop_the_world_under_type_lock(setter) + + def test_stop_the_world_in_setattr_key_eq_deadlock(self): + # type_setattro() compares the attribute name with a str subclass key + # of the type's __dict__ (calling its __eq__) with the type lock held. + N = 50 + + class S(str): + __hash__ = str.__hash__ + def __eq__(self, other): + _testinternalcapi.stop_the_world() + return str.__eq__(self, other) + + C = type("C", (), {S("x"): 0}) + + def setter(): + for i in range(N): + C.x = i + + self.check_stop_the_world_under_type_lock(setter) + + def test_reentrant_slot_update_consistency(self): + # Updating Base queues its slot update before visiting Sub. A key + # comparison in Sub's dict re-enters the setter and makes that queue + # stale. It must be rebuilt rather than overwriting the inner update. + armed = False + + class Base: + pass + + class Key(str): + def __hash__(self): + return hash("__hash__") + + def __eq__(self, other): + nonlocal armed + if armed and other == "__hash__": + armed = False + Base.__hash__ = object.__hash__ + return str.__eq__(self, other) + + # Collide with the special-method name without defining it on Sub, + # so that slot updates still propagate from Base to Sub. + Sub = type("Sub", (Base,), {Key("trigger"): None}) + objects = [Base(), Sub()] + + # Check actual slot dispatch, not just the class dictionaries. None + # and object.__hash__ use distinct C slots, unlike Python functions + # which share a generic wrapper that looks up the current method. + Base.__hash__ = None + for obj in objects: + with self.assertRaises(TypeError): + hash(obj) + Base.__hash__ = object.__hash__ + for obj in objects: + self.assertEqual(hash(obj), object.__hash__(obj)) + + armed = True + Base.__hash__ = None + self.assertFalse(armed) + self.assertIs(Base.__dict__["__hash__"], object.__hash__) + for obj in objects: + self.assertEqual(hash(obj), object.__hash__(obj)) + + # Both applied and discarded queues must release their references. + base_ref = weakref.ref(Base) + sub_ref = weakref.ref(Sub) + del obj, objects, Sub, Base + gc.collect() + self.assertIsNone(base_ref()) + self.assertIsNone(sub_ref()) + + def test_concurrent_bases_assignment(self): + # Several threads reassign __bases__ of the same class, and of its + # base, while others stop the world. The validated bases and the + # computed MRO can go stale while the type lock is released and + # must be recomputed. At the end, the MRO must agree with __bases__. + N = 50 + + class A: + def meth(self): + return "A" + def __repr__(self): + return "A" + class B: + def meth(self): + return "B" + def __repr__(self): + return "B" + class C(A): + pass + class D(C): + pass + + def setter(): + for _ in range(N): + C.__bases__ = (B,) + C.__bases__ = (A,) + + def subsetter(): + for _ in range(N): + D.__bases__ = (C, B) + D.__bases__ = (C,) + + threading_helper.run_concurrently( + [setter, setter, subsetter] + world_stoppers()) + + self.assertEqual(C.__bases__, (A,)) + self.assertEqual(C.__mro__, (C, A, object)) + self.assertEqual(D.__bases__, (C,)) + self.assertEqual(D.__mro__, (D, C, A, object)) + self.assertEqual(D().meth(), "A") + self.assertEqual(repr(C()), "A") + self.assertEqual(repr(D()), "A") + self.assertIn(C, A.__subclasses__()) + self.assertNotIn(C, B.__subclasses__()) + if __name__ == "__main__": unittest.main() diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-10-20-10-00-00.gh-issue-158195.UnpinTL.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-10-20-10-00-00.gh-issue-158195.UnpinTL.rst new file mode 100644 index 00000000000000..f189c7c563ca58 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-10-20-10-00-00.gh-issue-158195.UnpinTL.rst @@ -0,0 +1,7 @@ +Fix a deadlock in the free-threaded build between a thread that stops the +world while running Python code under the type lock (for example a custom +metaclass ``mro()`` or the ``__eq__`` of a :class:`str` subclass used as a +class ``__dict__`` key) and a thread that assigns to a special method or to +``__bases__`` of a class. The type lock is no longer held while waiting +for a stop-the-world; the prepared slot, MRO and bases updates are +validated with the world stopped and recomputed if they went stale. diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c index 479297a996c250..cac040efdd2b92 100644 --- a/Modules/_testinternalcapi.c +++ b/Modules/_testinternalcapi.c @@ -38,6 +38,7 @@ #include "pycore_pylifecycle.h" // _PyInterpreterConfig_InitFromDict() #include "pycore_pystate.h" // _PyThreadState_GET() #include "pycore_runtime_structs.h" // _PY_NSMALLPOSINTS +#include "pycore_time.h" // _PyTime_FromSecondsObject() #include "pycore_unicodeobject.h" // _PyUnicode_TransformDecimalAndSpaceToASCII() #include "clinic/_testinternalcapi.c.h" @@ -209,6 +210,42 @@ get_stack_margin(PyObject *self, PyObject *Py_UNUSED(args)) return PyLong_FromSize_t(_PyOS_STACK_MARGIN_BYTES); } +static PyObject * +stop_the_world(PyObject *self, PyObject *args) +{ + PyObject *duration_obj = NULL; + PyTime_t duration = 0; + if (!PyArg_ParseTuple(args, "|O:stop_the_world", &duration_obj)) { + return NULL; + } + if (duration_obj != NULL && + _PyTime_FromSecondsObject(&duration, duration_obj, + _PyTime_ROUND_TIMEOUT) < 0) { + return NULL; + } + if (duration < 0) { + PyErr_SetString(PyExc_ValueError, "duration must be non-negative"); + return NULL; + } +#ifdef Py_GIL_DISABLED + PyInterpreterState *interp = _PyInterpreterState_GET(); + _PyEval_StopTheWorld(interp); + if (duration != 0) { + PyEvent event = {0}; + PyEvent_WaitTimed(&event, duration, /*detach=*/0); + } + _PyEval_StartTheWorld(interp); +#endif + Py_RETURN_NONE; +} + +PyDoc_STRVAR(stop_the_world_doc, +"stop_the_world($module, duration=0, /)\n" +"--\n" +"\n" +"Stop the world, wait for duration seconds, then restart it.\n" +"Does nothing in GIL-enabled builds."); + static PyObject * test_stop_the_world(PyObject *self, PyObject *Py_UNUSED(args)) { @@ -3345,6 +3382,7 @@ static PyMethodDef module_functions[] = { {"get_c_recursion_remaining", get_c_recursion_remaining, METH_NOARGS}, {"get_stack_pointer", get_stack_pointer, METH_NOARGS}, {"get_stack_margin", get_stack_margin, METH_NOARGS}, + {"stop_the_world", stop_the_world, METH_VARARGS, stop_the_world_doc}, {"test_stop_the_world", test_stop_the_world, METH_NOARGS}, {"classify_stack_addresses", classify_stack_addresses, METH_VARARGS}, {"get_jit_code_ranges", get_jit_code_ranges, METH_NOARGS}, diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 5e772e5608d4c6..b4abcd09405cac 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -46,13 +46,6 @@ class object "PyObject *" "&PyBaseObject_Type" #define NEXT_VERSION_TAG(interp) \ (interp)->types.next_version_tag -// Storage for the mutexes saved by type_lock_prevent_release(). Defined for -// both builds so the call sites don't need to be conditionally compiled. -typedef struct { - PyMutex *mutex1; - PyMutex *mutex2; -} pinned_mutexes_t; - #ifdef Py_GIL_DISABLED // There's a global lock for types that ensures that tp_version_tag and @@ -131,55 +124,36 @@ types_start_world(void) assert(!types_world_is_stopped()); } -// Temporarily prevent the mutexes held by the topmost critical section from -// being released when the current thread blocks (blocking detaches the thread, -// which suspends its critical sections and releases the mutexes they hold). -// -// All of the mutexes held by the critical section are pinned, not just -// TYPE_LOCK. If only TYPE_LOCK was pinned then _PyCriticalSection_Resume() -// would have to re-acquire the other mutex while TYPE_LOCK is held. That -// deadlocks against a thread that holds that mutex and is waiting for -// TYPE_LOCK, which is exactly what BEGIN_TYPE_DICT_LOCK() does: the type dict -// mutex is on the heap and TYPE_LOCK is in _PyRuntime, so the address ordering -// used by two-mutex critical sections usually acquires the dict mutex first. -// By pinning both mutexes there is nothing to re-acquire on resume. -// -// Holding the mutexes while blocked does not prevent the world from being -// stopped: a thread waiting on either of them parks with _PY_LOCK_DETACH and -// so is detached while it waits. -static void -type_lock_prevent_release(pinned_mutexes_t *pinned) +// Publish generation counter. Type updates that are prepared under TYPE_LOCK +// and published with the world stopped snapshot it before preparing and +// compare it inside the stop-the-world. It is incremented by every change +// that could invalidate such a prepared update, so a mismatch means the +// update must be prepared again. +static inline uint64_t +types_publish_gen(void) { - PyThreadState *tstate = _PyThreadState_GET(); - uintptr_t tag = tstate->critical_section; - PyCriticalSection *c = (PyCriticalSection *)(tag & ~_Py_CRITICAL_SECTION_MASK); - pinned->mutex1 = c->_cs_mutex; - pinned->mutex2 = NULL; - c->_cs_mutex = NULL; - if ((tag & _Py_CRITICAL_SECTION_TWO_MUTEXES) != 0) { - PyCriticalSection2 *c2 = (PyCriticalSection2 *)c; - pinned->mutex2 = c2->_cs_mutex2; - c2->_cs_mutex2 = NULL; - } - assert(pinned->mutex1 == TYPE_LOCK || pinned->mutex2 == TYPE_LOCK); + PyInterpreterState *interp = _PyInterpreterState_GET(); + return _Py_atomic_load_uint64_relaxed(&interp->types.publish_gen); } -static void -type_lock_allow_release(pinned_mutexes_t *pinned) +static inline void +types_publish_gen_inc(void) { - PyThreadState *tstate = _PyThreadState_GET(); - uintptr_t tag = tstate->critical_section; - PyCriticalSection *c = (PyCriticalSection *)(tag & ~_Py_CRITICAL_SECTION_MASK); - assert(c->_cs_mutex == NULL); - c->_cs_mutex = pinned->mutex1; - if ((tag & _Py_CRITICAL_SECTION_TWO_MUTEXES) != 0) { - PyCriticalSection2 *c2 = (PyCriticalSection2 *)c; - assert(c2->_cs_mutex2 == NULL); - c2->_cs_mutex2 = pinned->mutex2; - } - else { - assert(pinned->mutex2 == NULL); - } + PyInterpreterState *interp = _PyInterpreterState_GET(); + _Py_atomic_add_uint64(&interp->types.publish_gen, 1); +} + +// Passed as 'gen' to the try_set_tp_*() helpers to store without checking +// the publish generation. +#define TYPES_PUBLISH_GEN_NONE UINT64_MAX + +// Return true if the prepared state that was snapshotted at 'gen' is stale. +// Must be called with the world stopped. +static inline bool +types_publish_is_stale(uint64_t gen) +{ + assert(types_world_is_stopped()); + return gen != TYPES_PUBLISH_GEN_NONE && types_publish_gen() != gen; } #else @@ -195,8 +169,10 @@ type_lock_allow_release(pinned_mutexes_t *pinned) #define types_world_is_stopped() 1 #define types_stop_world() #define types_start_world() -#define type_lock_prevent_release(pinned) ((void)(pinned)) -#define type_lock_allow_release(pinned) ((void)(pinned)) +#define TYPES_PUBLISH_GEN_NONE 0 +#define types_publish_gen() 0 +#define types_publish_gen_inc() +#define types_publish_is_stale(gen) 0 #endif @@ -599,7 +575,7 @@ _PyType_GetBases(PyTypeObject *self) } static inline void -set_tp_bases(PyTypeObject *self, PyObject *bases, int initial) +set_tp_bases(PyTypeObject *self, PyObject *bases) { assert(PyTuple_Check(bases)); ASSERT_NEW_TYPE_OR_LOCKED(self); @@ -607,7 +583,6 @@ set_tp_bases(PyTypeObject *self, PyObject *bases, int initial) // XXX tp_bases can probably be statically allocated for each // static builtin type. assert(PyTuple_CheckExact(bases)); - assert(initial); assert(self->tp_bases == NULL); if (PyTuple_GET_SIZE(bases) == 0) { assert(self->tp_base == NULL); @@ -623,6 +598,37 @@ set_tp_bases(PyTypeObject *self, PyObject *bases, int initial) self->tp_bases = bases; } +// Replace tp_bases and tp_base of a type with the world stopped. Returns 1 +// on success, 0 if tp_bases is no longer 'expected' (NULL matches anything) +// and -1 if the publish generation is no longer 'gen' (pass +// TYPES_PUBLISH_GEN_NONE to skip that check). In the last two cases the +// caller still owns 'bases' and 'base'. +static int +try_set_tp_bases(PyTypeObject *self, PyObject *expected, + PyObject *bases, PyTypeObject *base, uint64_t gen) +{ + ASSERT_TYPE_LOCK_HELD(); + assert(PyTuple_Check(bases)); + assert(!(self->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN)); + int res; + types_stop_world(); + if (expected != NULL && self->tp_bases != expected) { + res = 0; + } + else if (types_publish_is_stale(gen)) { + res = -1; + } + else { + self->tp_bases = bases; + self->tp_base = base; + types_publish_gen_inc(); + res = 1; + } + types_start_world(); + ASSERT_TYPE_LOCK_HELD(); + return res; +} + static inline void clear_tp_bases(PyTypeObject *self, int final) { @@ -651,14 +657,13 @@ lookup_tp_mro(PyTypeObject *self) } static inline void -set_tp_mro(PyTypeObject *self, PyObject *mro, int initial) +set_tp_mro(PyTypeObject *self, PyObject *mro) { if (mro != NULL) { assert(PyTuple_CheckExact(mro)); if (self->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN) { // XXX tp_mro can probably be statically allocated for each // static builtin type. - assert(initial); assert(self->tp_mro == NULL); /* Other checks are done via set_tp_bases. */ _Py_SetImmortal(mro); @@ -667,16 +672,39 @@ set_tp_mro(PyTypeObject *self, PyObject *mro, int initial) PyUnstable_Object_EnableDeferredRefcount(mro); } } - pinned_mutexes_t pinned; - if (!initial) { - type_lock_prevent_release(&pinned); - types_stop_world(); - } self->tp_mro = mro; - if (!initial) { - types_start_world(); - type_lock_allow_release(&pinned); +} + +// Replace tp_mro of a type with the world stopped. Returns 1 on success, 0 +// if tp_mro is no longer 'expected' and -1 if the publish generation is no +// longer 'gen' (pass TYPES_PUBLISH_GEN_NONE to skip that check). In the +// last two cases the caller still owns 'mro'. +static int +try_set_tp_mro(PyTypeObject *self, PyObject *expected, PyObject *mro, + uint64_t gen) +{ + ASSERT_TYPE_LOCK_HELD(); + assert(!(self->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN)); + assert(mro == NULL || PyTuple_CheckExact(mro)); + int res; + types_stop_world(); + if (self->tp_mro != expected) { + res = 0; + } + else if (types_publish_is_stale(gen)) { + res = -1; } + else { + if (mro != NULL) { + PyUnstable_Object_EnableDeferredRefcount(mro); + } + self->tp_mro = mro; + types_publish_gen_inc(); + res = 1; + } + types_start_world(); + ASSERT_TYPE_LOCK_HELD(); + return res; } static inline void @@ -1706,18 +1734,19 @@ type_set_abstractmethods(PyObject *tp, PyObject *value, void *Py_UNUSED(closure) return -1; } - pinned_mutexes_t pinned; BEGIN_TYPE_LOCK(); - _PyType_Modified_Unlocked(type); - type_lock_prevent_release(&pinned); types_stop_world(); if (abstract) type_add_flags(type, Py_TPFLAGS_IS_ABSTRACT); else type_clear_flags(type, Py_TPFLAGS_IS_ABSTRACT); types_start_world(); - type_lock_allow_release(&pinned); + // Invalidate the version tag after the flag update, while TYPE_LOCK is + // still held: no other thread can assign a new tag before we release it. + // The lock may have been released while waiting for the stop-the-world, + // so invalidating before the update would not be enough. ASSERT_TYPE_LOCK_HELD(); + _PyType_Modified_Unlocked(type); END_TYPE_LOCK(); return 0; @@ -1794,8 +1823,13 @@ mro_hierarchy_for_complete_type(PyTypeObject *type, PyObject *temp) Py_XDECREF(tuple); if (res < 0) { - set_tp_mro(type, old_mro, 0); - Py_DECREF(new_mro); + // Roll back, unless tp_mro has already been replaced again. + if (try_set_tp_mro(type, new_mro, old_mro, TYPES_PUBLISH_GEN_NONE) == 1) { + Py_DECREF(new_mro); + } + else { + Py_XDECREF(old_mro); + } return -1; } Py_XDECREF(old_mro); @@ -1831,8 +1865,10 @@ mro_hierarchy_for_complete_type(PyTypeObject *type, PyObject *temp) return res; } +// Checks on the new __bases__ value that do not depend on mutable type +// state. Done once per assignment. static int -type_check_new_bases(PyTypeObject *type, PyObject *new_bases, PyTypeObject **best_base) +type_check_new_bases(PyTypeObject *type, PyObject *new_bases) { // Check arguments, this is re-entrant due to the PySys_Audit() call if (!check_set_special_type_attr(type, new_bases, "__bases__")) { @@ -1861,7 +1897,19 @@ type_check_new_bases(PyTypeObject *type, PyObject *new_bases, PyTypeObject **bes type->tp_name, Py_TYPE(ob)->tp_name); return -1; } - PyTypeObject *base = (PyTypeObject*)ob; + } + return 0; +} + +// Checks on the new __bases__ value that depend on the current state of the +// type hierarchy (inheritance cycles, layout compatibility). +static int +type_validate_new_bases(PyTypeObject *type, PyObject *new_bases, PyTypeObject **best_base) +{ + ASSERT_TYPE_LOCK_HELD(); + Py_ssize_t n = PyTuple_GET_SIZE(new_bases); + for (Py_ssize_t i = 0; i < n; i++) { + PyTypeObject *base = (PyTypeObject *)PyTuple_GET_ITEM(new_bases, i); if (is_subtype_with_mro(lookup_tp_mro(base), base, type) || /* In case of reentering here again through a custom mro() @@ -1894,8 +1942,11 @@ type_check_new_bases(PyTypeObject *type, PyObject *new_bases, PyTypeObject **bes return 0; } +// Returns 0 on success, -1 on error and 1 if the validated bases went stale +// before they could be stored (the caller must validate again). static int -type_set_bases_unlocked(PyTypeObject *type, PyObject *new_bases, PyTypeObject *best_base) +type_set_bases_unlocked(PyTypeObject *type, PyObject *new_bases, + PyTypeObject *best_base, uint64_t gen) { ASSERT_TYPE_LOCK_HELD(); @@ -1903,14 +1954,14 @@ type_set_bases_unlocked(PyTypeObject *type, PyObject *new_bases, PyTypeObject *b PyObject *old_bases = lookup_tp_bases(type); assert(old_bases != NULL); PyTypeObject *old_base = type->tp_base; - pinned_mutexes_t pinned; - type_lock_prevent_release(&pinned); - types_stop_world(); - set_tp_bases(type, Py_NewRef(new_bases), 0); - type->tp_base = (PyTypeObject *)Py_NewRef(best_base); - types_start_world(); - type_lock_allow_release(&pinned); + if (try_set_tp_bases(type, NULL, Py_NewRef(new_bases), + (PyTypeObject *)Py_NewRef(best_base), gen) != 1) + { + Py_DECREF(new_bases); + Py_DECREF(best_base); + return 1; + } PyObject *temp = PyList_New(0); if (temp == NULL) { @@ -1934,11 +1985,11 @@ type_set_bases_unlocked(PyTypeObject *type, PyObject *new_bases, PyTypeObject *b add to all new_bases */ remove_all_subclasses(type, old_bases); res = add_all_subclasses(type, new_bases); + /* This also clears the VALID_VERSION flag of 'type' and all its + subclasses. */ if (update_all_slots(type) < 0) { goto bail; } - /* Clear the VALID_VERSION flag of 'type' and all its subclasses. */ - _PyType_Modified_Unlocked(type); } else { res = 0; @@ -1960,24 +2011,21 @@ type_set_bases_unlocked(PyTypeObject *type, PyObject *new_bases, PyTypeObject *b PyArg_UnpackTuple(PyList_GET_ITEM(temp, i), "", 2, 3, &cls, &new_mro, &old_mro); /* Do not rollback if cls has a newer version of MRO. */ - if (lookup_tp_mro(cls) == new_mro) { - set_tp_mro(cls, Py_XNewRef(old_mro), 0); + old_mro = Py_XNewRef(old_mro); + if (try_set_tp_mro(cls, new_mro, old_mro, TYPES_PUBLISH_GEN_NONE) == 1) { Py_DECREF(new_mro); } + else { + Py_XDECREF(old_mro); + } } Py_DECREF(temp); bail: - if (lookup_tp_bases(type) == new_bases) { - assert(type->tp_base == best_base); - - type_lock_prevent_release(&pinned); - types_stop_world(); - set_tp_bases(type, old_bases, 0); - type->tp_base = old_base; - types_start_world(); - type_lock_allow_release(&pinned); - + /* Restore the old bases, unless tp_bases has been replaced again. */ + if (try_set_tp_bases(type, new_bases, old_bases, old_base, + TYPES_PUBLISH_GEN_NONE) == 1) + { Py_DECREF(new_bases); Py_DECREF(best_base); } @@ -1996,12 +2044,30 @@ type_set_bases(PyObject *tp, PyObject *new_bases, void *Py_UNUSED(closure)) PyTypeObject *type = PyTypeObject_CAST(tp); PyTypeObject *best_base; int res; - BEGIN_TYPE_LOCK(); - res = type_check_new_bases(type, new_bases, &best_base); - if (res == 0) { - res = type_set_bases_unlocked(type, new_bases, best_base); + if (type_check_new_bases(type, new_bases) < 0) { + return -1; + } + // Create a fresh bases tuple to store. Its identity tells whether + // tp_bases was replaced again while TYPE_LOCK was released or due to + // re-entrancy from the mro() call. + Py_ssize_t n = PyTuple_GET_SIZE(new_bases); + PyObject *bases = PyTuple_New(n); + if (bases == NULL) { + return -1; + } + for (Py_ssize_t i = 0; i < n; i++) { + PyTuple_SET_ITEM(bases, i, Py_NewRef(PyTuple_GET_ITEM(new_bases, i))); } + BEGIN_TYPE_LOCK(); + do { + uint64_t gen = types_publish_gen(); + res = type_validate_new_bases(type, bases, &best_base); + if (res == 0) { + res = type_set_bases_unlocked(type, bases, best_base, gen); + } + } while (res == 1); END_TYPE_LOCK(); + Py_DECREF(bases); return res; } @@ -3632,25 +3698,52 @@ mro_internal(PyTypeObject *type, int initial, PyObject **p_old_mro) ASSERT_TYPE_LOCK_HELD(); PyObject *new_mro, *old_mro; - int reent; /* Keep a reference to be able to do a reentrancy check below. Don't let old_mro be GC'ed and its address be reused for another object, like (suddenly!) a new tp_mro. */ old_mro = Py_XNewRef(lookup_tp_mro(type)); - new_mro = mro_invoke(type); /* might cause reentrance */ - reent = (lookup_tp_mro(type) != old_mro); - Py_XDECREF(old_mro); - if (new_mro == NULL) { - return -1; - } - - if (reent) { + for (;;) { + uint64_t gen = types_publish_gen(); + new_mro = mro_invoke(type); /* might cause reentrance */ + if (new_mro == NULL) { + Py_XDECREF(old_mro); + return -1; + } + int res; + if (initial) { + // The type has not been revealed to other threads yet, so only + // same-thread reentrance can have replaced tp_mro. + if (lookup_tp_mro(type) == old_mro) { + set_tp_mro(type, new_mro); + res = 1; + } + else { + res = 0; + } + } + else { + // Store with the world stopped. TYPE_LOCK may have been + // released while mro_invoke() ran or while waiting to stop the + // world, so check for reentrance and for concurrent mutation of + // the hierarchy inside the stop-the-world. + res = try_set_tp_mro(type, old_mro, new_mro, gen); + } + if (res == 1) { + break; + } Py_DECREF(new_mro); - return 0; + if (res == 0) { + /* reentrance: tp_mro was replaced by someone else */ + Py_XDECREF(old_mro); + return 0; + } + /* stale: another thread changed the hierarchy, compute again */ + assert(res == -1); } - - set_tp_mro(type, new_mro, initial); + /* Drop our temporary reference. The reference previously owned by + the type is now owned by this function and transferred below. */ + Py_XDECREF(old_mro); type_mro_modified(type, new_mro); /* corner case: the super class might have been hidden @@ -3780,6 +3873,11 @@ solid_base(PyTypeObject *type) // are finished the lookups, we stop-the-world and apply all of the updates. // The apply_slot_updates() code is simple and easy to confirm that it is // safe. +// +// TYPE_LOCK is a critical section and so it can be released while the lookups +// run (they can call Python code and block on other locks) and while waiting +// to stop the world. The queue is therefore validated with the publish +// generation before it is applied and rebuilt if it went stale. typedef struct { PyTypeObject *type; @@ -3821,14 +3919,25 @@ slot_update_new_chunk(void) } static void -slot_update_free_chunks(slot_update_t *updates) +slot_update_clear(slot_update_t *updates, slot_update_chunk_t *keep) { + assert(!types_world_is_stopped()); slot_update_chunk_t *chunk = updates->head; while (chunk != NULL) { slot_update_chunk_t *prev = chunk->prev; - PyMem_Free(chunk); + for (Py_ssize_t i = 0; i < chunk->n; i++) { + Py_DECREF(chunk->updates[i].type); + } + if (chunk == keep) { + chunk->n = 0; + chunk->prev = NULL; + } + else { + PyMem_Free(chunk); + } chunk = prev; } + updates->head = keep; } static int @@ -3847,7 +3956,7 @@ queue_slot_update(slot_update_t *updates, PyTypeObject *type, updates->head = chunk; } slot_update_item_t *item = &updates->head->updates[updates->head->n]; - item->type = type; + item->type = (PyTypeObject *)Py_NewRef(type); item->slot_ptr = slot_ptr; item->slot_value = slot_value; updates->head->n++; @@ -3855,6 +3964,12 @@ queue_slot_update(slot_update_t *updates, PyTypeObject *type, return 0; } +static inline int +slot_update_is_empty(slot_update_t *updates) +{ + return updates->head == NULL || updates->head->n == 0; +} + static void apply_slot_updates(slot_update_t *updates) { @@ -3873,38 +3988,36 @@ apply_slot_updates(slot_update_t *updates) } } -static void -apply_type_slot_updates(slot_update_t *updates) -{ - // This must be done carefully to avoid data races and deadlocks. We - // have just updated the type __dict__, while holding TYPE_LOCK. We have - // collected all of the required type slot updates into the 'updates' - // queue. Note that those updates can apply to multiple types since - // subclasses might also be affected by the dict change. - // - // We need to prevent other threads from writing to the dict before we can - // finish updating the slots. The actual stores to the slots are done - // with the world stopped. If we block on the stop-the-world mutex then - // we could release TYPE_LOCK mutex and potentially allow other threads - // to update the dict. That's because TYPE_LOCK was acquired using a - // critical section. - // - // The type_lock_prevent_release() call prevents the mutexes held by the - // critical section (TYPE_LOCK and the type dict mutex) from being released - // even if we block on the STW mutex. We need to take care that we do not - // deadlock because of that. It is safe because a thread waiting for either - // of those mutexes detaches while it waits and so does not hold up the - // stop-the-world. Pinning both mutexes rather than only TYPE_LOCK is what - // makes this safe: otherwise the dict mutex would be released when we - // block and _PyCriticalSection_Resume() would have to re-acquire it while - // holding TYPE_LOCK, deadlocking with a thread that holds the dict mutex - // and is waiting for TYPE_LOCK. - pinned_mutexes_t pinned; - type_lock_prevent_release(&pinned); +// Apply the queued slot updates with the world stopped, if the publish +// generation is still 'gen' (the value snapshotted before the +// queue was built). Returns 1 if the updates were applied and 0 if the +// queue went stale and must be rebuilt. +// +// The queue was built while holding TYPE_LOCK (and, for type_setattro(), +// the type dict mutex). Both are critical section mutexes, so they were +// released whenever the lookups blocked and are released while this thread +// waits to stop the world. Other threads can update type dicts, MROs and +// slots in the meantime, which is what the generation check detects. +// +// Must be called with TYPE_LOCK held. The lock is re-acquired when the +// thread re-attaches after waiting for the stop-the-world mutex, so it is +// held continuously from before the world is stopped until after it is +// started again. Callers rely on that to invalidate version tags after the +// update, before any other thread can assign a new one. +static int +apply_type_slot_updates(slot_update_t *updates, uint64_t gen) +{ + ASSERT_TYPE_LOCK_HELD(); + int applied = 0; types_stop_world(); - apply_slot_updates(updates); + if (!types_publish_is_stale(gen)) { + apply_slot_updates(updates); + types_publish_gen_inc(); + applied = 1; + } types_start_world(); - type_lock_allow_release(&pinned); + ASSERT_TYPE_LOCK_HELD(); + return applied; } #else @@ -4424,7 +4537,7 @@ type_new_alloc(type_new_ctx *ctx) type->tp_as_mapping = &et->as_mapping; type->tp_as_buffer = &et->as_buffer; - set_tp_bases(type, Py_NewRef(ctx->bases), 1); + set_tp_bases(type, Py_NewRef(ctx->bases)); type->tp_base = (PyTypeObject *)Py_NewRef(ctx->base); type->tp_dealloc = subtype_dealloc; @@ -5630,7 +5743,7 @@ type_from_slots_or_spec( /* Set slots we have prepared */ type->tp_base = (PyTypeObject *)Py_NewRef(base); - set_tp_bases(type, bases, 1); + set_tp_bases(type, bases); bases = NULL; // We give our reference to bases to the type type->tp_doc = tp_doc; @@ -6397,24 +6510,22 @@ void _PyType_SetFlagsRecursive(PyTypeObject *self, unsigned long mask, unsigned long flags) { BEGIN_TYPE_LOCK(); - /* Ideally, changing flags and invalidating the old version tag would - happen in one step. But _PyType_Modified_Unlocked() is re-entrant and - cannot run with the world stopped, so we must invalidate first. - Immutable/static-builtin types are skipped because - set_flags_recursive() does not modify them. */ - if (!PyType_HasFeature(self, Py_TPFLAGS_IMMUTABLETYPE) && - (self->tp_flags & mask) != flags) - { - _PyType_Modified_Unlocked(self); - } - /* Keep TYPE_LOCK held while waiting for stop-the-world so no thread - can reassign a version tag before the flag update. */ - pinned_mutexes_t pinned; - type_lock_prevent_release(&pinned); + // Immutable/static-builtin types are skipped because + // set_flags_recursive() does not modify them. + int modified = (!PyType_HasFeature(self, Py_TPFLAGS_IMMUTABLETYPE) && + (self->tp_flags & mask) != flags); types_stop_world(); set_flags_recursive(self, mask, flags); types_start_world(); - type_lock_allow_release(&pinned); + // Invalidate the version tags after the flag update, while TYPE_LOCK is + // still held: no other thread can assign a new tag before we release it. + // _PyType_Modified_Unlocked() is re-entrant and cannot run with the world + // stopped, and invalidating before the update would not be enough because + // the lock may be released while waiting for the stop-the-world. + ASSERT_TYPE_LOCK_HELD(); + if (modified) { + _PyType_Modified_Unlocked(self); + } END_TYPE_LOCK(); } @@ -6584,7 +6695,7 @@ _Py_type_getattro_stackref(PyTypeObject *type, PyObject *name, // the type versions. static int type_update_dict(PyTypeObject *type, PyDictObject *dict, PyObject *name, - PyObject *value, PyObject **old_value) + PyObject *value, int is_slot_name, PyObject **old_value) { // We don't want any re-entrancy between when we update the dict // and call _PyType_Modified_Unlocked, including running the destructor @@ -6611,6 +6722,10 @@ type_update_dict(PyTypeObject *type, PyDictObject *dict, PyObject *name, return -1; } + if (is_slot_name) { + types_publish_gen_inc(); + } + return 0; } @@ -6625,17 +6740,34 @@ update_slot_after_setattr(PyTypeObject *type, PyObject *name) // the type has more than SLOT_UPDATE_CHUNK_SIZE subclasses. slot_update_t queued_updates = {&chunk}; - int res = update_slot(type, name, &queued_updates); - if (res == 0 && queued_updates.head->n > 0) { - apply_type_slot_updates(&queued_updates); - ASSERT_TYPE_LOCK_HELD(); + int res; + for (;;) { + uint64_t gen = types_publish_gen(); + res = update_slot(type, name, &queued_updates); + if (res < 0 || slot_update_is_empty(&queued_updates)) { + break; + } + if (apply_type_slot_updates(&queued_updates, gen)) { + break; + } + // The queue went stale while TYPE_LOCK was released, build it again. + // Nothing was published, so it does not matter if dropping the + // references here runs type_dealloc() and releases TYPE_LOCK. + slot_update_clear(&queued_updates, &chunk); } - slot_update_chunk_t *cur = queued_updates.head; - while (cur != &chunk) { - slot_update_chunk_t *prev = cur->prev; - PyMem_Free(cur); - cur = prev; + if (res == 0) { + // Invalidate again, after the slots are updated and while TYPE_LOCK + // is still held. Other threads may have assigned new version tags + // while the lock was released and cached decisions based on the old + // slots. This is a no-op if the tags are still invalid. + ASSERT_TYPE_LOCK_HELD(); + _PyType_Modified_Unlocked(type); } + // Drop the references only now. If one of them is the last reference + // to a queued subclass, type_dealloc() takes TYPE_LOCK again, which + // releases it until it is re-acquired. That must not happen between + // publishing the slots and invalidating the version tags above. + slot_update_clear(&queued_updates, &chunk); return res; #else update_slot(type, name, NULL); @@ -6723,14 +6855,15 @@ type_setattro(PyObject *self, PyObject *name, PyObject *value) } } + // Does the name correspond to a type slot? + int is_slot_name = is_dunder_name(name) && has_slotdef(name); + BEGIN_TYPE_DICT_LOCK(dict); - res = type_update_dict(type, (PyDictObject *)dict, name, value, &old_value); + res = type_update_dict(type, (PyDictObject *)dict, name, value, + is_slot_name, &old_value); assert(_PyType_CheckConsistency(type)); - if (res == 0) { - if (is_dunder_name(name) && has_slotdef(name)) { - // The name corresponds to a type slot. - res = update_slot_after_setattr(type, name); - } + if (res == 0 && is_slot_name) { + res = update_slot_after_setattr(type, name); } END_TYPE_DICT_LOCK(); @@ -9021,7 +9154,7 @@ type_ready_set_bases(PyTypeObject *type, int initial) if (bases == NULL) { return -1; } - set_tp_bases(type, bases, 1); + set_tp_bases(type, bases); } return 0; } @@ -12093,7 +12226,8 @@ fixup_slot_dispatchers(PyTypeObject *type) #ifdef Py_GIL_DISABLED -// Called when __bases__ is re-assigned. +// Called when __bases__ is re-assigned. Also clears the VALID_VERSION +// flag of 'type' and all its subclasses. static int update_all_slots(PyTypeObject* type) { @@ -12103,25 +12237,36 @@ update_all_slots(PyTypeObject* type) // can fail internally for other reasons (a lookup fails) but those // errors are suppressed. slot_update_t queued_updates = {0}; - for (pytype_slotdef *p = slotdefs; p->name; p++) { - if (update_slot(type, p->name_strobj, &queued_updates) < 0) { - if (queued_updates.head) { - slot_update_free_chunks(&queued_updates); + for (;;) { + uint64_t gen = types_publish_gen(); + for (pytype_slotdef *p = slotdefs; p->name; p++) { + if (update_slot(type, p->name_strobj, &queued_updates) < 0) { + slot_update_clear(&queued_updates, NULL); + return -1; } - return -1; } + if (slot_update_is_empty(&queued_updates)) { + break; + } + if (apply_type_slot_updates(&queued_updates, gen)) { + break; + } + // The queue went stale while TYPE_LOCK was released, build it again. + // Nothing was published, so it does not matter if dropping the + // references here runs type_dealloc() and releases TYPE_LOCK. + slot_update_clear(&queued_updates, NULL); } - if (queued_updates.head != NULL) { - apply_type_slot_updates(&queued_updates); - ASSERT_TYPE_LOCK_HELD(); - slot_update_free_chunks(&queued_updates); - } + // Invalidate the version tags while TYPE_LOCK is still held, before + // dropping the references: see update_slot_after_setattr(). + _PyType_Modified_Unlocked(type); + slot_update_clear(&queued_updates, NULL); return 0; } #else -// Called when __bases__ is re-assigned. +// Called when __bases__ is re-assigned. Also clears the VALID_VERSION +// flag of 'type' and all its subclasses. static int update_all_slots(PyTypeObject* type) { @@ -12130,6 +12275,7 @@ update_all_slots(PyTypeObject* type) /* update_slot returns int but can't actually fail in this case*/ update_slot(type, p->name_strobj, NULL); } + _PyType_Modified_Unlocked(type); return 0; }