gh-127266: avoid data races when updating type slots v2#133177
Merged
nascheme merged 24 commits intopython:mainfrom May 28, 2025
Merged
gh-127266: avoid data races when updating type slots v2#133177nascheme merged 24 commits intopython:mainfrom
nascheme merged 24 commits intopython:mainfrom
Conversation
In the free-threaded build, avoid data races caused by updating type slots or type flags after the type was initially created. For those (typically rare) cases, use the stop-the-world mechanism. Remove the use of atomics when reading or writing type flags. The use of atomics is not sufficient to avoid races (since flags are sometimes read without a lock and without atomics) and are no longer required.
To avoid deadlocks while the world is stopped, we need to avoid calling APIs like _PyObject_HashFast() and _PyDict_GetItemRef_KnownHash(). Collect the slot updates to be done and then apply them all at once. This reduces the amount of code running in the stop-the-world condition.
nascheme
added
type-bug
nascheme
added
the
🔨 test-with-buildbots
bedevere-bot
removed
the
🔨 test-with-buildbots
Now that stop-the-world is used to do the slot update, these tests are a lot slower in the free-threaded build. Test with fewer items, which will still hopefully be enough to find bugs in the specializer.
The clearing of Py_TPFLAGS_HAVE_VECTORCALL must be done when the world is stopped too.
nascheme
added
the
🔨 test-with-buildbots
bedevere-bot
removed
the
🔨 test-with-buildbots
Since we stack allocate one chunk, we need to check 'n' to see if there are actually any updates to make. It's pretty common that no updates are actually needed.
colesbury
reviewed
May 1, 2025
Member
Author
|
@colesbury Not sure if you would like this approach but it is a variation on your idea to modify the critical section to prevent release of the mutex. I changed it to work with a PyCriticalSection2 as well. Not using a critical section for the type dict looks kind of difficult to do. For example, |
Contributor
|
The |
If the two mutex form of the critical section is used, need to put the other mutex into '_cs_mutex'.
nascheme
added
the
🔨 test-with-buildbots
bedevere-bot
removed
the
🔨 test-with-buildbots
This test gets a bit slower, due to stop-the-world but it is not so dramatic.
nascheme
added
the
🔨 test-with-buildbots
bedevere-bot
removed
the
🔨 test-with-buildbots
colesbury
reviewed
May 27, 2025
The apply_slot_updates_world_stopped() name implies that the world is already stopped, based on convention. Rename for clarity.
Pranjal095
pushed a commit
to Pranjal095/cpython
that referenced
this pull request
Jul 12, 2025
…133177) In the free-threaded build, avoid data races caused by updating type slots or type flags after the type was initially created. For those (typically rare) cases, use the stop-the-world mechanism. Remove the use of atomics when reading or writing type flags.
taegyunkim
pushed a commit
to taegyunkim/cpython
that referenced
this pull request
Aug 4, 2025
…133177) In the free-threaded build, avoid data races caused by updating type slots or type flags after the type was initially created. For those (typically rare) cases, use the stop-the-world mechanism. Remove the use of atomics when reading or writing type flags.
Contributor
|
Should this be backported to 3.14? |
Member
Author
It feels a little scary to backport this to a stable release. It's not a simple fix. OTOH, it seems to be working in 3.15 without issue. |
3 tasks
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
4 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 is an updated version of GH-131174, which was reverted. I figured the cleanest thing to do is make a new PR.
This is the same as the previous PR with the following additional change. The
update_all_slots()andtype_setattro()functions are now more careful when the world is stopped. Instead of doing the MRO lookups while the world is stopped, we do them all first and collect the slot pointers to be updated. Then, we stop the world and do those updates. This makes it much easier to confirm the code running during the stop-the-world is safe and that should avoid the deadlocks.The
test_opcachetest has become quite a bit slower. It seems to be due to mutex contention in the__getitem__and__getattribute__method assignment tests. I reduced the items count from 1000 to 100 to keep the test from becoming much slower.