Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-112069: Add _PySet_NextEntryRef to be thread-safe.#117990
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
corona10 commented Apr 17, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
corona10 commented Apr 17, 2024
OKay, it's too slow. |
colesbury left a comment
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.cmarshal.c
bedevere-bot commented Apr 17, 2024
corona10 commented Apr 17, 2024
buildbot/AMD64 RHEL7 Refleaks PR: Unrelated |
colesbury left a comment
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_NextEntryas 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_NextEntryis needed._PySet_NextEntry(or_PySet_NextEntryRef) looks sufficient.
bedevere-bot commented Apr 18, 2024
bedevere-bot commented Apr 18, 2024
53f949a to 99fb7e6Comparebedevere-bot commented Apr 18, 2024
corona10 commented Apr 18, 2024
Need to check. |
corona10 commented Apr 18, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
test_compile: #118023 So leaks are not related to this PR. |
setthread-safe in--disable-gilbuilds #112069