diff --git a/Lib/test/test_free_threading/test_slots.py b/Lib/test/test_free_threading/test_slots.py index a3b9f4b0175ae7..8357a2a75043b1 100644 --- a/Lib/test/test_free_threading/test_slots.py +++ b/Lib/test/test_free_threading/test_slots.py @@ -43,6 +43,50 @@ def reader(): run_in_threads([writer, reader, reader, reader]) + def test_del_object_is_atomic(self): + # Testing whether the implementation of `del slots_object.attribute` + # removes the attribute atomically, thus avoiding non-sequentially- + # consistent behaviors. + # https://github.com/python/cpython/issues/146270 + + class Spam: + __slots__ = [ + "eggs", + "foo", + ] + + spam = Spam() + non_seq_cst_behaviour_observed = False + + def deleter(): + barrier.wait() + try: + del spam.eggs + except AttributeError: + pass + else: + try: + del spam.foo + except AttributeError: + nonlocal non_seq_cst_behaviour_observed + non_seq_cst_behaviour_observed = True + # The fact that the else branch was reached implies that + # the `del spam.eggs` call was successful. If that were + # atomic, there is no way for two threads to enter the else + # branch, therefore it must be that only one thread + # attempts to `del spam.foo`. Thus, hitting an + # AttributeError here is non-sequentially-consistent, + # falsifying the assumption that `del spam.eggs` was + # atomic. The test fails if this point is reached. + + for _ in range(10): + spam.eggs = 0 + spam.foo = 0 + barrier = _testcapi.SpinningBarrier(2) + # threading.Barrier would not create enough contention here + run_in_threads([deleter, deleter]) + self.assertFalse(non_seq_cst_behaviour_observed) + def test_T_BOOL(self): spam_old = _testcapi._test_structmembersType_OldAPI() spam_new = _testcapi._test_structmembersType_NewAPI() diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-04-20-15-25-55.gh-issue-146270.qZYfyc.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-04-20-15-25-55.gh-issue-146270.qZYfyc.rst new file mode 100644 index 00000000000000..339697d6f3132e --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-04-20-15-25-55.gh-issue-146270.qZYfyc.rst @@ -0,0 +1 @@ +Fix a sequential consistency bug in ``structmember.c`` and add ``_testcapi.SpinningBarrier`` to support the new test. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 3ebe4ceea6a72e..35f251e3c86971 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -2642,6 +2642,108 @@ test_soft_deprecated_macros(PyObject *Py_UNUSED(self), PyObject *Py_UNUSED(args) Py_RETURN_NONE; } +/** + * A spinning barrier is a multithreading barrier similar to threading.Barrier, + * except that it never parks threads that are waiting on the barrier. + * + * This is useful in scenarios where it is desirable to increase contention on + * the code that follows the barrier. For instance, consider this test: + * + * def test_my_method_is_atomic(): + * x = MyClass() + * b = _testcapi.SpinningBarrier() + * + * def thread(): + * b.wait() + * x.my_method() + * + * for _ in range(1_000): + * threads = [Thread(target=thread), Thread(target=thread)] + * for t in threads: t.start() + * for t in threads: t.join() + * + * It can be desirable (and sometimes necessary) to increase the contention + * on x's internal data structure by avoiding threads parking. + * Here, not parking may become necessary when the code in my_method() is so + * short that contention-related code paths are never exercised otherwise. + * + * It is roughly equivalent to this Python class: + * + * class SpinningBarrier: + * def __init__(self, parties: int): + * self.parties = AtomicInt(parties) # if only we had atomic integers + * + * def wait(self): + * v = self.parties.decrement_and_get() + * while True: + * if v < 0: + * raise ValueError("wait was called too many times") + * if v == 0: + * return + * v = self.parties.get() + * + */ + +typedef struct { + PyObject_HEAD + int parties; +} SpinningBarrier; + +int +SpinningBarrier_init(PyObject *self, PyObject *args, PyObject *kwargs) +{ + int parties = 0; + const char *kwlist[] = {"parties", NULL}; + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "i", (char **)kwlist, &parties)) { + return -1; + } + if (parties <= 0) { + PyErr_SetString(PyExc_ValueError, "parties must be greater than zero"); + return -1; + } + + SpinningBarrier *self_b = (SpinningBarrier *) self; + self_b->parties = parties; + + return 0; +} + +PyObject * +SpinningBarrier_wait(PyObject *self, PyObject *Py_UNUSED(args)) +{ + SpinningBarrier *self_b = (SpinningBarrier *) self; + const long decremented = _Py_atomic_add_int(&self_b->parties, -1) - 1; + long v = decremented; + while (1) { + if (v < 0) { + PyErr_SetString(PyExc_ValueError, "wait was called too many times"); + return NULL; + } + if (v == 0) { + return PyLong_FromLong(decremented); + } + v = _Py_atomic_load_int_relaxed(&self_b->parties); + if (PyErr_CheckSignals()) { + return NULL; + } + } +} + +static PyMethodDef SpinningBarrier_methods[] = { + {"wait", SpinningBarrier_wait, METH_NOARGS}, + {NULL, NULL}, +}; + +static PyTypeObject SpinningBarrier_Type = { + PyVarObject_HEAD_INIT(NULL, 0) + .tp_name = "SpinningBarrier", + .tp_basicsize = sizeof(SpinningBarrier), + .tp_new = PyType_GenericNew, + .tp_free = PyObject_Free, + .tp_init = &SpinningBarrier_init, + .tp_methods = SpinningBarrier_methods, +}; + static PyMethodDef TestMethods[] = { {"set_errno", set_errno, METH_VARARGS}, {"test_config", test_config, METH_NOARGS}, @@ -3369,6 +3471,12 @@ _testcapi_exec(PyObject *m) Py_INCREF(&MethStatic_Type); PyModule_AddObject(m, "MethStatic", (PyObject *)&MethStatic_Type); + if (PyType_Ready(&SpinningBarrier_Type) < 0) { + return -1; + } + Py_INCREF(&SpinningBarrier_Type); + PyModule_AddObject(m, "SpinningBarrier", (PyObject *)&SpinningBarrier_Type); + PyModule_AddObject(m, "CHAR_MAX", PyLong_FromLong(CHAR_MAX)); PyModule_AddObject(m, "CHAR_MIN", PyLong_FromLong(CHAR_MIN)); PyModule_AddObject(m, "UCHAR_MAX", PyLong_FromLong(UCHAR_MAX)); diff --git a/Python/structmember.c b/Python/structmember.c index b88e13ac0462b8..adea8216b8796b 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -171,19 +171,10 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) PyErr_SetString(PyExc_AttributeError, "readonly attribute"); return -1; } - if (v == NULL) { - if (l->type == Py_T_OBJECT_EX) { - /* Check if the attribute is set. */ - if (*(PyObject **)addr == NULL) { - PyErr_SetString(PyExc_AttributeError, l->name); - return -1; - } - } - else if (l->type != _Py_T_OBJECT) { - PyErr_SetString(PyExc_TypeError, - "can't delete numeric/char attribute"); - return -1; - } + if (v == NULL && l->type != Py_T_OBJECT_EX && l->type != _Py_T_OBJECT) { + PyErr_SetString(PyExc_TypeError, + "can't delete numeric/char attribute"); + return -1; } switch (l->type) { case Py_T_BOOL:{ @@ -334,6 +325,15 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) oldv = *(PyObject **)addr; FT_ATOMIC_STORE_PTR_RELEASE(*(PyObject **)addr, Py_XNewRef(v)); Py_END_CRITICAL_SECTION(); + if (v == NULL && oldv == NULL && l->type == Py_T_OBJECT_EX) { + // Raise an exception when attempting to delete an already deleted + // attribute. + // Differently from Py_T_OBJECT_EX, _Py_T_OBJECT does not raise an + // exception here (PyMember_GetOne will return Py_None instead of + // NULL). + PyErr_SetString(PyExc_AttributeError, l->name); + return -1; + } Py_XDECREF(oldv); break; case Py_T_CHAR: { diff --git a/Tools/c-analyzer/cpython/ignored.tsv b/Tools/c-analyzer/cpython/ignored.tsv index d2489387f46caa..4933a08aca429e 100644 --- a/Tools/c-analyzer/cpython/ignored.tsv +++ b/Tools/c-analyzer/cpython/ignored.tsv @@ -542,6 +542,7 @@ Modules/_testcapimodule.c - meth_class_methods - Modules/_testcapimodule.c - meth_instance_methods - Modules/_testcapimodule.c - meth_static_methods - Modules/_testcapimodule.c - ml - +Modules/_testcapimodule.c - SpinningBarrier_Type - Modules/_testcapimodule.c - str1 - Modules/_testcapimodule.c - str2 - Modules/_testcapimodule.c - test_c_thread -