gh-146270: Fix PyMember_SetOne(..., NULL) not being atomic

[dpdani]

This PR fixes a sequential consistency bug (introduced by yours truly) whereby two threads that are deleting a struct member may observe both their deletions to be successful.

In order to test this properly, I couldn't use threading.Barrier because its overhead was enough to mask the bug, making the test flaky. Therefore, a spinning-loop barrier was added in the _testcapi module.

• Issue: slot.__delete__() behaves as non-atomic in free-threading #146270

[colesbury]

Drop these comments. They're not sufficiently helpful to a reader.

[colesbury]

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);

[dpdani]

I didn't understand what you meant with "The l->type is redundant."

I have expanded a comment, hopefully it clarifies why that check is needed?

[colesbury]

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)

[dpdani]

When you say it's too slow, what upper bound do you have in mind?

Btw, with your test, I cannot get it to fail when reverting structmember.c, nor did I get TSan warnings in 5 out of 5 attempts.

I think the reason why that's so is that just starting threads doesn't generate enough contention: they're not actually going to hit the current *(PyObject **)addr == NULL at line 177 concurrently with the FT_ATOMIC_STORE_PTR_RELEASE at line 335. This is why I added SpinningBarrier, to generate more contention and have them hit these lines concurrently, in a reliable way.

I can also just reduce the number of iterations in my test. Even with just iters = 10, TSan reported a race 10 out of 10 times. I kinda put 1000 to begin with and then forgot about it.

With iters = 10, the whole test suite runs in about 35ms on my MacBook, albeit the test doesn't fail reliably with TSan turned off, only failing ~33% of the times.

[colesbury]

You should aim for <100ms. I get a TSan error on nearly every run

[dpdani]

With the current test on 10 iterations, the whole suite runs in <50ms, so I think that should be fine.

It's curious that you're getting more TSan errors, on my macbook I'm seeing 1 TSan warning every ~20 runs. Can I ask you what's your ./configure? Mine is ./configure --config-cache --disable-gil --with-pydebug --with-thread-sanitizer.

Btw, if I use your test code and switch threading.Barrier -> _testcapi.SpinningBarrier inside run_concurrently, I saw a TSan warning on 100 out of 100 runs.

To be clear, I'm not suggesting to use SpinningBarrier in run_concurrently, I'm saying that it definitely gives more determinism to the results, and I think it's worth keeping it.

[colesbury]

I don't think it's worth adding this

[dpdani]

Without SpinningBarrier the bug becomes significantly harder to reproduce. Also see my comment above.

[colesbury]

It's still not worth it. I'm more than happy to accept reduced chances of catching one specific regression per test run in exchange for avoiding this.

[colesbury]

Thanks for fixing this! I left some comments above.

I think it's worth prioritizing keeping the tests fast. In general, it helps to keep them small and understandable too.