From 3af2d849be201a40b3ee36c10bba7de1fd46fc6a Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Fri, 23 May 2025 10:43:53 -0400 Subject: [PATCH 1/5] feat: scoped_critical_section Signed-off-by: Henry Schreiner --- docs/advanced/misc.rst | 9 +++--- include/pybind11/gil.h | 40 +++++++++++++++++++++++++++ tests/test_embed/test_interpreter.cpp | 6 +--- 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/docs/advanced/misc.rst b/docs/advanced/misc.rst index b8cb1923e9..9331efab02 100644 --- a/docs/advanced/misc.rst +++ b/docs/advanced/misc.rst @@ -295,8 +295,10 @@ This module is sub-interpreter safe, for both ``shared_gil`` ("legacy") and function concurrently from different threads. This is safe because each sub-interpreter's GIL protects it's own Python objects from concurrent access. -However, the module is no longer free-threading safe, for the same reason as before, because the -calculation is not synchronized. We can synchronize it using a Python critical section. +However, the module is no longer free-threading safe, for the same reason as +before, because the calculation is not synchronized. We can synchronize it +using a Python critical section. This will do nothing if not in free-threaded +Python. You can have it lock one or two Python objects. You cannot nest it. .. code-block:: cpp :emphasize-lines: 1,5,10 @@ -305,12 +307,11 @@ calculation is not synchronized. We can synchronize it using a Python critical s m.def("calc_next", []() { size_t old; py::dict g = py::globals(); - Py_BEGIN_CRITICAL_SECTION(g); + py::scoped_critical_section guard(g); if (!g.contains("myseed")) g["myseed"] = 0; old = g["myseed"]; g["myseed"] = (old + 1) * 10; - Py_END_CRITICAL_SECTION(); return old; }); } diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h index e90ea41528..9d043ebc0b 100644 --- a/include/pybind11/gil.h +++ b/include/pybind11/gil.h @@ -199,3 +199,43 @@ class gil_scoped_release { PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) #endif // !PYBIND11_SIMPLE_GIL_MANAGEMENT + +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) + +class scoped_critical_section { +public: + /// This does not do anything if there's a GIL. On free-threaded Python, + /// it locks an object. This uses the CPython API, which has limits + scoped_critical_section(handle obj) : single(true) { +#ifdef Py_GIL_DISABLED + PyCriticalSection_Begin(§ion, obj.ptr()); +#endif + } + + scoped_critical_section(handle obj1, handle obj2) : single(true) { +#ifdef Py_GIL_DISABLED + PyCriticalSection2_Begin(§ion2, obj1.ptr(), obj2.ptr()); +#endif + } + + ~scoped_critical_section() { +#ifdef Py_GIL_DISABLED + if(single) { + PyCriticalSection_End(§ion); + } else { + PyCriticalSection2_End(§ion2); + } +#endif + } + +private: + bool single; +#ifdef Py_GIL_DISABLED + union { + PyCriticalSection section; + PyCriticalSection2 section2; + }; +#endif +}; + +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index e555c0d70c..a77a1e3de0 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -365,13 +365,9 @@ TEST_CASE("Threads") { #ifdef Py_GIL_DISABLED # if PY_VERSION_HEX < 0x030E0000 std::lock_guard lock(mutex); - locals["count"] = locals["count"].cast() + 1; # else - Py_BEGIN_CRITICAL_SECTION(locals.ptr()); - locals["count"] = locals["count"].cast() + 1; - Py_END_CRITICAL_SECTION(); + py::scoped_critical_section lock(locals); # endif -#else locals["count"] = locals["count"].cast() + 1; #endif }); From 70852d52144b2d63d1ec1b95191f79c745a8dbc5 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Fri, 23 May 2025 12:42:19 -0400 Subject: [PATCH 2/5] refactor: pull out to file Signed-off-by: Henry Schreiner --- CMakeLists.txt | 1 + docs/advanced/misc.rst | 5 ++- include/pybind11/critical_section.h | 50 ++++++++++++++++++++++++ include/pybind11/gil.h | 40 ------------------- tests/extra_python_package/test_files.py | 1 + tests/test_embed/test_interpreter.cpp | 1 + 6 files changed, 57 insertions(+), 41 deletions(-) create mode 100644 include/pybind11/critical_section.h diff --git a/CMakeLists.txt b/CMakeLists.txt index a1a6f4f4d8..ba5f665a2f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -205,6 +205,7 @@ set(PYBIND11_HEADERS include/pybind11/conduit/pybind11_conduit_v1.h include/pybind11/conduit/pybind11_platform_abi_id.h include/pybind11/conduit/wrap_include_python_h.h + include/pybind11/critical_section.h include/pybind11/options.h include/pybind11/eigen.h include/pybind11/eigen/common.h diff --git a/docs/advanced/misc.rst b/docs/advanced/misc.rst index 9331efab02..7cce2d2505 100644 --- a/docs/advanced/misc.rst +++ b/docs/advanced/misc.rst @@ -301,7 +301,10 @@ using a Python critical section. This will do nothing if not in free-threaded Python. You can have it lock one or two Python objects. You cannot nest it. .. code-block:: cpp - :emphasize-lines: 1,5,10 + :emphasize-lines: 1,4,8 + + #include + // ... PYBIND11_MODULE(example, m, py::multiple_interpreters::per_interpreter_gil(), py::mod_gil_not_used()) { m.def("calc_next", []() { diff --git a/include/pybind11/critical_section.h b/include/pybind11/critical_section.h new file mode 100644 index 0000000000..56d4fdd4de --- /dev/null +++ b/include/pybind11/critical_section.h @@ -0,0 +1,50 @@ +// Copyright (c) 2016-2025 The Pybind Development Team. +// All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +#pragma once + +#include "pytypes.h" + +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) + +/// This does not do anything if there's a GIL. On free-threaded Python, +/// it locks an object. This uses the CPython API, which has limits +class scoped_critical_section { +public: +#ifdef Py_GIL_DISABLED + explicit scoped_critical_section(handle obj) : has2(false) { + PyCriticalSection_Begin(§ion, obj.ptr()); + } + + scoped_critical_section(handle obj1, handle obj2) : has2(true) { + PyCriticalSection2_Begin(§ion2, obj1.ptr(), obj2.ptr()); + } + + ~scoped_critical_section() { + if(has2) { + PyCriticalSection2_End(§ion2); + } else { + PyCriticalSection_End(§ion); + } + } +#else + explicit scoped_critical_section(handle _obj) = default; + scoped_critical_section(handle _obj1, handle _obj2) = default; + ~scoped_critical_section() = default; +#endif + + scoped_critical_section(const scoped_critical_section &) = delete; + scoped_critical_section &operator=(const scoped_critical_section &) = delete; + +private: +#ifdef Py_GIL_DISABLED + bool has2; + union { + PyCriticalSection section; + PyCriticalSection2 section2; + }; +#endif +}; + +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h index 9d043ebc0b..e90ea41528 100644 --- a/include/pybind11/gil.h +++ b/include/pybind11/gil.h @@ -199,43 +199,3 @@ class gil_scoped_release { PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) #endif // !PYBIND11_SIMPLE_GIL_MANAGEMENT - -PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) - -class scoped_critical_section { -public: - /// This does not do anything if there's a GIL. On free-threaded Python, - /// it locks an object. This uses the CPython API, which has limits - scoped_critical_section(handle obj) : single(true) { -#ifdef Py_GIL_DISABLED - PyCriticalSection_Begin(§ion, obj.ptr()); -#endif - } - - scoped_critical_section(handle obj1, handle obj2) : single(true) { -#ifdef Py_GIL_DISABLED - PyCriticalSection2_Begin(§ion2, obj1.ptr(), obj2.ptr()); -#endif - } - - ~scoped_critical_section() { -#ifdef Py_GIL_DISABLED - if(single) { - PyCriticalSection_End(§ion); - } else { - PyCriticalSection2_End(§ion2); - } -#endif - } - -private: - bool single; -#ifdef Py_GIL_DISABLED - union { - PyCriticalSection section; - PyCriticalSection2 section2; - }; -#endif -}; - -PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index 9725bedae1..09ae0dcfb8 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -44,6 +44,7 @@ "include/pybind11/chrono.h", "include/pybind11/common.h", "include/pybind11/complex.h", + "include/pybind11/critical_section.h", "include/pybind11/eigen.h", "include/pybind11/embed.h", "include/pybind11/eval.h", diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index a77a1e3de0..aabd69acb9 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -1,4 +1,5 @@ #include +#include // Silence MSVC C++17 deprecation warning from Catch regarding std::uncaught_exceptions (up to // catch 2.0.1; this should be fixed in the next catch release after 2.0.1). From 5aaffb2bb6730952c658e0107c5d9bb274ca944d Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 23 May 2025 16:52:11 +0000 Subject: [PATCH 3/5] style: pre-commit fixes --- include/pybind11/critical_section.h | 2 +- tests/test_embed/test_interpreter.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/critical_section.h b/include/pybind11/critical_section.h index 56d4fdd4de..4703031a59 100644 --- a/include/pybind11/critical_section.h +++ b/include/pybind11/critical_section.h @@ -22,7 +22,7 @@ class scoped_critical_section { } ~scoped_critical_section() { - if(has2) { + if (has2) { PyCriticalSection2_End(§ion2); } else { PyCriticalSection_End(§ion); diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index aabd69acb9..a1ec7f1d75 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -1,5 +1,5 @@ -#include #include +#include // Silence MSVC C++17 deprecation warning from Catch regarding std::uncaught_exceptions (up to // catch 2.0.1; this should be fixed in the next catch release after 2.0.1). From 4db3dba39e0fc16efb77104e2d90fae944313ed2 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Fri, 23 May 2025 15:02:13 -0400 Subject: [PATCH 4/5] fix: GIL code in some compilers Signed-off-by: Henry Schreiner --- include/pybind11/critical_section.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/critical_section.h b/include/pybind11/critical_section.h index 4703031a59..e94ca765cb 100644 --- a/include/pybind11/critical_section.h +++ b/include/pybind11/critical_section.h @@ -29,8 +29,8 @@ class scoped_critical_section { } } #else - explicit scoped_critical_section(handle _obj) = default; - scoped_critical_section(handle _obj1, handle _obj2) = default; + explicit scoped_critical_section(handle) {}; + scoped_critical_section(handle, handle) {}; ~scoped_critical_section() = default; #endif From c1a323ebd75043884ae3ee95f115dae7a2fb5d15 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Fri, 23 May 2025 15:23:44 -0400 Subject: [PATCH 5/5] fix: move to correct spot Signed-off-by: Henry Schreiner --- docs/advanced/misc.rst | 3 +++ tests/test_embed/test_interpreter.cpp | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/advanced/misc.rst b/docs/advanced/misc.rst index 7cce2d2505..42cfc69f8c 100644 --- a/docs/advanced/misc.rst +++ b/docs/advanced/misc.rst @@ -299,6 +299,9 @@ However, the module is no longer free-threading safe, for the same reason as before, because the calculation is not synchronized. We can synchronize it using a Python critical section. This will do nothing if not in free-threaded Python. You can have it lock one or two Python objects. You cannot nest it. +(Note: In Python 3.13t, Python re-locks if you enter a critical section again, +which happens in various places. This was optimized away in 3.14+. Use a +``std::mutex`` instead if this is a problem). .. code-block:: cpp :emphasize-lines: 1,4,8 diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index a1ec7f1d75..3654708d2e 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -369,8 +369,8 @@ TEST_CASE("Threads") { # else py::scoped_critical_section lock(locals); # endif - locals["count"] = locals["count"].cast() + 1; #endif + locals["count"] = locals["count"].cast() + 1; }); }