Skip to content

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

Merged
colesbury merged 67 commits into
python:mainfrom
mpage:gh-111926-thread-safe-weakref
Apr 8, 2024
Merged

gh-111926: Make weakrefs thread-safe in free-threaded builds#117168
colesbury merged 67 commits into
python:mainfrom
mpage:gh-111926-thread-safe-weakref

Conversation

@mpage

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

Copy link
Copy Markdown
Contributor

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
@mpage
mpage 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

mpage commented Mar 29, 2024

Copy link
Copy Markdown
ContributorAuthor

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

@mpage
mpage requested a review from colesburyMarch 29, 2024 21:48
colesbury
colesbury reviewed Apr 1, 2024
View reviewed changes

@colesburycolesbury left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

Comment threadModules/_sqlite/blob.c
Comment threadModules/_sqlite/connection.c Outdated
Comment threadModules/_weakref.c
Comment threadObjects/weakrefobject.c Outdated
Comment threadObjects/weakrefobject.c Outdated
Comment threadObjects/weakrefobject.c Outdated
Comment threadObjects/weakrefobject.c Outdated
Comment threadObjects/weakrefobject.c Outdated
Comment threadObjects/weakrefobject.c Outdated
Comment threadInclude/cpython/weakrefobject.h
@colesburycolesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 1, 2024
@bedevere-bot

bedevere-bot commented Apr 1, 2024

Copy link
Copy Markdown

🤖 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
@mpage
mpage requested a review from colesburyApril 2, 2024 19:44
colesbury
colesbury approved these changes Apr 3, 2024
View reviewed changes

@colesburycolesbury left a comment

Copy link
Copy Markdown
Contributor

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
Comment threadPython/pystate.c Outdated
@colesbury@erlend-aasland
Update Python/pystate.c
73466bf
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@colesburycolesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 8, 2024
@bedevere-bot

bedevere-bot commented Apr 8, 2024

Copy link
Copy Markdown

🤖 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
@colesbury
colesbury 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

hugovk commented Apr 26, 2024

Copy link
Copy Markdown
Member

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 berkerpeksag
@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