Skip to content

Fix race condition with py::make_key_iterator in free threading #5971

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
colesbury wants to merge 11 commits into
base: master
Choose a base branch
from
+27 −11
Open

Fix race condition with py::make_key_iterator in free threading #5971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bfe55ed
Fix race condition with py::make_key_iterator in free threading
colesbury Jan 21, 2026
fa03953
style: pre-commit fixes
pre-commit-ci[bot] Jan 21, 2026
70c2ffc
Use PyCriticalSection_BeginMutex instead of recursive mutex
colesbury Jan 22, 2026
8690dd7
style: pre-commit fixes
pre-commit-ci[bot] Jan 22, 2026
be01d1e
Make pycritical_section non-copyable and non-movable
rwgk Jan 24, 2026
61e032e
Drop Python 3.13t support from CI
rwgk Jan 24, 2026
29840ce
Add Python 3.13 (default) replacement jobs for removed 3.13t jobs
rwgk Jan 24, 2026
91189c9
ci: run in free-threading mode a bit more on 3.14
henryiii Jan 26, 2026
603b25a
Merge branch 'master' into iterator-nogil-threadsafety
rwgk Jan 29, 2026
dc8e35c
Merge branch 'master' into colesbury→iterator-nogil-threadsafety
rwgk Feb 1, 2026
f3197de
Revert "ci: run in free-threading mode a bit more on 3.14"
rwgk Feb 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.13'
cmake-args: -DCMAKE_CXX_STANDARD=20 -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION=ON
- runs-on: ubuntu-latest
python-version: '3.14'
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -240,7 +237,7 @@ jobs:


manylinux:
name: Manylinux on 🐍 3.13t • GIL
name: Manylinux on 🐍 3.13 • GIL
if: github.event.pull_request.draft == false
runs-on: ubuntu-latest
timeout-minutes: 40
Expand All @@ -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.13

- name: Build C++11
run: cmake --build --preset venv
Expand Down
22 changes: 20 additions & 2 deletions include/pybind11/detail/internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ using instance_map = std::unordered_multimap<const void *, instance *>;
#ifdef Py_GIL_DISABLED
// Wrapper around PyMutex to provide BasicLockable semantics
class pymutex {
friend class pycritical_section;
PyMutex mutex;

public:
Expand All @@ -238,6 +239,23 @@ class pymutex {
void unlock() { PyMutex_Unlock(&mutex); }
};

class pycritical_section {
Copy link
Collaborator

@rwgk rwgk Jan 24, 2026

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator

@rwgk rwgk Jan 24, 2026

Choose a reason for hiding this comment

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

Result: be01d1e

(created with Cursor Composer 1 model)

pymutex &mutex;
Copy link
Collaborator

@Skylion007 Skylion007 Feb 1, 2026

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@rwgk rwgk Feb 1, 2026

Choose a reason for hiding this comment

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

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); }
};

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;
Expand Down Expand Up @@ -856,7 +874,7 @@ inline local_internals &get_local_internals() {
}

#ifdef Py_GIL_DISABLED
# define PYBIND11_LOCK_INTERNALS(internals) std::unique_lock<pymutex> lock((internals).mutex)
# define PYBIND11_LOCK_INTERNALS(internals) pycritical_section lock((internals).mutex)
#else
# define PYBIND11_LOCK_INTERNALS(internals)
#endif
Expand Down Expand Up @@ -885,7 +903,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<pymutex> 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,
Expand Down
1 change: 1 addition & 0 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3173,6 +3173,7 @@ iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&...extra) {
using state = detail::iterator_state<Access, Policy, Iterator, Sentinel, ValueType, Extra...>;
// 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_<state>(handle(), "iterator", pybind11::module_local())
.def(
Expand Down
Loading