gh-146270: Fix PyMember_SetOne(..., NULL) not being atomic#148800
Open
dpdani wants to merge 3 commits intopython:mainfrom
Open
gh-146270: Fix PyMember_SetOne(..., NULL) not being atomic#148800dpdani wants to merge 3 commits intopython:mainfrom
PyMember_SetOne(..., NULL) not being atomic#148800dpdani wants to merge 3 commits intopython:mainfrom
Conversation
colesbury
reviewed
Apr 20, 2026
Comment on lines
+338
to
+341
| // 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 |
Contributor
There was a problem hiding this comment.
Drop these comments. They're not sufficiently helpful to a reader.
Comment on lines
+329
to
+337
| 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; | ||
| } |
Contributor
There was a problem hiding this comment.
- Keep the comments simple, i.e., "deleting an already deleted attribute raises an exception
- Move the decref after the check. Reading a pointer that has been freed, even if it's just a NULL check, is UB
- The
l->typeis 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);|
|
||
| run_in_threads([writer, reader, reader, reader]) | ||
|
|
||
| def test_del_object_is_atomic(self): |
Contributor
There was a problem hiding this comment.
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)| Py_RETURN_NONE; | ||
| } | ||
|
|
||
| /** |
Contributor
There was a problem hiding this comment.
I don't think it's worth adding this
Contributor
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.Barrierbecause its overhead was enough to mask the bug, making the test flaky. Therefore, a spinning-loop barrier was added in the_testcapimodule.slot.__delete__()behaves as non-atomic in free-threading #146270