Skip to content

feat: scoped_critical_section #5684

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.

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
merged 5 commits into from
May 23, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 12 additions & 5 deletions docs/advanced/misc.rst
Original file line number Diff line number Diff line change
Expand Up @@ -295,22 +295,29 @@ 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.
(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).
Copy link
Collaborator

Choose a reason for hiding this comment

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

My concern: I feel like this could send inexperienced users into the deadlock pitfall.

Please take a look here for background:

https://chatgpt.com/share/682a07ec-dd70-8008-92ea-e722ff92a055

Concrete suggestions from the chat:

Add a comment to clarify:

            py::scoped_critical_section guard(g); // Safe only if this is not nested and no GIL is held.

Suggested addition to the explanatory text below the code block:


⚠️ When using py::scoped_critical_section, make sure it is not nested and that no other synchronization primitives (such as the GIL or a std::mutex) are held that could lead to deadlocks. This RAII guard wraps Python’s PyCriticalSection_* APIs, which are only active in free-threaded builds (Py_GIL_DISABLED). In standard (GIL-enabled) Python builds, the guard is a no-op.

On Python 3.13, nested use of critical sections may re-lock underlying internal structures; this was optimized away in Python 3.14+. If needed, consider using a std::mutex to avoid such cases.


My own refinement of the last point: Depending on the situation, you may have to resort to a std::mutex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

py::scoped_critical_section is safe WRT the GIL, because it is GIL aware, so "such as the GIL" doesn't apply to it. But std::mutex could deadlock.


.. code-block:: cpp
:emphasize-lines: 1,5,10
:emphasize-lines: 1,4,8

#include <pybind11/critical_section.h>
// ...

PYBIND11_MODULE(example, m, py::multiple_interpreters::per_interpreter_gil(), py::mod_gil_not_used()) {
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;
});
}
Expand Down
50 changes: 50 additions & 0 deletions include/pybind11/critical_section.h
Original file line number Diff line number Diff line change
@@ -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(&section, obj.ptr());
}

scoped_critical_section(handle obj1, handle obj2) : has2(true) {
PyCriticalSection2_Begin(&section2, obj1.ptr(), obj2.ptr());
}

~scoped_critical_section() {
if (has2) {
PyCriticalSection2_End(&section2);
} else {
PyCriticalSection_End(&section);
}
}
#else
explicit scoped_critical_section(handle) {};
scoped_critical_section(handle, handle) {};
~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)
1 change: 1 addition & 0 deletions tests/extra_python_package/test_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
9 changes: 3 additions & 6 deletions tests/test_embed/test_interpreter.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include <pybind11/critical_section.h>
#include <pybind11/embed.h>

// Silence MSVC C++17 deprecation warning from Catch regarding std::uncaught_exceptions (up to
Expand Down Expand Up @@ -365,15 +366,11 @@ TEST_CASE("Threads") {
#ifdef Py_GIL_DISABLED
# if PY_VERSION_HEX < 0x030E0000
std::lock_guard<std::mutex> lock(mutex);
Copy link
Collaborator

Choose a reason for hiding this comment

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

While we're touching this code, I think it'll be prudent to warn readers about the classic pitfall:

                 py::gil_scoped_acquire gil{};
 #ifdef Py_GIL_DISABLED
 #    if PY_VERSION_HEX < 0x030E0000
+                // WARNING: This assumes that the Python C API call below does
+                // not release and reacquire the GIL. Guarding Python object
+                // access with a C++ mutex (while holding the GIL) can
+                // deadlock if that assumption is violated.
                 std::lock_guard<std::mutex> lock(mutex);
 #    else
+                // Safe: uses CPython's thread-safe API in no-GIL mode.
                 py::scoped_critical_section lock(locals);
 #    endif
 #endif

locals["count"] = locals["count"].cast<int>() + 1;
# else
Py_BEGIN_CRITICAL_SECTION(locals.ptr());
locals["count"] = locals["count"].cast<int>() + 1;
Py_END_CRITICAL_SECTION();
py::scoped_critical_section lock(locals);
# endif
#else
locals["count"] = locals["count"].cast<int>() + 1;
#endif
locals["count"] = locals["count"].cast<int>() + 1;
Comment on lines 371 to +373
Copy link
Preview

Copilot AI May 23, 2025

Choose a reason for hiding this comment

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

In the conditional block for Py_GIL_DISABLED, the update to locals['count'] appears inside the lock for the older branch but is performed outside for the new branch. To ensure consistent thread safety, consider moving the update inside the critical section scope in all branches.

Copilot uses AI. Check for mistakes.

});
}

Expand Down
Loading