From 98cbbc04c283029166c3305db7ff0a349b11c4da Mon Sep 17 00:00:00 2001 From: Daniele Parmeggiani Date: Wed, 1 Apr 2026 17:41:41 +0100 Subject: [PATCH 1/6] gh-146270: Fix `PyMember_SetOne(..., NULL)` not being atomic --- Lib/test/test_free_threading/test_slots.py | 45 +++++++++ Modules/_testcapimodule.c | 108 +++++++++++++++++++++ Python/structmember.c | 30 +++--- Tools/c-analyzer/cpython/ignored.tsv | 1 + 4 files changed, 171 insertions(+), 13 deletions(-) diff --git a/Lib/test/test_free_threading/test_slots.py b/Lib/test/test_free_threading/test_slots.py index a3b9f4b0175ae7..351819ca4ac19a 100644 --- a/Lib/test/test_free_threading/test_slots.py +++ b/Lib/test/test_free_threading/test_slots.py @@ -43,6 +43,51 @@ 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() + iters = 1_000 + 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(iters): + 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/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..8cd2c928605bfa 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:{ @@ -335,6 +326,19 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) FT_ATOMIC_STORE_PTR_RELEASE(*(PyObject **)addr, Py_XNewRef(v)); Py_END_CRITICAL_SECTION(); Py_XDECREF(oldv); + if (v == NULL && oldv == NULL && l->type == Py_T_OBJECT_EX) { + // Pseudo-non-existing attribute is deleted: raise AttributeError. + // The attribute doesn't exist to Python, but CPython knows that it + // could have existed because it was declared in __slots__. + // _Py_T_OBJECT does not raise an exception here, and + // PyMember_GetOne will return Py_None instead of NULL. + PyErr_SetString(PyExc_AttributeError, l->name); + return -1; + } + // Other cases are already covered by the above: + // oldv == NULL && v != NULL: pseudo-non-existing attribute is set, ok + // oldv != NULL && v == NULL: existing attribute is deleted, ok + // oldv != NULL && v != NULL: existing attribute is set, ok break; case Py_T_CHAR: { const char *string; 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 - From 7a7114bbefabbf032c4c434d092193250cff0b86 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Mon, 20 Apr 2026 15:25:59 +0000 Subject: [PATCH 2/6] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2026-04-20-15-25-55.gh-issue-146270.qZYfyc.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2026-04-20-15-25-55.gh-issue-146270.qZYfyc.rst 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..03374c174d3538 --- /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. From 47b58c9035c96e9d477f3bc6003fa9718878edff Mon Sep 17 00:00:00 2001 From: Daniele Parmeggiani Date: Mon, 20 Apr 2026 16:28:55 +0100 Subject: [PATCH 3/6] fix backticks --- .../2026-04-20-15-25-55.gh-issue-146270.qZYfyc.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 03374c174d3538..339697d6f3132e 100644 --- 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 @@ -1 +1 @@ -Fix a sequential consistency bug in `structmember.c` and add `_testcapi.SpinningBarrier` to support the new test. +Fix a sequential consistency bug in ``structmember.c`` and add ``_testcapi.SpinningBarrier`` to support the new test. From 015795a841734df01c8fcec1cd47794a90415d4d Mon Sep 17 00:00:00 2001 From: Daniele Parmeggiani Date: Tue, 21 Apr 2026 10:33:16 +0100 Subject: [PATCH 4/6] address some review comments --- Python/structmember.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/Python/structmember.c b/Python/structmember.c index 8cd2c928605bfa..adea8216b8796b 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -325,20 +325,16 @@ 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(); - Py_XDECREF(oldv); if (v == NULL && oldv == NULL && l->type == Py_T_OBJECT_EX) { - // Pseudo-non-existing attribute is deleted: raise AttributeError. - // The attribute doesn't exist to Python, but CPython knows that it - // could have existed because it was declared in __slots__. - // _Py_T_OBJECT does not raise an exception here, and - // PyMember_GetOne will return Py_None instead of NULL. + // 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; } - // Other cases are already covered by the above: - // oldv == NULL && v != NULL: pseudo-non-existing attribute is set, ok - // oldv != NULL && v == NULL: existing attribute is deleted, ok - // oldv != NULL && v != NULL: existing attribute is set, ok + Py_XDECREF(oldv); break; case Py_T_CHAR: { const char *string; From 50c19f66a5a692d0047aa330408571dd0cfc95c1 Mon Sep 17 00:00:00 2001 From: Daniele Parmeggiani Date: Tue, 21 Apr 2026 11:03:06 +0100 Subject: [PATCH 5/6] decrease number of iterations --- Lib/test/test_free_threading/test_slots.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/test/test_free_threading/test_slots.py b/Lib/test/test_free_threading/test_slots.py index 351819ca4ac19a..8357a2a75043b1 100644 --- a/Lib/test/test_free_threading/test_slots.py +++ b/Lib/test/test_free_threading/test_slots.py @@ -56,7 +56,6 @@ class Spam: ] spam = Spam() - iters = 1_000 non_seq_cst_behaviour_observed = False def deleter(): @@ -80,7 +79,7 @@ def deleter(): # falsifying the assumption that `del spam.eggs` was # atomic. The test fails if this point is reached. - for _ in range(iters): + for _ in range(10): spam.eggs = 0 spam.foo = 0 barrier = _testcapi.SpinningBarrier(2) From e036ecc9190216d847e29159f163e3a33f209990 Mon Sep 17 00:00:00 2001 From: Daniele Parmeggiani Date: Mon, 27 Apr 2026 13:31:40 +0100 Subject: [PATCH 6/6] change test and remove spinningbarrier --- Lib/test/test_free_threading/test_slots.py | 57 +++------ ...-04-20-15-25-55.gh-issue-146270.qZYfyc.rst | 2 +- Modules/_testcapimodule.c | 108 ------------------ Tools/c-analyzer/cpython/ignored.tsv | 1 - 4 files changed, 17 insertions(+), 151 deletions(-) diff --git a/Lib/test/test_free_threading/test_slots.py b/Lib/test/test_free_threading/test_slots.py index 8357a2a75043b1..a73525e1bebfb4 100644 --- a/Lib/test/test_free_threading/test_slots.py +++ b/Lib/test/test_free_threading/test_slots.py @@ -16,18 +16,19 @@ def run_in_threads(targets): thread.join() +class Spam: + __slots__ = [ + "eggs", + ] + + def __init__(self, initial_value): + self.eggs = initial_value + + @threading_helper.requires_working_threading() class TestSlots(TestCase): def test_object(self): - class Spam: - __slots__ = [ - "eggs", - ] - - def __init__(self, initial_value): - self.eggs = initial_value - spam = Spam(0) iters = 20_000 @@ -48,44 +49,18 @@ def test_del_object_is_atomic(self): # 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() + def deleter(spam, successes): try: del spam.eggs + successes.append(True) 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. + successes.append(False) 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) + spam = Spam(0) + successes = [] + threading_helper.run_concurrently(deleter, nthreads=4, args=(spam, successes)) + self.assertEqual(sum(successes), 1) def test_T_BOOL(self): spam_old = _testcapi._test_structmembersType_OldAPI() 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 index 339697d6f3132e..46c292e183e0fd 100644 --- 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 @@ -1 +1 @@ -Fix a sequential consistency bug in ``structmember.c`` and add ``_testcapi.SpinningBarrier`` to support the new test. +Fix a sequential consistency bug in ``structmember.c``. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 35f251e3c86971..3ebe4ceea6a72e 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -2642,108 +2642,6 @@ 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}, @@ -3471,12 +3369,6 @@ _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/Tools/c-analyzer/cpython/ignored.tsv b/Tools/c-analyzer/cpython/ignored.tsv index 4933a08aca429e..d2489387f46caa 100644 --- a/Tools/c-analyzer/cpython/ignored.tsv +++ b/Tools/c-analyzer/cpython/ignored.tsv @@ -542,7 +542,6 @@ 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 -