mirror of
https://github.com/pybind/pybind11.git
synced 2026-04-20 23:09:13 +00:00
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 commit91189c9242. 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 commitf3197de975. 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 <rgrossekunst@nvidia.com> Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com> Co-authored-by: Ralf W. Grosse-Kunstleve <rwgkio@gmail.com>
This commit is contained in:
15
.github/workflows/ci.yml
vendored
15
.github/workflows/ci.yml
vendored
@@ -84,7 +84,7 @@ jobs:
|
|||||||
python-version: '3.12'
|
python-version: '3.12'
|
||||||
cmake-args: -DPYBIND11_TEST_SMART_HOLDER=ON -DPYBIND11_SIMPLE_GIL_MANAGEMENT=ON
|
cmake-args: -DPYBIND11_TEST_SMART_HOLDER=ON -DPYBIND11_SIMPLE_GIL_MANAGEMENT=ON
|
||||||
- runs-on: ubuntu-latest
|
- 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
|
cmake-args: -DCMAKE_CXX_STANDARD=20 -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION=ON
|
||||||
- runs-on: ubuntu-latest
|
- runs-on: ubuntu-latest
|
||||||
python-version: '3.14'
|
python-version: '3.14'
|
||||||
@@ -102,12 +102,12 @@ jobs:
|
|||||||
- runs-on: macos-15-intel
|
- runs-on: macos-15-intel
|
||||||
python-version: '3.11'
|
python-version: '3.11'
|
||||||
cmake-args: -DPYBIND11_TEST_SMART_HOLDER=ON
|
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
|
- runs-on: macos-latest
|
||||||
python-version: '3.12'
|
python-version: '3.12'
|
||||||
cmake-args: -DCMAKE_CXX_STANDARD=17 -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION=ON
|
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
|
- runs-on: macos-latest
|
||||||
python-version: '3.14t'
|
python-version: '3.14t'
|
||||||
cmake-args: -DCMAKE_CXX_STANDARD=20
|
cmake-args: -DCMAKE_CXX_STANDARD=20
|
||||||
@@ -138,9 +138,6 @@ jobs:
|
|||||||
- runs-on: windows-2022
|
- runs-on: windows-2022
|
||||||
python-version: '3.13'
|
python-version: '3.13'
|
||||||
cmake-args: -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebugDLL
|
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
|
- runs-on: windows-latest
|
||||||
python-version: '3.14'
|
python-version: '3.14'
|
||||||
cmake-args: -DCMAKE_CXX_STANDARD=20
|
cmake-args: -DCMAKE_CXX_STANDARD=20
|
||||||
@@ -240,7 +237,7 @@ jobs:
|
|||||||
|
|
||||||
|
|
||||||
manylinux:
|
manylinux:
|
||||||
name: Manylinux on 🐍 3.13t • GIL
|
name: Manylinux on 🐍 3.14t
|
||||||
if: github.event.pull_request.draft == false
|
if: github.event.pull_request.draft == false
|
||||||
runs-on: ubuntu-latest
|
runs-on: ubuntu-latest
|
||||||
timeout-minutes: 40
|
timeout-minutes: 40
|
||||||
@@ -257,7 +254,7 @@ jobs:
|
|||||||
run: uv tool install ninja
|
run: uv tool install ninja
|
||||||
|
|
||||||
- name: Configure via preset
|
- 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
|
- name: Build C++11
|
||||||
run: cmake --build --preset venv
|
run: cmake --build --preset venv
|
||||||
|
|||||||
@@ -230,6 +230,7 @@ using instance_map = std::unordered_multimap<const void *, instance *>;
|
|||||||
#ifdef Py_GIL_DISABLED
|
#ifdef Py_GIL_DISABLED
|
||||||
// Wrapper around PyMutex to provide BasicLockable semantics
|
// Wrapper around PyMutex to provide BasicLockable semantics
|
||||||
class pymutex {
|
class pymutex {
|
||||||
|
friend class pycritical_section;
|
||||||
PyMutex mutex;
|
PyMutex mutex;
|
||||||
|
|
||||||
public:
|
public:
|
||||||
@@ -238,6 +239,23 @@ public:
|
|||||||
void unlock() { PyMutex_Unlock(&mutex); }
|
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.
|
// Instance map shards are used to reduce mutex contention in free-threaded Python.
|
||||||
struct instance_map_shard {
|
struct instance_map_shard {
|
||||||
instance_map registered_instances;
|
instance_map registered_instances;
|
||||||
@@ -905,7 +923,7 @@ inline local_internals &get_local_internals() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#ifdef Py_GIL_DISABLED
|
#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
|
#else
|
||||||
# define PYBIND11_LOCK_INTERNALS(internals)
|
# define PYBIND11_LOCK_INTERNALS(internals)
|
||||||
#endif
|
#endif
|
||||||
@@ -934,7 +952,7 @@ inline auto with_exception_translators(const F &cb)
|
|||||||
get_local_internals().registered_exception_translators)) {
|
get_local_internals().registered_exception_translators)) {
|
||||||
auto &internals = get_internals();
|
auto &internals = get_internals();
|
||||||
#ifdef Py_GIL_DISABLED
|
#ifdef Py_GIL_DISABLED
|
||||||
std::unique_lock<pymutex> lock((internals).exception_translator_mutex);
|
pycritical_section lock((internals).exception_translator_mutex);
|
||||||
#endif
|
#endif
|
||||||
auto &local_internals = get_local_internals();
|
auto &local_internals = get_local_internals();
|
||||||
return cb(internals.registered_exception_translators,
|
return cb(internals.registered_exception_translators,
|
||||||
|
|||||||
@@ -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...>;
|
using state = detail::iterator_state<Access, Policy, Iterator, Sentinel, ValueType, Extra...>;
|
||||||
// TODO: state captures only the types of Extra, not the values
|
// TODO: state captures only the types of Extra, not the values
|
||||||
|
|
||||||
|
PYBIND11_LOCK_INTERNALS(get_internals());
|
||||||
if (!detail::get_type_info(typeid(state), false)) {
|
if (!detail::get_type_info(typeid(state), false)) {
|
||||||
class_<state>(handle(), "iterator", pybind11::module_local())
|
class_<state>(handle(), "iterator", pybind11::module_local())
|
||||||
.def(
|
.def(
|
||||||
|
|||||||
Reference in New Issue
Block a user