Skip to content

Conversation

@eendebakpt
Copy link
Contributor

@eendebakpteendebakpt commented Oct 19, 2024

We make concurrent iteration with enumerate thread-safe (e.g. not crash, correct results are not guaranteed, see #124397) by

  • Using _PyObject_IsUniquelyReferenced for the re-use of the result tuple

  • In enum_next using FT_ATOMIC_LOAD_SSIZE_RELAXED/FT_ATOMIC_STORE_SSIZE_RELAXED for loading and saving of the mutable en->en_index

  • In enum_next_long (and enum_reduce) we have to deal with en->en_longindex which is mutable. Since the objects in en_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 rare
    ii) The en->en_longindex can be atomically swapped with stepped_up using _Py_atomic_exchange_ptr. With this approach en->en_longindex is either zero, or has a refcount of 1. The thread performing the swap of en->en_longindex can decrement the old en->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.

@bedevere-appbedevere-appbot mentioned this pull request Oct 19, 2024
@eendebakpteendebakpt changed the title Draft: gh-120608: Make concurrent iteration over enumerate safe under free-threadingDraft: gh-121464: Make concurrent iteration over enumerate safe under free-threadingOct 19, 2024
@eendebakpteendebakpt changed the title Draft: gh-121464: Make concurrent iteration over enumerate safe under free-threadinggh-121464: Make concurrent iteration over enumerate safe under free-threadingOct 19, 2024
@ethanfurman
Copy link
Member

My expertise is with the Python-based Enum, not the C-based enumerate. Removing myself as a reviewer.

@ethanfurmanethanfurman removed their request for review October 20, 2024 19:05


classEnumerateThreading(unittest.TestCase):
@staticmethod
Copy link
Contributor

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.

@rruuaanng
Copy link
Contributor

cc @colesbury

Copy link
Contributor

@colesburycolesbury left a 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.

eendebakptand others added 2 commits January 15, 2025 20:27
@kumaraditya303kumaraditya303 merged commit ec46a55 into python:mainMar 13, 2025
48 checks passed
plashchynski pushed a commit to plashchynski/cpython that referenced this pull request Mar 17, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@eendebakpt@ethanfurman@rruuaanng@colesbury@kumaraditya303