-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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(§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) {}; | ||
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) |
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 | ||
|
@@ -365,15 +366,11 @@ TEST_CASE("Threads") { | |
#ifdef Py_GIL_DISABLED | ||
# if PY_VERSION_HEX < 0x030E0000 | ||
std::lock_guard<std::mutex> lock(mutex); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. Positive FeedbackNegative Feedback |
||
}); | ||
} | ||
|
||
|
There was a problem hiding this comment.
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:
Suggested addition to the explanatory text below the code block:
py::scoped_critical_section
, make sure it is not nested and that no other synchronization primitives (such as the GIL or astd::mutex
) are held that could lead to deadlocks. This RAII guard wraps Python’sPyCriticalSection_*
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
.There was a problem hiding this comment.
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. Butstd::mutex
could deadlock.