-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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-121464: Make concurrent iteration over enumerate safe under free-threading #125734
Conversation
My expertise is with the Python-based |
|
||
|
||
class EnumerateThreading(unittest.TestCase): | ||
@staticmethod |
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 @staticmethod
seems to be removable.
cc @colesbury |
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.
Looks good overall. Two comments below.
Co-authored-by: Sam Gross <colesbury@gmail.com>
We make concurrent iteration with
enumerate
thread-safe (e.g. not crash, correct results are not guaranteed, see #124397) byUsing
_PyObject_IsUniquelyReferenced
for the re-use of the result tupleIn
enum_next
usingFT_ATOMIC_LOAD_SSIZE_RELAXED
/FT_ATOMIC_STORE_SSIZE_RELAXED
for loading and saving of the mutableen->en_index
In
enum_next_long
(andenum_reduce
) we have to deal withen->en_longindex
which is mutable. Since the objects inen_longindex
need to be properly refcounted this is a bit hard to handle. Two options:i) Use a lock. This is a simple approach and the performance cost might be acceptable since use cases of
enumerate
with numbers that do not fit in a long are rareii) The
en->en_longindex
can be atomically swapped withstepped_up
using_Py_atomic_exchange_ptr
. With this approachen->en_longindex
is either zero, or has a refcount of 1. The thread performing the swap ofen->en_longindex
can decrement the olden->en_longindex
. A branch with this approach is enumerate_ft_v3.In this PR we pick option i) because it is simpler and more robust.