-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
gh-146270: Fix PyMember_SetOne(..., NULL) not being atomic
#148800
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Fix a sequential consistency bug in ``structmember.c`` and add ``_testcapi.SpinningBarrier`` to support the new test. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2642,6 +2642,108 @@ test_soft_deprecated_macros(PyObject *Py_UNUSED(self), PyObject *Py_UNUSED(args) | |
| Py_RETURN_NONE; | ||
| } | ||
|
|
||
| /** | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's worth adding this |
||
| * 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)); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
| } | ||
|
Comment on lines
+329
to
+337
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
if (v == NULL && oldv == NULL && l->type == Py_T_OBJECT_EX) {
// Raise an exception when attempting to delete an already deleted attribute
PyErr_SetString(PyExc_AttributeError, l->name);
return -1;
}
Py_XDECREF(oldv); |
||
| // 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 | ||
|
Comment on lines
+338
to
+341
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drop these comments. They're not sufficiently helpful to a reader. |
||
| break; | ||
| case Py_T_CHAR: { | ||
| const char *string; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is too slow. It's not worth trying to catch every sort non-sequential consistency. If you catch the data race under TSan reasonably often, that's fine: