Skip to content

Commit 0e2c3e5

Browse files
authored
Add pybind11/gil_safe_call_once.h (to fix deadlocks in pybind11/numpy.h) (#4877)
* LazyInitializeAtLeastOnceDestroyNever v1 * Go back to using `union` as originally suggested by jbms@. The trick (also suggested by jbms@) is to add empty ctor + dtor. * Revert "Go back to using `union` as originally suggested by jbms@. The trick (also suggested by jbms@) is to add empty ctor + dtor." This reverts commit e7b8c4f. * Remove `#include <stdalign.h>` * `include\pybind11/numpy.h(24,10): fatal error C1083: Cannot open include file: 'stdalign.h': No such file or directory` * @tkoeppe wrote: this is a C interop header (and we're not writing C) * Suppress gcc 4.8.5 (CentOS 7) warning. ``` include/pybind11/eigen/../numpy.h:63:53: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing] return *reinterpret_cast<T *>(value_storage_); ^ ``` * Replace comments: Document PRECONDITION. Adopt comment suggested by @tkoeppe: #4877 (comment) * Adopt suggestion by @tkoeppe: * #4877 (comment) * https://godbolt.org/z/Wa79nKz6e * Add `PYBIND11_CONSTINIT`, but it does not work for the current use cases: ``` g++ -o pybind11/tests/test_numpy_array.os -c -std=c++20 -fPIC -fvisibility=hidden -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Wunused-result -Werror -isystem /usr/include/python3.11 -isystem /usr/include/eigen3 -DPYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/local/google/home/rwgk/forked/pybind11/include -I/usr/local/google/home/rwgk/clone/pybind11/include /usr/local/google/home/rwgk/forked/pybind11/tests/test_numpy_array.cpp ``` ``` In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/test_numpy_array.cpp:10: /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/numpy.h: In static member function ‘static pybind11::detail::npy_api& pybind11::detail::npy_api::get()’: /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/numpy.h:258:82: error: ‘constinit’ variable ‘api_init’ does not have a constant initializer 258 | PYBIND11_CONSTINIT static LazyInitializeAtLeastOnceDestroyNever<npy_api> api_init; | ^~~~~~~~ ``` ``` In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/test_numpy_array.cpp:10: /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/numpy.h: In static member function ‘static pybind11::object& pybind11::dtype::_dtype_from_pep3118()’: /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/numpy.h:697:13: error: ‘constinit’ variable ‘imported_obj’ does not have a constant initializer 697 | imported_obj; | ^~~~~~~~~~~~ ``` * Revert "Add `PYBIND11_CONSTINIT`, but it does not work for the current use cases:" This reverts commit f07b28b. * Reapply "Add `PYBIND11_CONSTINIT`, but it does not work for the current use cases:" This reverts commit 36be645. * Add Default Member Initializer on `value_storage_` as suggested by @tkoeppe: #4877 (comment) This fixes the errors reported under commit f07b28b. * Fix copy-paste-missed-a-change mishap in commit 88cec11. * Semi-paranoid placement new (based on #4877 (comment)). * Move PYBIND11_CONSTINIT to detail/common.h * Move code to the right places, rename new class and some variables. * Fix oversight: update tests/extra_python_package/test_files.py * Get the name right first. * Use `std::call_once`, `std::atomic`, following a pattern developed by @tkoeppe * Make the API more self-documenting (and possibly more easily reusable). * google-clang-tidy IWYU fixes * Rewrite comment as suggested by @tkoeppe * Update test_exceptions.cpp and exceptions.rst * Fix oversight in previous commit: add `PYBIND11_CONSTINIT` * Make `get_stored()` non-const for simplicity. As suggested by @tkoeppe: not seeing any reasonable use in which `get_stored` has to be const. * Add comment regarding `KeyboardInterrupt` behavior, based heavily on information provided by @jbms. * Add `assert(PyGILState_Check())` in `gil_scoped_release` ctor (simple & non-simple implementation) as suggested by @EthanSteinberg. * Fix oversight in previous commit (missing include cassert). * Remove use of std::atomic, leaving comments with rationale, why it is not needed. * Rewrite comment re `std:optional` based on deeper reflection (aka 2nd thoughts). * Additional comment with the conclusion of a discussion under PR #4877. * #4877 (comment) * Small comment changes suggested by @tkoeppe.
1 parent 6c77208 commit 0e2c3e5

File tree

8 files changed

+133
-14
lines changed

8 files changed

+133
-14
lines changed

CMakeLists.txt

+1
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,7 @@ set(PYBIND11_HEADERS
142142
include/pybind11/embed.h
143143
include/pybind11/eval.h
144144
include/pybind11/gil.h
145+
include/pybind11/gil_safe_call_once.h
145146
include/pybind11/iostream.h
146147
include/pybind11/functional.h
147148
include/pybind11/numpy.h

docs/advanced/exceptions.rst

+4-5
Original file line numberDiff line numberDiff line change
@@ -141,15 +141,14 @@ standard python RuntimeError:
141141

142142
.. code-block:: cpp
143143
144-
// This is a static object, so we must leak the Python reference:
145-
// It is undefined when the destructor will run, possibly only after the
146-
// Python interpreter is finalized already.
147-
static py::handle exc = py::exception<MyCustomException>(m, "MyCustomError").release();
144+
PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store<py::object> exc_storage;
145+
exc_storage.call_once_and_store_result(
146+
[&]() { return py::exception<MyCustomException>(m, "MyCustomError"); });
148147
py::register_exception_translator([](std::exception_ptr p) {
149148
try {
150149
if (p) std::rethrow_exception(p);
151150
} catch (const MyCustomException &e) {
152-
py::set_error(exc, e.what());
151+
py::set_error(exc_storage.get_stored(), e.what());
153152
} catch (const OtherException &e) {
154153
py::set_error(PyExc_RuntimeError, e.what());
155154
}

include/pybind11/detail/common.h

+8
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,14 @@
118118
# endif
119119
#endif
120120

121+
#if defined(PYBIND11_CPP20)
122+
# define PYBIND11_CONSTINIT constinit
123+
# define PYBIND11_DTOR_CONSTEXPR constexpr
124+
#else
125+
# define PYBIND11_CONSTINIT
126+
# define PYBIND11_DTOR_CONSTEXPR
127+
#endif
128+
121129
// Compiler version assertions
122130
#if defined(__INTEL_COMPILER)
123131
# if __INTEL_COMPILER < 1800

include/pybind11/gil.h

+9-1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111

1212
#include "detail/common.h"
1313

14+
#include <cassert>
15+
1416
#if defined(WITH_THREAD) && !defined(PYBIND11_SIMPLE_GIL_MANAGEMENT)
1517
# include "detail/internals.h"
1618
#endif
@@ -137,7 +139,9 @@ class gil_scoped_acquire {
137139

138140
class gil_scoped_release {
139141
public:
142+
// PRECONDITION: The GIL must be held when this constructor is called.
140143
explicit gil_scoped_release(bool disassoc = false) : disassoc(disassoc) {
144+
assert(PyGILState_Check());
141145
// `get_internals()` must be called here unconditionally in order to initialize
142146
// `internals.tstate` for subsequent `gil_scoped_acquire` calls. Otherwise, an
143147
// initialization race could occur as multiple threads try `gil_scoped_acquire`.
@@ -201,7 +205,11 @@ class gil_scoped_release {
201205
PyThreadState *state;
202206

203207
public:
204-
gil_scoped_release() : state{PyEval_SaveThread()} {}
208+
// PRECONDITION: The GIL must be held when this constructor is called.
209+
gil_scoped_release() {
210+
assert(PyGILState_Check());
211+
state = PyEval_SaveThread();
212+
}
205213
gil_scoped_release(const gil_scoped_release &) = delete;
206214
gil_scoped_release &operator=(const gil_scoped_release &) = delete;
207215
~gil_scoped_release() { PyEval_RestoreThread(state); }

include/pybind11/gil_safe_call_once.h

+91
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
// Copyright (c) 2023 The pybind Community.
2+
3+
#pragma once
4+
5+
#include "detail/common.h"
6+
#include "gil.h"
7+
8+
#include <cassert>
9+
#include <mutex>
10+
11+
PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
12+
13+
// Use the `gil_safe_call_once_and_store` class below instead of the naive
14+
//
15+
// static auto imported_obj = py::module_::import("module_name"); // BAD, DO NOT USE!
16+
//
17+
// which has two serious issues:
18+
//
19+
// 1. Py_DECREF() calls potentially after the Python interpreter was finalized already, and
20+
// 2. deadlocks in multi-threaded processes (because of missing lock ordering).
21+
//
22+
// The following alternative avoids both problems:
23+
//
24+
// PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store<py::object> storage;
25+
// auto &imported_obj = storage // Do NOT make this `static`!
26+
// .call_once_and_store_result([]() {
27+
// return py::module_::import("module_name");
28+
// })
29+
// .get_stored();
30+
//
31+
// The parameter of `call_once_and_store_result()` must be callable. It can make
32+
// CPython API calls, and in particular, it can temporarily release the GIL.
33+
//
34+
// `T` can be any C++ type, it does not have to involve CPython API types.
35+
//
36+
// The behavior with regard to signals, e.g. `SIGINT` (`KeyboardInterrupt`),
37+
// is not ideal. If the main thread is the one to actually run the `Callable`,
38+
// then a `KeyboardInterrupt` will interrupt it if it is running normal Python
39+
// code. The situation is different if a non-main thread runs the
40+
// `Callable`, and then the main thread starts waiting for it to complete:
41+
// a `KeyboardInterrupt` will not interrupt the non-main thread, but it will
42+
// get processed only when it is the main thread's turn again and it is running
43+
// normal Python code. However, this will be unnoticeable for quick call-once
44+
// functions, which is usually the case.
45+
template <typename T>
46+
class gil_safe_call_once_and_store {
47+
public:
48+
// PRECONDITION: The GIL must be held when `call_once_and_store_result()` is called.
49+
template <typename Callable>
50+
gil_safe_call_once_and_store &call_once_and_store_result(Callable &&fn) {
51+
if (!is_initialized_) { // This read is guarded by the GIL.
52+
// Multiple threads may enter here, because the GIL is released in the next line and
53+
// CPython API calls in the `fn()` call below may release and reacquire the GIL.
54+
gil_scoped_release gil_rel; // Needed to establish lock ordering.
55+
std::call_once(once_flag_, [&] {
56+
// Only one thread will ever enter here.
57+
gil_scoped_acquire gil_acq;
58+
::new (storage_) T(fn()); // fn may release, but will reacquire, the GIL.
59+
is_initialized_ = true; // This write is guarded by the GIL.
60+
});
61+
// All threads will observe `is_initialized_` as true here.
62+
}
63+
// Intentionally not returning `T &` to ensure the calling code is self-documenting.
64+
return *this;
65+
}
66+
67+
// This must only be called after `call_once_and_store_result()` was called.
68+
T &get_stored() {
69+
assert(is_initialized_);
70+
PYBIND11_WARNING_PUSH
71+
#if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5
72+
// Needed for gcc 4.8.5
73+
PYBIND11_WARNING_DISABLE_GCC("-Wstrict-aliasing")
74+
#endif
75+
return *reinterpret_cast<T *>(storage_);
76+
PYBIND11_WARNING_POP
77+
}
78+
79+
constexpr gil_safe_call_once_and_store() = default;
80+
PYBIND11_DTOR_CONSTEXPR ~gil_safe_call_once_and_store() = default;
81+
82+
private:
83+
alignas(T) char storage_[sizeof(T)] = {};
84+
std::once_flag once_flag_ = {};
85+
bool is_initialized_ = false;
86+
// The `is_initialized_`-`storage_` pair is very similar to `std::optional`,
87+
// but the latter does not have the triviality properties of former,
88+
// therefore `std::optional` is not a viable alternative here.
89+
};
90+
91+
PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)

include/pybind11/numpy.h

+13-6
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,10 @@
1010
#pragma once
1111

1212
#include "pybind11.h"
13+
#include "detail/common.h"
1314
#include "complex.h"
15+
#include "gil_safe_call_once.h"
16+
#include "pytypes.h"
1417

1518
#include <algorithm>
1619
#include <array>
@@ -206,8 +209,8 @@ struct npy_api {
206209
};
207210

208211
static npy_api &get() {
209-
static npy_api api = lookup();
210-
return api;
212+
PYBIND11_CONSTINIT static gil_safe_call_once_and_store<npy_api> storage;
213+
return storage.call_once_and_store_result(lookup).get_stored();
211214
}
212215

213216
bool PyArray_Check_(PyObject *obj) const {
@@ -643,10 +646,14 @@ class dtype : public object {
643646
char flags() const { return detail::array_descriptor_proxy(m_ptr)->flags; }
644647

645648
private:
646-
static object _dtype_from_pep3118() {
647-
module_ m = detail::import_numpy_core_submodule("_internal");
648-
static PyObject *obj = m.attr("_dtype_from_pep3118").cast<object>().release().ptr();
649-
return reinterpret_borrow<object>(obj);
649+
static object &_dtype_from_pep3118() {
650+
PYBIND11_CONSTINIT static gil_safe_call_once_and_store<object> storage;
651+
return storage
652+
.call_once_and_store_result([]() {
653+
return detail::import_numpy_core_submodule("_internal")
654+
.attr("_dtype_from_pep3118");
655+
})
656+
.get_stored();
650657
}
651658

652659
dtype strip_padding(ssize_t itemsize) {

tests/extra_python_package/test_files.py

+1
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
"include/pybind11/eval.h",
3636
"include/pybind11/functional.h",
3737
"include/pybind11/gil.h",
38+
"include/pybind11/gil_safe_call_once.h",
3839
"include/pybind11/iostream.h",
3940
"include/pybind11/numpy.h",
4041
"include/pybind11/operators.h",

tests/test_exceptions.cpp

+6-2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
All rights reserved. Use of this source code is governed by a
77
BSD-style license that can be found in the LICENSE file.
88
*/
9+
#include <pybind11/gil_safe_call_once.h>
10+
911
#include "test_exceptions.h"
1012

1113
#include "local_bindings.h"
@@ -114,15 +116,17 @@ TEST_SUBMODULE(exceptions, m) {
114116
[]() { throw std::runtime_error("This exception was intentionally thrown."); });
115117

116118
// PLEASE KEEP IN SYNC with docs/advanced/exceptions.rst
117-
static py::handle ex = py::exception<MyException>(m, "MyException").release();
119+
PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store<py::object> ex_storage;
120+
ex_storage.call_once_and_store_result(
121+
[&]() { return py::exception<MyException>(m, "MyException"); });
118122
py::register_exception_translator([](std::exception_ptr p) {
119123
try {
120124
if (p) {
121125
std::rethrow_exception(p);
122126
}
123127
} catch (const MyException &e) {
124128
// Set MyException as the active python error
125-
py::set_error(ex, e.what());
129+
py::set_error(ex_storage.get_stored(), e.what());
126130
}
127131
});
128132

0 commit comments

Comments
 (0)