Skip to content

gh-153062: Fix a crash iterating itertools.tee on the free-threaded build#153063

Merged
nascheme merged 6 commits into
python:mainfrom
tonghuaroot:tee-next-ft-crash
Jul 10, 2026
Merged

gh-153062: Fix a crash iterating itertools.tee on the free-threaded build#153063
nascheme merged 6 commits into
python:mainfrom
tonghuaroot:tee-next-ft-crash

Conversation

@tonghuaroot

@tonghuaroot tonghuaroot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Iterating an itertools.tee iterator concurrently crashes the free-threaded build. tee iterators are documented as not thread-safe, so concurrent use returning undefined results or raising RuntimeError is by design and is preserved here; this fixes only the memory-unsafety (corrupted reference counts and a segfault), which the free-threaded build must not permit. The other itertools iterators already lock their __next__; tee was the remaining one.

The shared teedataobject cells are locked while read, extended, or cleared. Each branch's position (dataobj and index) is snapshotted under the tee object's own critical section and revalidated before it is advanced, so the tee lock and the data-object lock are never nested: a nested critical section can be suspended under contention, which would break the atomicity the position fetch depends on (this is why Py_BEGIN_CRITICAL_SECTION2 on the two objects does not work here).

Tests in Lib/test/test_free_threading/ cover both a single branch consumed by many threads and sibling branches each consumed by their own thread; run_concurrently is barrier-synchronized so the threads actually contend. The re-entrancy guard's RuntimeError is tolerated, since it is a documented outcome. Single-threaded behavior is unchanged and test_itertools still passes.

Backport labels left to a maintainer.

…aded build

itertools.tee branches share a linked list of teedataobject cells.  On the free-threaded build, iterating one branch from multiple threads, or iterating sibling branches concurrently, raced on the shared cells and each branch's position, corrupting refcounts and crashing.

Lock each teedataobject while reading, extending, or clearing it, and snapshot each branch's position under the tee object's own lock, revalidating before advancing, so the two locks are never nested.  Concurrent iteration of one tee is undefined and may raise RuntimeError as documented, but no longer crashes.
Drop scratch files accidentally staged in the previous commit.

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't check the PR detail, but if you write whole code with AI, please consider that the approach is proper. Adding critical section is not a free.

The free-threading snapshot and revalidation add per-element overhead on the default build, where the GIL already serializes access. Guard that path under Py_GIL_DISABLED so the default build keeps the original iteration and the free-threaded path is unchanged.
@nascheme

nascheme commented Jul 9, 2026

Copy link
Copy Markdown
Member

I took a look at this. I think critical sections are a reasonable tool in this case. Using them here is similar to what other itertools iterators do. One gap, need to make tee_copy_impl safe as well:

--- a/Modules/itertoolsmodule.c
+++ b/Modules/itertoolsmodule.c
@@ -1028,8 +1028,10 @@ tee_copy_impl(teeobject *to)
     if (newto == NULL) {
         return NULL;
     }
+    Py_BEGIN_CRITICAL_SECTION(to);
     newto->dataobj = (teedataobject *)Py_NewRef(to->dataobj);
     newto->index = to->index;
+    Py_END_CRITICAL_SECTION();
     newto->weakreflist = NULL;
     newto->state = to->state;
     PyObject_GC_Track(newto);

I think after that change, this PR would be okay to merge.

@tonghuaroot

Copy link
Copy Markdown
Contributor Author

Applied in 3dab9e0, thanks. test_itertools and the tee race test pass on a free-threaded build.

@nascheme nascheme merged commit 6c389c4 into python:main Jul 10, 2026
54 checks passed
@nascheme nascheme added type-crash A hard crash of the interpreter, possibly with a core dump topic-free-threading needs backport to 3.14 bugs and security fixes needs backport to 3.15 pre-release feature fixes, bugs and security fixes labels Jul 10, 2026
@miss-islington-app

Copy link
Copy Markdown

Thanks @tonghuaroot for the PR, and @nascheme for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

@miss-islington-app

Copy link
Copy Markdown

Thanks @tonghuaroot for the PR, and @nascheme for merging it 🌮🎉.. I'm working now to backport this PR to: 3.15.
🐍🍒⛏🤖

@miss-islington-app

Copy link
Copy Markdown

Sorry, @tonghuaroot and @nascheme, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 6c389c446e487bcb9d09e85de30aa42052990f1c 3.14

@bedevere-app

bedevere-app Bot commented Jul 10, 2026

Copy link
Copy Markdown

GH-153475 is a backport of this pull request to the 3.15 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.15 pre-release feature fixes, bugs and security fixes label Jul 10, 2026
@bedevere-app

bedevere-app Bot commented Jul 10, 2026

Copy link
Copy Markdown

GH-153476 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic-free-threading type-crash A hard crash of the interpreter, possibly with a core dump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants