gh-111926: Make weakrefs thread-safe in free-threaded builds #117168
Merged
Conversation
This file contains 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
We need to tag weakrefs and their referents
The `is_dead` check from the default build is insufficient, since the refcount may go to zero between when we check and when we incref. More generally, `Py_NewRef` is unsafe to use if you have a borrowed refernce.
Locking is handled in the implementation
|
@colesbury - Would you take another look at this when you get a chance, please? |
colesbury
reviewed
Apr 1, 2024
colesbury
added
the
🔨 test-with-refleak-buildbots
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit fece88d 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
We cannot assert now that we clear weakrefs without holding the lock in _PyStaticType_ClearWeakRefs
erlend-aasland
approved these changes
Apr 4, 2024
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
colesbury
added
the
🔨 test-with-refleak-buildbots
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 73466bf 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
diegorusso
pushed a commit
to diegorusso/cpython
that referenced
this pull request
Apr 17, 2024
…ython#117168) Most mutable data is protected by a striped lock that is keyed on the referenced object's address. The weakref's hash is protected using the weakref's per-object lock. Note that this only affects free-threaded builds. Apart from some minor refactoring, the added code is all either gated by `ifdef`s or is a no-op (e.g. `Py_BEGIN_CRITICAL_SECTION`).
|
I've started seeing a test failure that bisects to this PR. Please see issue #118331. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 makes weakrefs thread-safe in free-threaded builds. Please see the comment at the beginning of
Objects/weakrefobject.cfor an explanation of the approach.Note that this only affects free-threaded builds. Apart from some minor refactoring, the added code is all either gated by
ifdefs or is a no-op (e.g.Py_BEGIN_CRITICAL_SECTION).