gh-111926: Make weakrefs thread-safe in free-threaded builds#117168
Conversation
Sorry, something went wrong.
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? |
Sorry, something went wrong.
There was a problem hiding this comment.
This is looking good. Some comments inline.
Additionally, proxy_dealloc should remove the redundant PyObject_GC_UnTrack inside the if statement. It's unnecessary and the plain read of self->wr_callback is possibly a data race.
Sorry, something went wrong.
colesbury
added
the
🔨 test-with-refleak-buildbots
Test PR w/ refleak buildbots; report in status section
label
Apr 1, 2024
|
🤖 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. |
Sorry, something went wrong.
We cannot assert now that we clear weakrefs without holding the lock in _PyStaticType_ClearWeakRefs
colesbury
left a comment
colesbury
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
colesbury
added
the
🔨 test-with-refleak-buildbots
Test PR w/ refleak buildbots; report in status section
label
Apr 8, 2024
|
🤖 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. |
Sorry, something went wrong.
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
Test PR w/ refleak buildbots; report in status section
label
Apr 8, 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. |
Sorry, something went wrong.
Reviewers
5 participants
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).