From 98cbbc04c283029166c3305db7ff0a349b11c4da Mon Sep 17 00:00:00 2001 From: Daniele Parmeggiani Date: Wed, 1 Apr 2026 17:41:41 +0100 Subject: [PATCH 1/5] 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/5] =?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/5] 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/5] 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/5] 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)