GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-111926: Make weakrefs thread-safe in free-threaded builds by mpage · Pull Request #117168 · python/cpython

Conversation

mpage commented Mar 22, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

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).

Issue: Make weakref thread-safe without the GIL #111926

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mpage added 21 commits

March 22, 2024 12:29


          Add _PyObject_SetMaybeWeakref

9bf3820

We need to tag weakrefs and their referents


          Make PyWeakref_NewRef thread-safe

75603c1


          Make PyWeakref_NewProxy thread-safe

c4f8453


          Make weakref___new___ thread-safe

ece230f


          Make weakref_hash thread-safe

9acae59


          Make _PyStaticType_ClearWeakRefs thread-safe

6d00f51


          Handle multiple weakrefs

363a1a6


          Make _PyWeakref_GetWeakrefCount thread-safe

c9b3bf9


          Make _weakref._getweakrefs thread-safe

b7b3315


          Make weakref clearing in subtype_dealloc thread-safe

e6e2d28


          Fix incorrect size in test_sys for weakref/proxy

0d259d1


          Use a once flag

7cc523b


          Simplify clearing callback-free refs

11be247


          Rationalize naming

8fcd33f


          Fix warning

1e1e609


          Simplify ClearWeakRefs

19a274e


          Refactor clearing code

817a5a3


          Protect wr_object with a separate mutex

d493e7d


          Switch to a sharded lock for linked list and object

88078a6


          Fix formatting

dc1f651


          Remove vestigial include

37c511a

bedevere-app Bot mentioned this pull request

Mar 22, 2024

Make weakref thread-safe without the GIL #111926

Closed

mpage added the skip news label

Mar 22, 2024

mpage requested a review from colesbury

March 22, 2024 22:13

mpage added 5 commits

March 22, 2024 16:45


          Make sure we set the cleared flag

ccaeded


          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.


          Don't leak ref if initialization fails

d998d1b


          Remove clinic critical section directives for _weakref module

37076b5

Locking is handled in the implementation


          Support async removal of dead weakrefs from WeakValueDictionary

a07d2db

colesbury mentioned this pull request

Mar 25, 2024

Disable GIL by default in free-threaded build #116749

Closed

mpage commented Mar 29, 2024

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mpage requested a review from colesbury

March 29, 2024 21:48

colesbury reviewed

Apr 1, 2024

View reviewed changes

colesbury left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mpage reacted with thumbs up emoji

All reactions

👍 1 reaction

colesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label

Apr 1, 2024

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label

Apr 1, 2024

mpage added 10 commits

April 2, 2024 09:20


          Remove redundant gc untrack

7a86e35


          Update comment

f89edd1


          Add missing braces

5a0b976


          Use Py_ssize_t for tracking number of items

9e67740


          Tighten up clearing basic refs/proxies

55a211f


          Simplify _PyStaticType_ClearWeakRefs

cdb9290


          Simplify get_or_create_weakref

c93c336


          Add _PyWeakref_IsDead

0366f01


          Remove TODO

3e135e4

We cannot assert now that we clear weakrefs without holding the
lock in _PyStaticType_ClearWeakRefs


          Fix formatting

dbf7c58

mpage requested a review from colesbury

April 2, 2024 19:44

colesbury approved these changes

Apr 3, 2024

View reviewed changes

colesbury left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting merge and removed awaiting review labels

Apr 3, 2024

erlend-aasland approved these changes

Apr 4, 2024

View reviewed changes

Comment thread

Python/pystate.c Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Update Python/pystate.c

73466bf

Co-authored-by: Erlend E. Aasland <[email protected]>

colesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label

Apr 8, 2024

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label

Apr 8, 2024

colesbury merged commit df73179 into python:main

Apr 8, 2024

bedevere-app Bot removed the awaiting merge label

Apr 8, 2024

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request

Apr 17, 2024


          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`).

hugovk mentioned this pull request

Apr 26, 2024

test_repl's test_no_memory: AssertionError: -6 not found in (1, 120) #118331

Closed

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Reviewers

colesbury

colesbury approved these changes

erlend-aasland

erlend-aasland approved these changes

ericsnowcurrently

Awaiting requested review from ericsnowcurrently

ericsnowcurrently is a code owner

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

berkerpeksag

Awaiting requested review from berkerpeksag

methane

Awaiting requested review from methane

methane is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

mpage commented Mar 22, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpage commented Mar 29, 2024

Uh oh!

colesbury left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Apr 1, 2024

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Apr 8, 2024

Uh oh!

hugovk commented Apr 26, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mpage commented Mar 22, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

colesbury left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.