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

Merged
rwgk merged 13 commits into from
Feb 2, 2026
+27 −11
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 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
07e0360
Reapply "ci: run in free-threading mode a bit more on 3.14"
rwgk Feb 2, 2026
5505b4d
[skip ci] Merge branch 'master' into colesbury→iterator-nogil-threads…
rwgk Feb 2, 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
31 changes: 29 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 @@ -238,6 +238,32 @@ class pymutex {
void unlock() { PyMutex_Unlock(&mutex); }
};

// A recursive mutex implementation using PyMutex
class pyrecursive_mutex {
PyMutex mutex;
std::atomic<uintptr_t> owner;
size_t lock_count;

public:
pyrecursive_mutex() : mutex({}), owner(0), lock_count(0) {}
void lock() {
if (owner.load(std::memory_order_relaxed) == _Py_ThreadId()) {
++lock_count;
return;
}
PyMutex_Lock(&mutex);
owner.store(_Py_ThreadId(), std::memory_order_relaxed);
}
void unlock() {
if (lock_count > 0) {
--lock_count;
return;
}
owner.store(0, std::memory_order_relaxed);
PyMutex_Unlock(&mutex);
}
};

// 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 @@ -271,7 +297,7 @@ class loader_life_support;
/// `PYBIND11_INTERNALS_VERSION` must be incremented.
struct internals {
#ifdef Py_GIL_DISABLED
pymutex mutex;
pyrecursive_mutex mutex;
Copy link
Contributor Author

@colesbury colesbury Jan 21, 2026

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

@rwgk rwgk Jan 21, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@henryiii henryiii Jan 21, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@henryiii henryiii Jan 21, 2026

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@colesbury colesbury Jan 22, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@XuehaiPan XuehaiPan Feb 2, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@rwgk rwgk Feb 2, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@XuehaiPan XuehaiPan Feb 2, 2026

Choose a reason for hiding this comment

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

I opened #5981

pymutex exception_translator_mutex;
#endif
#if PYBIND11_INTERNALS_VERSION >= 12
Expand Down Expand Up @@ -856,7 +882,8 @@ 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) \
std::unique_lock<pyrecursive_mutex> lock((internals).mutex)
#else
# define PYBIND11_LOCK_INTERNALS(internals)
#endif
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