-
-
Notifications
You must be signed in to change notification settings - Fork 34.1k
gh-144356: Make set iterator __length_hint__ and iternext race-safe under no-gil
#144357
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
base: main
Are you sure you want to change the base?
Conversation
setiter_len() was reading so->used without atomic access while concurrent mutations update it atomically under Py_GIL_DISABLED. Use an atomic load for so->used to avoid a data race. This preserves the existing semantics of __length_hint__ while making the access thread-safe. Signed-off-by: Yongtao Huang <yongtaoh2022@gmail.com>
setiter_len() under no-gilset_iterator.__length_hint__ under no-gil
|
Thanks for the review. I’ve decided to address both issues in this PR. I also added a corresponding test case for the issue you pointed out. |
eendebakpt
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 left some more review comments. I think we can get this right, but a simpler approach here would be to put a critical section on the set iterator itself.
Thanks for the suggestion. I simplified the implementation by protecting both the iterator and the set with It looks like there is an unrelated flaky test case. |
eendebakpt
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.
@hyongtao-code Thanks for your work on this! The core idea of the PR has shifted a bit, which is why I have some more review comments, but we are getting closer I believe.
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
This reverts commit 78241a8.
set_iterator.__length_hint__ under no-gil__length_hint__ and iternext race-safe under no-gil
| threading_helper.run_concurrently(worker, nthreads=NUM_THREADS) | ||
|
|
||
| @threading_helper.reap_threads | ||
| def test_iternext_concurrent_exhaust_race(self): |
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.
The test_iternext_concurrent_exhaust_race and test_length_hint_exhaust_race test slightly difference things, but I think they can be combined (to reduce pressure on the CI)
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.
something like this work?
def worker():
n = 0
while True:
if n & 1:
it.__length_hint__()
try:
next(it)
except StopIteration:
break
n += 1
Long log:
In free-threaded builds, setiter_len() and setiter_iternext() could race with concurrent set mutation and iterator exhaustion.
We now serialize operations on the iterator itself (and keep the set stable) using a critical section, and ensure the iterator’s exhaustion state is handled consistently.
The non-free-threaded build keeps the existing semantics. Add free-threading-style concurrency tests (similar to the itertools FT tests) to exercise
__length_hint__anditernextexhaustion under concurrent execution.setiter_len()was readingso->usedwithout atomic access while concurrentmutations update it atomically under Py_GIL_DISABLED.
In free-threaded builds,setiter_len()could race with concurrent setmutation and iterator exhaustion.
Use an atomic load forso->usedto avoid a data race. This preserves theexisting semantics of
__length_hint__while making the access thread-safe.Signed-off-by: Yongtao Huang yongtaoh2022@gmail.com