Skip to content

Commit 4d7d02a

Browse files
colesburypre-commit-ci[bot]rwgkhenryiii
authored
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 91189c9. Reason: #5971 (comment) * Reapply "ci: run in free-threading mode a bit more on 3.14" This reverts commit f3197de. After #5972 is/was merged, tests should pass (already tested under #5980). See also #5972 (comment) --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com> Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com> Co-authored-by: Ralf W. Grosse-Kunstleve <rwgkio@gmail.com>
1 parent e7754de commit 4d7d02a

File tree

3 files changed

+27
-11
lines changed

3 files changed

+27
-11
lines changed

.github/workflows/ci.yml

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ jobs:
8484
python-version: '3.12'
8585
cmake-args: -DPYBIND11_TEST_SMART_HOLDER=ON -DPYBIND11_SIMPLE_GIL_MANAGEMENT=ON
8686
- runs-on: ubuntu-latest
87-
python-version: '3.13t'
87+
python-version: '3.14t'
8888
cmake-args: -DCMAKE_CXX_STANDARD=20 -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION=ON
8989
- runs-on: ubuntu-latest
9090
python-version: '3.14'
@@ -102,12 +102,12 @@ jobs:
102102
- runs-on: macos-15-intel
103103
python-version: '3.11'
104104
cmake-args: -DPYBIND11_TEST_SMART_HOLDER=ON
105+
- runs-on: macos-15-intel
106+
python-version: '3.13'
107+
cmake-args: -DCMAKE_CXX_STANDARD=11
105108
- runs-on: macos-latest
106109
python-version: '3.12'
107110
cmake-args: -DCMAKE_CXX_STANDARD=17 -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION=ON
108-
- runs-on: macos-15-intel
109-
python-version: '3.13t'
110-
cmake-args: -DCMAKE_CXX_STANDARD=11
111111
- runs-on: macos-latest
112112
python-version: '3.14t'
113113
cmake-args: -DCMAKE_CXX_STANDARD=20
@@ -138,9 +138,6 @@ jobs:
138138
- runs-on: windows-2022
139139
python-version: '3.13'
140140
cmake-args: -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebugDLL
141-
- runs-on: windows-latest
142-
python-version: '3.13t'
143-
cmake-args: -DCMAKE_CXX_STANDARD=17
144141
- runs-on: windows-latest
145142
python-version: '3.14'
146143
cmake-args: -DCMAKE_CXX_STANDARD=20
@@ -240,7 +237,7 @@ jobs:
240237

241238

242239
manylinux:
243-
name: Manylinux on 🐍 3.13t • GIL
240+
name: Manylinux on 🐍 3.14t
244241
if: github.event.pull_request.draft == false
245242
runs-on: ubuntu-latest
246243
timeout-minutes: 40
@@ -257,7 +254,7 @@ jobs:
257254
run: uv tool install ninja
258255

259256
- name: Configure via preset
260-
run: cmake --preset venv -DPYBIND11_CREATE_WITH_UV=python3.13t
257+
run: cmake --preset venv -DPYBIND11_CREATE_WITH_UV=python3.14t
261258

262259
- name: Build C++11
263260
run: cmake --build --preset venv

include/pybind11/detail/internals.h

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,7 @@ using instance_map = std::unordered_multimap<const void *, instance *>;
230230
#ifdef Py_GIL_DISABLED
231231
// Wrapper around PyMutex to provide BasicLockable semantics
232232
class pymutex {
233+
friend class pycritical_section;
233234
PyMutex mutex;
234235

235236
public:
@@ -238,6 +239,23 @@ class pymutex {
238239
void unlock() { PyMutex_Unlock(&mutex); }
239240
};
240241

242+
class pycritical_section {
243+
pymutex &mutex;
244+
PyCriticalSection cs;
245+
246+
public:
247+
explicit pycritical_section(pymutex &m) : mutex(m) {
248+
PyCriticalSection_BeginMutex(&cs, &mutex.mutex);
249+
}
250+
~pycritical_section() { PyCriticalSection_End(&cs); }
251+
252+
// Non-copyable and non-movable to prevent double-unlock
253+
pycritical_section(const pycritical_section &) = delete;
254+
pycritical_section &operator=(const pycritical_section &) = delete;
255+
pycritical_section(pycritical_section &&) = delete;
256+
pycritical_section &operator=(pycritical_section &&) = delete;
257+
};
258+
241259
// Instance map shards are used to reduce mutex contention in free-threaded Python.
242260
struct instance_map_shard {
243261
instance_map registered_instances;
@@ -905,7 +923,7 @@ inline local_internals &get_local_internals() {
905923
}
906924

907925
#ifdef Py_GIL_DISABLED
908-
# define PYBIND11_LOCK_INTERNALS(internals) std::unique_lock<pymutex> lock((internals).mutex)
926+
# define PYBIND11_LOCK_INTERNALS(internals) pycritical_section lock((internals).mutex)
909927
#else
910928
# define PYBIND11_LOCK_INTERNALS(internals)
911929
#endif
@@ -934,7 +952,7 @@ inline auto with_exception_translators(const F &cb)
934952
get_local_internals().registered_exception_translators)) {
935953
auto &internals = get_internals();
936954
#ifdef Py_GIL_DISABLED
937-
std::unique_lock<pymutex> lock((internals).exception_translator_mutex);
955+
pycritical_section lock((internals).exception_translator_mutex);
938956
#endif
939957
auto &local_internals = get_local_internals();
940958
return cb(internals.registered_exception_translators,

include/pybind11/pybind11.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3173,6 +3173,7 @@ iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&...extra) {
31733173
using state = detail::iterator_state<Access, Policy, Iterator, Sentinel, ValueType, Extra...>;
31743174
// TODO: state captures only the types of Extra, not the values
31753175

3176+
PYBIND11_LOCK_INTERNALS(get_internals());
31763177
if (!detail::get_type_info(typeid(state), false)) {
31773178
class_<state>(handle(), "iterator", pybind11::module_local())
31783179
.def(

0 commit comments

Comments
 (0)