Skip to content

gh-111926: Make weakrefs thread-safe in free-threaded builds#117168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
colesbury merged 67 commits into from
Apr 8, 2024
Merged

gh-111926: Make weakrefs thread-safe in free-threaded builds #117168

colesbury merged 67 commits into from
Apr 8, 2024

Conversation

@mpage
Copy link
Contributor

@mpagempage commented Mar 22, 2024•
edited by bedevere-app bot
Loading

This makes weakrefs thread-safe in free-threaded builds. Please see the comment at the beginning of Objects/weakrefobject.c for 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).

mpage added 21 commits March 22, 2024 12:29
@mpage
Add _PyObject_SetMaybeWeakref
9bf3820
We need to tag weakrefs and their referents
@mpage
Make PyWeakref_NewRef thread-safe
75603c1
@mpage
Make PyWeakref_NewProxy thread-safe
c4f8453
@mpage
Make weakref___new___ thread-safe
ece230f
@mpage
Make weakref_hash thread-safe
9acae59
@mpage
Make _PyStaticType_ClearWeakRefs thread-safe
6d00f51
@mpage
Handle multiple weakrefs
363a1a6
@mpage
Make _PyWeakref_GetWeakrefCount thread-safe
c9b3bf9
@mpage
Make _weakref._getweakrefs thread-safe
b7b3315
@mpage
Make weakref clearing in subtype_dealloc thread-safe
e6e2d28
@mpage
Fix incorrect size in test_sys for weakref/proxy
0d259d1
@mpage
Use a once flag
7cc523b
@mpage
Simplify clearing callback-free refs
11be247
@mpage
Rationalize naming
8fcd33f
@mpage
Fix warning
1e1e609
@mpage
Simplify ClearWeakRefs
19a274e
@mpage
Refactor clearing code
817a5a3
@mpage
Protect wr_object with a separate mutex
d493e7d
@mpage
Switch to a sharded lock for linked list and object
88078a6
@mpage
Fix formatting
dc1f651
@mpage
Remove vestigial include
37c511a
@bedevere-appbedevere-appbot mentioned this pull request Mar 22, 2024
Closed
@mpagempage requested a review from colesburyMarch 22, 2024 22:13
mpage added 5 commits March 22, 2024 16:45
@mpage
Make sure we set the cleared flag
ccaeded
@mpage
Use Py_TryIncref when returning the ref
6a50381
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.
@mpage
Don't leak ref if initialization fails
d998d1b
@mpage
Remove clinic critical section directives for _weakref module
37076b5
Locking is handled in the implementation
@mpage
Support async removal of dead weakrefs from WeakValueDictionary
a07d2db
@colesburycolesbury mentioned this pull request Mar 25, 2024
Closed
@mpage
Copy link
ContributorAuthor

mpage commented Mar 29, 2024

@colesbury - Would you take another look at this when you get a chance, please?

@mpagempage requested a review from colesburyMarch 29, 2024 21:48
colesbury
colesbury reviewed Apr 1, 2024
View reviewed changes
Copy link
Contributor

@colesburycolesbury left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@colesburycolesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 1, 2024
@bedevere-bot
Copy link

bedevere-bot commented 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.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 1, 2024
@mpagempage requested a review from colesburyApril 2, 2024 19:44
colesbury
colesbury approved these changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@colesburycolesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

erlend-aasland
erlend-aasland approved these changes Apr 4, 2024
View reviewed changes
@colesbury@erlend-aasland
Update Python/pystate.c
73466bf
Co-authored-by: Erlend E. Aasland <[email protected]>
@colesburycolesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 8, 2024
@bedevere-bot
Copy link

bedevere-bot commented 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.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 8, 2024
@colesburycolesbury merged commit df73179 into python:mainApr 8, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@mpage@diegorusso
pythongh-111926: Make weakrefs thread-safe in free-threaded builds (p…
a1c8a00
…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`).
@hugovkhugovk mentioned this pull request Apr 26, 2024
Closed
@hugovk
Copy link
Member

hugovk commented Apr 26, 2024

I've started seeing a test failure that bisects to this PR. Please see issue #118331.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesburycolesburycolesbury approved these changes

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksagberkerpeksag is a code owner

@methanemethaneAwaiting requested review from methanemethane is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mpage@bedevere-bot@hugovk@colesbury@erlend-aasland