Fix race condition with py::make_key_iterator in free threading by colesbury · Pull Request #5971 · pybind/pybind11

colesbury · 2026-01-21T21:46:46Z

Description

The creation of the iterator class needs to be synchronized.

Fixes #5970

Suggested changelog entry:

Fix race condition in py::make_key_iterator with free threaded Python

The creation of the iterator class needs to be synchronized.

colesbury · 2026-01-21T21:53:50Z

include/pybind11/detail/internals.h

 struct internals {
 #ifdef Py_GIL_DISABLED
-    pymutex mutex;
+    pyrecursive_mutex mutex;


@henryiii @rwgk - I could use some advice here. I probably should have used some sort of recursive mutex here from the start -- it's pretty difficult to do the locking for make_iterator_impl without it.

I think changing this would require bumping PYBIND11_INTERNALS_VERSION, at least for Py_GIL_DISABLED builds. Is that acceptable?

Alternatively, maybe we should use something like Py_BEGIN_CRITICAL_SECTION_MUTEX, which supports recursion and eliminates some potential lock ordering deadlocks if you call into Python. The downside is that it is 3.14+ only.

I think changing this would require bumping PYBIND11_INTERNALS_VERSION, at least for Py_GIL_DISABLED builds. Is that acceptable?

After the v3.0.2 release, yes. We already have three other PRs that need an internals version bump.

I was planning to release today (PR #5969), but there was a show stopper. We're waiting for a fix for the segfault.

My thinking: the race isn't new, therefore it would seem reasonable to defer fixing it until after the v3.0.2 release, when we have a window of opportunity to bump the internals version.

Caveat: I don't have enough background to decide between the internals version bump and the critical section alternative.

Py_BEGIN_CRITICAL_SECTION_MUTEX sounds fine, I'd be okay to deprecate and remove Python 3.13t support. Maybe we could just fix this bug on 3.14t, and then drop 3.13t in 3.1?

We are thinking about dropping 3.13t in cibuildwheel too.

Also fine to make the lock recursive in 3.1, we have several ABI bumps coming up.

Great, I think dropping 3.13t makes sense. I switched to PyCriticalSection_BeginMutex.

I saw there's also a py::scoped_critical_section. Not sure if you'd want to combine them.

Could we have a guard with #if defined(Py_GIL_DISABLED) && defined(PY_VERSION_HEX) && PY_VERSION_HEX >= ... here? It is causing a compilation error with Python 3.13t. In addition, the function PyCriticalSection_BeginMutex was added in Python 3.14.0rc1 and Python 3.15.0a1.

gh-133296: Publicly expose critical section API that accepts PyMutex python/cpython#135899

It is causing a compilation error with Python 3.13t.

Ah, looks like we were too efficient dropping 3.13t. We should keep it working at the level before this PR for the v3.0.2 release, then fully drop 3.13t support after the release. @XuehaiPan could you help starting a PR for that? Just the code changes, to be sure it works for you. I can help adding back one 3.13t job.

I opened #5981

ikrommyd · 2026-01-21T22:13:59Z

Thanks for working on this so quickly! I can try it with the full library that I discovered the issue with once it is in a ready-to-review state 😃

colesbury · 2026-01-22T17:48:56Z

@ikrommyd - if you can run test with the full library, that would be great.

ikrommyd · 2026-01-23T06:15:49Z

Looks good from my side! (if I checkout pybind11 master branch, tests/test_core.py fails with the original error reported in the issue TypeError: Object of type 'iterator' is not an instance of 'iterator')

henryiii · 2026-01-23T15:06:08Z

@rwgk what should we do with 3.13t here? Okay to drop it in a patch release, or should we gate this for 3.14t+ only (fine to leave the bug unfixed on 3.13t), and drop in the next minor release instead? CPython 3.13t was always experimental, while 3.14t is not.

rwgk · 2026-01-24T18:07:23Z

Okay to drop it in a patch release

Move vote: yes, drop

I don't think 3.13t is worth the trouble doing anything special.

rwgk · 2026-01-24T18:10:49Z

include/pybind11/detail/internals.h

    void unlock() { PyMutex_Unlock(&mutex); }
 };

+class pycritical_section {


Do we need to be careful about copy/move here?

... give me a moment to look at this with the help of some LLM ...

Result: be01d1e

(created with Cursor Composer 1 model)

The pycritical_section class is a RAII wrapper that manages a Python critical section lifecycle: - Acquires the critical section in the constructor via PyCriticalSection_BeginMutex - Releases it in the destructor via PyCriticalSection_End - Holds a reference to a pymutex Allowing copy or move operations would be dangerous: 1. Copy: Both the original and copied objects would call PyCriticalSection_End on the same PyCriticalSection object in their destructors, leading to double-unlock and undefined behavior. 2. Move: The moved-from object's destructor would still run and attempt to end the critical section, while the moved-to object would also try to end it, again causing double-unlock. This follows the same pattern used by other RAII lock guards in the codebase, such as gil_scoped_acquire and gil_scoped_release, which also explicitly delete copy/move operations to prevent similar issues. By explicitly deleting these operations, we prevent accidental misuse and ensure the critical section is properly managed by a single RAII object throughout its lifetime.

Python 3.13t was experimental, while Python 3.14t is not. This PR uses PyCriticalSection_BeginMutex which is only available in Python 3.14+, making Python 3.13t incompatible with the changes. Removed all Python 3.13t CI jobs: - ubuntu-latest, 3.13t (standard-large matrix) - macos-15-intel, 3.13t (standard-large matrix) - windows-latest, 3.13t (standard-large matrix) - manylinux job testing 3.13t This aligns with the decision to drop Python 3.13t support as discussed in PR pybind#5971.

After removing Python 3.13t support (incompatible with PyCriticalSection_BeginMutex which requires Python 3.14+), we're adding replacement jobs using Python 3.13 (default) to maintain test coverage in key dimensions: 1. ubuntu-latest, Python 3.13: C++20 + DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION - Replaces: ubuntu-latest, 3.13t with same config - Maintains coverage for this specific configuration combination 2. macos-15-intel, Python 3.13: C++11 - Replaces: macos-15-intel, 3.13t with same config - Maintains macOS coverage for Python 3.13 3. manylinux (musllinux), Python 3.13: GIL testing - Replaces: manylinux, 3.13t job - Maintains manylinux/musllinux container testing coverage These additions are proposed to get feedback on which jobs should be kept to maintain appropriate test coverage without the experimental 3.13t builds.

rwgk · 2026-01-24T18:39:42Z

@rwgk what should we do with 3.13t here? Okay to drop it in a patch release, or should we gate this for 3.14t+ only (fine to leave the bug unfixed on 3.13t), and drop in the next minor release instead? CPython 3.13t was always experimental, while 3.14t is not.

While I was at it, I went ahead and added two commits:

I'm not sure about the second commit: @henryiii which of these jobs would you add?

rwgk · 2026-01-24T19:34:58Z

This job failed:

https://github.com/pybind/pybind11/actions/runs/21319724363/job/61367809291

Run Build Command(s): /opt/homebrew/bin/ninja -v cpptest
[0/2] /opt/homebrew/bin/cmake -P /Users/runner/work/pybind11/pybind11/build/CMakeFiles/VerifyGlobs.cmake
[0/2] cd /Users/runner/work/pybind11/pybind11/build/tests/test_with_catch && /Users/runner/work/pybind11/pybind11/build/tests/test_with_catch/test_with_catch
[ STARTING ] 2026-01-24 19:01:13.460Z
[ PYTHON   ] 3.14.2 free-threading build (v3.14.2:df793163d58, Dec  5 2025, 12:31:15) [Clang 16.0.0 (clang-1600.0.26.6)]
...
[ RUN      ] Per-Subinterpreter GIL
<frozen importlib._bootstrap>:491: RuntimeWarning: The global interpreter lock (GIL) has been enabled to load module 'widget_module', which has not declared that it can run safely without the GIL. To override this behavior and keep the GIL disabled (at your own risk), run with PYTHON_GIL=0 or -Xgil=0.
Error: The action 'C++ tests' has timed out after 3 minutes.

I triggered a rerun to see if it's a flake.

rwgk · 2026-01-24T19:47:33Z

This job failed:

https://github.com/pybind/pybind11/actions/runs/21319724363/job/61367809291
...
I triggered a rerun to see if it's a flake.

It was a flake, but maybe that's a problem for another day? Or do you suspect that the changes in this PR could play a role?

colesbury · 2026-01-26T17:29:52Z

I'm not sure what caused that timeout, but I don't think it's related to the PR. Mixing subinterpreters and free threading isn't very well tested currently.

.github/workflows/ci.yml

colesbury · 2026-01-26T18:20:35Z

I think #5973 should fix the occasional deadlock @rwgk saw

rwgk · 2026-02-01T16:18:41Z

The failure below is reproducible (not a flake) and almost certainly related to commit 91189c9 here.

For comparison, running the CI for current master comes back all green.

I also verified that #5972 does not fix it (@b-pass @XuehaiPan for visibility), but there is a second failure of the same kind:

CI / 🐍 (ubuntu-latest, 3.14t, -DCMAKE_CXX_STANDARD=20 -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_I... / 🧪 (pull_request)Failing after 5m
CI / Manylinux on 🐍 3.14t (pull_request)Failing after 6m

I'll undo 91189c9 in hopes that it'll allow us to get this PR merged.

CI / Manylinux on 🐍 3.14t (pull_request)Failing after 6m

=================================== FAILURES ===================================
__________________ test_import_in_subinterpreter_concurrently __________________

    @pytest.mark.skipif(
        sys.platform.startswith("emscripten"), reason="Requires loadable modules"
    )
    @pytest.mark.skipif(not CONCURRENT_INTERPRETERS_SUPPORT, reason="Requires 3.14.0b3+")
    def test_import_in_subinterpreter_concurrently():
        """Tests that importing a module in multiple subinterpreters concurrently works correctly"""
>       check_script_success_in_subprocess(
            PREAMBLE_CODE
            + textwrap.dedent(
                """
                import gc
                from concurrent.futures import InterpreterPoolExecutor, as_completed
    
                futures = future = None
                with InterpreterPoolExecutor(max_workers=16) as executor:
                    futures = [executor.submit(test) for _ in range(32)]
                    for future in as_completed(futures):
                        future.result()
                del futures, future, executor
    
                for _ in range(5):
                    gc.collect()
                """
            )
        )


../../tests/test_multiple_interpreters.py:437: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

code = "def test():\n    import sys\n\n    sys.path.insert(0, '/__w/pybind11/pybind11/build/tests')\n\n    import collections...as_completed(futures):\n        future.result()\ndel futures, future, executor\n\nfor _ in range(5):\n    gc.collect()"

    def check_script_success_in_subprocess(code: str, *, rerun: int = 8) -> None:
        """Runs the given code in a subprocess."""
E               # The expected objects are dynamically imported from `collections`.
E               assert objects[-2:] == expected[-2:], (
E                   f"Expected heap objects {expected[-2:]!r}, got {objects[-2:]!r}."
E               )
E           
E               assert hasattr(m, 'MyClass'), "Module missing MyClass"
E               assert hasattr(m, 'MyGlobalError'), "Module missing MyGlobalError"
E               assert hasattr(m, 'MyLocalError'), "Module missing MyLocalError"
E               assert hasattr(m, 'MyEnum'), "Module missing MyEnum"
E           
E           import gc
E           from concurrent.futures import InterpreterPoolExecutor, as_completed
E           
E           futures = future = None
E           with InterpreterPoolExecutor(max_workers=16) as executor:
E               futures = [executor.submit(test) for _ in range(32)]
E               for future in as_completed(futures):
E                   future.result()
E           del futures, future, executor
E           
E           for _ in range(5):
E               gc.collect()
E           ```
E           
E           Output:
E           Exception: Traceback (most recent call last):
E             File "<frozen importlib._bootstrap>", line 1371, in _find_and_load
E             File "<frozen importlib._bootstrap>", line 1333, in _find_and_load_unlocked
E             File "<frozen importlib._bootstrap>", line 1267, in _find_spec
E             File "<frozen importlib._bootstrap_external>", line 1292, in find_spec
E             File "<frozen importlib._bootstrap_external>", line 1266, in _get_spec
E             File "<frozen importlib._bootstrap_external>", line 1369, in find_spec
E             File "<frozen importlib._bootstrap_external>", line 1412, in _fill_cache
E           BlockingIOError: [Errno 11] Resource temporarily unavailable: '/opt/python/cp314-cp314t/lib/python3.14t/concurrent'
E           
E           The above exception was the direct cause of the following exception:
E           
E           concurrent.interpreters.NotShareableError: object could not be unpickled
E           
E           The above exception was the direct cause of the following exception:
E           
E           Traceback (most recent call last):
E             File "<string>", line 45, in <module>
E               future.result()
E               ~~~~~~~~~~~~~^^
E             File "/opt/python/cp314-cp314t/lib/python3.14t/concurrent/futures/_base.py", line 443, in result
E               return self.__get_result()
E                      ~~~~~~~~~~~~~~~~~^^
E             File "/opt/python/cp314-cp314t/lib/python3.14t/concurrent/futures/_base.py", line 395, in __get_result
E               raise self._exception
E             File "/opt/python/cp314-cp314t/lib/python3.14t/concurrent/futures/thread.py", line 86, in run
E               result = ctx.run(self.task)
E             File "/opt/python/cp314-cp314t/lib/python3.14t/concurrent/futures/interpreter.py", line 84, in run
E               return self.interp.call(do_call, self.results, *task)
E                      ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E             File "/opt/python/cp314-cp314t/lib/python3.14t/concurrent/interpreters/__init__.py", line 238, in call
E               return self._call(callable, args, kwargs)
E                      ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^
E             File "/opt/python/cp314-cp314t/lib/python3.14t/concurrent/interpreters/__init__.py", line 220, in _call
E               res, excinfo = _interpreters.call(self._id, callable, args, kwargs, restrict=True)
E                              ~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E           concurrent.interpreters.NotShareableError: func not shareable

_          = 1
code       = "def test():\n    import sys\n\n    sys.path.insert(0, '/__w/pybind11/pybind11/build/tests')\n\n    import collections...as_completed(futures):\n        future.result()\ndel futures, future, executor\n\nfor _ in range(5):\n    gc.collect()"
rerun      = 8

../../tests/test_multiple_interpreters.py:284: RuntimeError
=========================== short test summary info ============================

This reverts commit 91189c9. Reason: pybind#5971 (comment)

Skylion007 · 2026-02-01T17:38:50Z

include/pybind11/detail/internals.h

 };

+class pycritical_section {
+    pymutex &mutex;


Okay, interesting to me the RAII doesn't own the mutex and the ctor takes a mutable reference.

This looks correct to me, based on this Cursor-generated analysis:

This is the standard lock-guard pattern: the RAII object owns the critical-section lifetime, not the mutex. It takes a
non-const ref because locking mutates the mutex (and PyCriticalSection_BeginMutex needs a mutable `PyMutex). The mutex itself is owned by the internals and outlives the guard:

pybind11/include/pybind11/detail/internals.h

Lines 230 to 239 in 0080cae

#ifdef Py_GIL_DISABLED

// Wrapper around PyMutex to provide BasicLockable semantics

class pymutex {

PyMutex mutex;

public:

pymutex() : mutex({}) {}

void lock() { PyMutex_Lock(&mutex); }

void unlock() { PyMutex_Unlock(&mutex); }

};

This reverts commit f3197de. After pybind#5972 is/was merged, tests should pass (already tested under pybind#5980). See also pybind#5972 (comment)

…afety This exact state was already tested under pybind#5980 CI - https://github.com/pybind/pybind11/actions/runs/21575190538?pr=5980 CIBW - https://github.com/pybind/pybind11/actions/runs/21575190471?pr=5980

rwgk · 2026-02-02T06:02:44Z

Not rerunning the CI, because the exact same state was tested under #5980 already.

colesbury and others added 2 commits January 21, 2026 16:46

Fix race condition with py::make_key_iterator in free threading

bfe55ed

The creation of the iterator class needs to be synchronized.

style: pre-commit fixes

fa03953

colesbury commented Jan 21, 2026

View reviewed changes

colesbury and others added 2 commits January 22, 2026 12:42

Use PyCriticalSection_BeginMutex instead of recursive mutex

70c2ffc

style: pre-commit fixes

8690dd7

colesbury marked this pull request as ready for review January 22, 2026 17:48

ikrommyd mentioned this pull request Jan 23, 2026

build: add freethreaded python ci and wheels cms-nanoAOD/correctionlib#284

Open

3 tasks

rwgk reviewed Jan 24, 2026

View reviewed changes

rwgk added 3 commits January 24, 2026 10:23

henryiii reviewed Jan 26, 2026

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

henryiii reviewed Jan 26, 2026

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

henryiii reviewed Jan 26, 2026

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

ci: run in free-threading mode a bit more on 3.14

91189c9

rwgk and others added 2 commits January 29, 2026 14:05

Merge branch 'master' into iterator-nogil-threadsafety

603b25a

Merge branch 'master' into colesbury→iterator-nogil-threadsafety

dc8e35c

rwgk mentioned this pull request Feb 1, 2026

For testing only: Combined #5971 and #5972 #5979

Closed

Revert "ci: run in free-threading mode a bit more on 3.14"

f3197de

This reverts commit 91189c9. Reason: pybind#5971 (comment)

Skylion007 reviewed Feb 1, 2026

View reviewed changes

rwgk approved these changes Feb 1, 2026

View reviewed changes

rwgk mentioned this pull request Feb 2, 2026

Revert internals destruction and add test for internals recreation #5972

Merged

rwgk added 2 commits February 1, 2026 21:55

Reapply "ci: run in free-threading mode a bit more on 3.14"

07e0360

This reverts commit f3197de. After pybind#5972 is/was merged, tests should pass (already tested under pybind#5980). See also pybind#5972 (comment)

[skip ci] Merge branch 'master' into colesbury→iterator-nogil-threads…

5505b4d

…afety This exact state was already tested under pybind#5980 CI - https://github.com/pybind/pybind11/actions/runs/21575190538?pr=5980 CIBW - https://github.com/pybind/pybind11/actions/runs/21575190471?pr=5980

rwgk merged commit 4d7d02a into pybind:master Feb 2, 2026
3 checks passed

github-actions bot added the needs changelog Possibly needs a changelog entry label Feb 2, 2026

rwgk mentioned this pull request Feb 2, 2026

For testing only: Combined #5971 and #5972 plus 91189c9 (i.e. "more 3.14t") #5980

Open

XuehaiPan mentioned this pull request Feb 2, 2026

Add fallback implementation of PyCriticalSection_BeginMutex for Python 3.13t #5981

Open

	#ifdef Py_GIL_DISABLED
	// Wrapper around PyMutex to provide BasicLockable semantics
	class pymutex {
	PyMutex mutex;

	public:
	pymutex() : mutex({}) {}
	void lock() { PyMutex_Lock(&mutex); }
	void unlock() { PyMutex_Unlock(&mutex); }
	};

Conversation

colesbury commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Suggested changelog entry:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikrommyd commented Jan 21, 2026

Uh oh!

colesbury commented Jan 22, 2026

Uh oh!

ikrommyd commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henryiii commented Jan 23, 2026

Uh oh!

rwgk commented Jan 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwgk commented Jan 24, 2026

Uh oh!

rwgk commented Jan 24, 2026

Uh oh!

rwgk commented Jan 24, 2026

Uh oh!

colesbury commented Jan 26, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

colesbury commented Jan 26, 2026

Uh oh!

rwgk commented Feb 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwgk commented Feb 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

colesbury commented Jan 21, 2026 •

edited

Loading

ikrommyd commented Jan 23, 2026 •

edited

Loading