-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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.
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
gh-112069: Add _PySet_NextEntryRef to be thread-safe. #117990
Conversation
OKay, it's too slow. |
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.
I'm not sure what the right approach here is, but as written _PySet_NextEntry
still returns a borrowed reference, so the cases that were not thread-safe are probably still not thread-safe.
I don't think it makes sense to try to make individual calls to _PySet_NextEntry
thread-safe. The whole loop of while(_PySet_NextEntry(...))
is generally what needs to be thread-safe. For example, the comment in set_next
says:
Lines 477 to 478 in fccedbd
* CAUTION: In general, it isn't safe to use set_next in a loop that | |
* mutates the table. |
I think we should add locking in the use sites of _PySet_NextEntry
as necessary. Most look like they're already okay to me: either they already do locking (listobject.c
and dictobject.c
) or operate on private or immutable data (pylifecycle.c
, codeobject.c
).
The two cases that concern me and may need locking are:
_pickle.c
marshal.c
buildbot/AMD64 RHEL7 Refleaks PR: Unrelated
|
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.
- We should not change the reference count behavior of
_PySet_NextEntry
as it's exposed publicly and used outside of CPython (like in Cython). If needed, add a new function_PySet_NextEntryRef
. - I don't think
_PyFrozenSet_NextEntry
is needed._PySet_NextEntry
(or_PySet_NextEntryRef
) looks sufficient.
53f949a
to
99fb7e6
Compare
Need to check. |
test_compile: #118023 So leaks are not related to this PR. |
set
thread-safe in--disable-gil
builds #112069