From 4d7d02a8e528f3d3ba14d9b10b813557b2e9d0e2 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 2 Feb 2026 01:02:50 -0500 Subject: [PATCH] Fix race condition with py::make_key_iterator in free threading (#5971) * Fix race condition with py::make_key_iterator in free threading The creation of the iterator class needs to be synchronized. * style: pre-commit fixes * Use PyCriticalSection_BeginMutex instead of recursive mutex * style: pre-commit fixes * Make pycritical_section non-copyable and non-movable 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. * Drop Python 3.13t support from CI 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 #5971. * Add Python 3.13 (default) replacement jobs for removed 3.13t jobs 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. * ci: run in free-threading mode a bit more on 3.14 * Revert "ci: run in free-threading mode a bit more on 3.14" This reverts commit 91189c9242e787922d26ca467710dcc494871b82. Reason: https://github.com/pybind/pybind11/pull/5971#issuecomment-3831321903 * Reapply "ci: run in free-threading mode a bit more on 3.14" This reverts commit f3197de97557a86a9a73df7890be1c227a7c4c59. After #5972 is/was merged, tests should pass (already tested under #5980). See also https://github.com/pybind/pybind11/pull/5972#discussion_r2752674989 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ralf W. Grosse-Kunstleve Co-authored-by: Henry Schreiner Co-authored-by: Ralf W. Grosse-Kunstleve --- .github/workflows/ci.yml | 15 ++++++--------- include/pybind11/detail/internals.h | 22 ++++++++++++++++++++-- include/pybind11/pybind11.h | 1 + 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cda2c214d..a0965fa81 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -84,7 +84,7 @@ jobs: python-version: '3.12' cmake-args: -DPYBIND11_TEST_SMART_HOLDER=ON -DPYBIND11_SIMPLE_GIL_MANAGEMENT=ON - runs-on: ubuntu-latest - python-version: '3.13t' + python-version: '3.14t' cmake-args: -DCMAKE_CXX_STANDARD=20 -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION=ON - runs-on: ubuntu-latest python-version: '3.14' @@ -102,12 +102,12 @@ jobs: - runs-on: macos-15-intel python-version: '3.11' cmake-args: -DPYBIND11_TEST_SMART_HOLDER=ON + - runs-on: macos-15-intel + python-version: '3.13' + cmake-args: -DCMAKE_CXX_STANDARD=11 - runs-on: macos-latest python-version: '3.12' cmake-args: -DCMAKE_CXX_STANDARD=17 -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION=ON - - runs-on: macos-15-intel - python-version: '3.13t' - cmake-args: -DCMAKE_CXX_STANDARD=11 - runs-on: macos-latest python-version: '3.14t' cmake-args: -DCMAKE_CXX_STANDARD=20 @@ -138,9 +138,6 @@ jobs: - runs-on: windows-2022 python-version: '3.13' cmake-args: -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebugDLL - - runs-on: windows-latest - python-version: '3.13t' - cmake-args: -DCMAKE_CXX_STANDARD=17 - runs-on: windows-latest python-version: '3.14' cmake-args: -DCMAKE_CXX_STANDARD=20 @@ -240,7 +237,7 @@ jobs: manylinux: - name: Manylinux on 🐍 3.13t • GIL + name: Manylinux on 🐍 3.14t if: github.event.pull_request.draft == false runs-on: ubuntu-latest timeout-minutes: 40 @@ -257,7 +254,7 @@ jobs: run: uv tool install ninja - name: Configure via preset - run: cmake --preset venv -DPYBIND11_CREATE_WITH_UV=python3.13t + run: cmake --preset venv -DPYBIND11_CREATE_WITH_UV=python3.14t - name: Build C++11 run: cmake --build --preset venv diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 7cfa5da92..b9b0f08f2 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -230,6 +230,7 @@ using instance_map = std::unordered_multimap; #ifdef Py_GIL_DISABLED // Wrapper around PyMutex to provide BasicLockable semantics class pymutex { + friend class pycritical_section; PyMutex mutex; public: @@ -238,6 +239,23 @@ public: void unlock() { PyMutex_Unlock(&mutex); } }; +class pycritical_section { + pymutex &mutex; + PyCriticalSection cs; + +public: + explicit pycritical_section(pymutex &m) : mutex(m) { + PyCriticalSection_BeginMutex(&cs, &mutex.mutex); + } + ~pycritical_section() { PyCriticalSection_End(&cs); } + + // Non-copyable and non-movable to prevent double-unlock + pycritical_section(const pycritical_section &) = delete; + pycritical_section &operator=(const pycritical_section &) = delete; + pycritical_section(pycritical_section &&) = delete; + pycritical_section &operator=(pycritical_section &&) = delete; +}; + // Instance map shards are used to reduce mutex contention in free-threaded Python. struct instance_map_shard { instance_map registered_instances; @@ -905,7 +923,7 @@ inline local_internals &get_local_internals() { } #ifdef Py_GIL_DISABLED -# define PYBIND11_LOCK_INTERNALS(internals) std::unique_lock lock((internals).mutex) +# define PYBIND11_LOCK_INTERNALS(internals) pycritical_section lock((internals).mutex) #else # define PYBIND11_LOCK_INTERNALS(internals) #endif @@ -934,7 +952,7 @@ inline auto with_exception_translators(const F &cb) get_local_internals().registered_exception_translators)) { auto &internals = get_internals(); #ifdef Py_GIL_DISABLED - std::unique_lock lock((internals).exception_translator_mutex); + pycritical_section lock((internals).exception_translator_mutex); #endif auto &local_internals = get_local_internals(); return cb(internals.registered_exception_translators, diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 02d2e72c2..f88fc2027 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -3173,6 +3173,7 @@ iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&...extra) { using state = detail::iterator_state; // TODO: state captures only the types of Extra, not the values + PYBIND11_LOCK_INTERNALS(get_internals()); if (!detail::get_type_info(typeid(state), false)) { class_(handle(), "iterator", pybind11::module_local()) .def(