Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions Lib/test/test_free_threading/test_slots.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,51 @@ def reader():

run_in_threads([writer, reader, reader, reader])

def test_del_object_is_atomic(self):
Copy link
Copy Markdown
Contributor

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:

from test.support.threading_helper import run_concurrently

...

        class Spam:
            __slots__ = [ "foo" ]

        def deleter(spam, successes):
            try:
                del spam.foo
                successes.append(True)
            except AttributeError:
                successes.append(False)

        for _ in range(10):
            spam = Spam()
            spam.foo = 0
            successes = []
            run_concurrently(deleter, nthreads=4, args=(spam, successes))
            self.assertEqual(sum(successes), 1)

# 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()
Expand Down
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.
108 changes: 108 additions & 0 deletions Modules/_testcapimodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -2642,6 +2642,108 @@ test_soft_deprecated_macros(PyObject *Py_UNUSED(self), PyObject *Py_UNUSED(args)
Py_RETURN_NONE;
}

/**
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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},
Expand Down Expand Up @@ -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));
Expand Down
30 changes: 17 additions & 13 deletions Python/structmember.c
Original file line number Diff line number Diff line change
Expand Up @@ -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:{
Expand Down Expand Up @@ -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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Keep the comments simple, i.e., "deleting an already deleted attribute raises an exception
  2. Move the decref after the check. Reading a pointer that has been freed, even if it's just a NULL check, is UB
  3. The l->type is redundant
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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;
Expand Down
1 change: 1 addition & 0 deletions Tools/c-analyzer/cpython/ignored.tsv
Original file line number Diff line number Diff line change
Expand Up @@ -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 -
Expand Down
Loading