gh-117657: TSAN Fix races in PyMember_Get and PyMember_Set, for C extensions#123211
gh-117657: TSAN Fix races in PyMember_Get and PyMember_Set, for C extensions#123211colesbury merged 32 commits intopython:mainfrom
PyMember_Get and PyMember_Set, for C extensions#123211Conversation
picnixz
left a comment
There was a problem hiding this comment.
Quite hard to review since everything looks the same and the functions are not ordered by pairs apparently. I didn't go through everything but I had a question and an observation.
|
I'm not exactly sure why Anyways, the TSAN checks for |
I've relaunched the test manually (I can't relaunch the SSL test though, I don't know why) |
colesbury
left a comment
There was a problem hiding this comment.
Thanks. I left a few comments below.
How long do the added tests takes to run?
|
I finally had some time to come back to this. I guess that moving the stores after error checking does slightly change the externally-visible behavior. |
|
!buildbot nogil |
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 6c5cec5 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
|
Thanks @dpdani! |
Fix data races that would only be visible when using C extensions.
This is a follow-up on #119368.
I'm intentionally not testing:
_Py_T_NONE(deprecated)_Py_T_OBJECT(deprecated)Py_T_STRING(immutable)Py_T_STRING_INPLACE(immutable)Py_T_CHAR
For some reason
Py_T_CHARis untested also intest_capi.test_structmembers.In fact, it's not even in the supporting C types, which I'm using for the TSAN tests as well: old api, new api.
I'm wondering if there's a specific reason for this, or if it should be in the test suite instead.
I'm guessing that the TSAN suite should cover the thread-safety of
Py_T_CHARregardless?Or should we dismiss it for TSAN tests as well?
I've added a new member to the
T_CHARmember to the_testcapimodule, and the rest of the tests don't seem to break.I can revert this change if needed.