Skip to content

gh-160573: Drop the set once when a shared iterator is exhausted - #160619

Open
scrieve wants to merge 1 commit into
python:mainfrom
scrieve:gh-160573-setiter-release
Open

scrieve wants to merge 1 commit into
python:mainfrom
scrieve:gh-160573-setiter-release

Conversation

@scrieve

@scrieve scrieve commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

A set iterator shared between threads over-releases its set when it is exhausted.

setiter_iternext() reads si->si_set with no lock held, returns early only when it is already NULL, and clears the field plus Py_DECREF(so) after Py_END_CRITICAL_SECTION(). Threads that arrive on the exhausted branch together all pass the NULL check, so each of them drops the iterator's single reference.

Measured on a free-threaded debug build (--disable-gil --with-assertions, gcc 15.2), with eight threads exhausting one iterator over a 300,000-slot set:

  • before: sys.getrefcount(s) goes 1002 to 995, seven extra decrefs, then validate_gc_objects() aborts at finalization with "refcount is too small", exit 134.
  • with this branch: 1002 to 1002, exit 0.

ThreadSanitizer reported the race on the unpatched tree and is clean with the change, on the same reproducer.

The change holds the iterator's own critical section across the size check, the table walk, the clear, si_pos, si_used and len, and stores NULL with FT_ATOMIC_STORE_PTR_RELAXED, which is what the lock-free fast path reads. The iterator's reference is dropped once, after the lock has been released. On a build with the GIL the critical section compiles away, so behaviour there is unchanged.

I kept the table walk inside the set's critical section as it is rather than moving to the _Py_XGetRef plus exchange shape discussed in gh-154130, because gh-124397 asks for the minimal change that stops the crash and this keeps the single release easy to see. I am happy to rework it to the other shape if that is the direction you want for the iterator family.

New test in Lib/test/test_free_threading/test_set.py: TestSetIteratorExhaustion. It aborts with _Py_NegativeRefcount on unpatched main and passes here. test_set (644 tests) and test_free_threading.test_set (8 tests) are green on the free-threaded debug build.

NEWS entry added.

setiter_iternext() read si->si_set, checked it for NULL, walked the table
under the set's critical section and then cleared si->si_set and decrefed
the set after Py_END_CRITICAL_SECTION(). Threads that reach the exhausted
branch together all pass the NULL check, so every one of them drops the
iterator's single reference and the set is over-released.

On a free-threaded debug build the damage shows up at finalization as
"refcount is too small" in validate_gc_objects(); when the set is still
alive it shows up earlier as a negative refcount the next time
sys.getrefcount() reads it.

Hold the iterator's own critical section across the size check, the walk,
the clear and the counter updates, and store NULL with the same atomic
store the lock-free fast path uses for its read. The iterator's reference
is then dropped exactly once, after the lock has been released.

Add a regression test in test_free_threading: eight threads race to
exhaust one iterator over a set with a large dummy tail, and the set's
refcount must stay unchanged. Without the fix it aborts with
_Py_NegativeRefcount.
@nascheme

nascheme commented Nov 4, 2026

Copy link
Copy Markdown
Member

Thanks for the analysis and the reproducer. Unfortunately I don't think this fully fixes the race, and I'd prefer the fix used for dict iterators in gh-160312.

A problem is nested critical section can be suspended: Py_BEGIN_CRITICAL_SECTION(so) is nested inside the iterator's critical section. If the set's mutex is contended, PyMutex_Lock() parks the thread, and detaching suspends the outer (iterator) critical section. So the iterator lock does not actually serialize the whole operation:

  1. T1 holds the iterator lock, reads so = si->si_set (non-NULL), and blocks on the set lock. The iterator lock is released.
  2. T2 takes the iterator lock, reads the same non-NULL so, and blocks on the set lock.
  3. T1 resumes, walks off the end, stores NULL to si_set, and Py_DECREF(so).
  4. T2 resumes with its stale local so, walks from si_pos (now past mask), gets key == NULL, sets released = 1, and decrefs again. If T1's decref freed the set, T2 also touches freed memory.

The set lock is contended whenever another thread mutates or iterates the set, which the new test doesn't do. Re-checking si->si_set after taking the inner lock would close this specific window, but see below.

Other unlocked readers still read si->si_set without a lock and dereference / Py_XINCREF it. With this design they can race with the thread that clears and releases it.

Suggested approach (same as gh-160312 for dict): on the free-threaded build, don't clear si_set and drop the reference when the iterator is exhausted:

  • Keep the iterator's strong reference to the set until setiter_dealloc(). This matches list and tuple iterators, and dict iterators after gh-154130: Fix double decref of the dict in shared dict iterators #160312.
  • Mark exhaustion with a sentinel position, e.g. store si_pos = -1 (atomic relaxed store) and len = 0. Check it at the top of setiter_iternext() and before walking the table under the set's critical section.
  • Keep the existing si_set = NULL; Py_DECREF(so) behaviour on the default (GIL) build, e.g. via a small helper with #ifdef Py_GIL_DISABLED.
  • setiter_len() should return 0 when exhausted. setiter_reduce() then needs no special handling because si_set is never freed under it. With this there is no "release exactly once" problem to solve, and the iterator's own critical section shouldn't be needed. The set's critical section around the table walk, as it is today, is enough.

Test: add a case where the set lock is contended, e.g. a thread that keeps mutating or iterating the same set while others exhaust a shared iterator. Something along the lines of the dict test in gh-160312 (threading_helper.run_concurrently, repeatedly creating small iterators) is cheaper than the 300k-entry set and covers more interleavings.

Note: this review as assisted by an LLM.

@nascheme nascheme added type-bug An unexpected behavior, bug, or error topic-free-threading labels Nov 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review topic-free-threading type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants