Skip to content
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

feat: add pybind11/gil_safe_call_once.h (to fix deadlocks in pybind11/numpy.h) #4877

Merged
merged 31 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
38317c3
LazyInitializeAtLeastOnceDestroyNever v1
Oct 9, 2023
e7b8c4f
Go back to using `union` as originally suggested by jbms@. The trick …
Oct 9, 2023
74ac0d9
Revert "Go back to using `union` as originally suggested by jbms@. Th…
Oct 9, 2023
109a165
Remove `#include <stdalign.h>`
Oct 9, 2023
88cec11
Suppress gcc 4.8.5 (CentOS 7) warning.
Oct 9, 2023
1ce2715
Replace comments:
Oct 9, 2023
e7be9c2
Adopt suggestion by @tkoeppe:
Oct 9, 2023
f07b28b
Add `PYBIND11_CONSTINIT`, but it does not work for the current use ca…
Oct 9, 2023
36be645
Revert "Add `PYBIND11_CONSTINIT`, but it does not work for the curren…
Oct 9, 2023
7bc16a6
Reapply "Add `PYBIND11_CONSTINIT`, but it does not work for the curre…
Oct 9, 2023
78f4e93
Add Default Member Initializer on `value_storage_` as suggested by @t…
Oct 9, 2023
6d9441d
Fix copy-paste-missed-a-change mishap in commit 88cec1152ab5576db19ba…
Oct 9, 2023
a864f21
Semi-paranoid placement new (based on https://github.com/pybind/pybin…
Oct 9, 2023
6689b06
Move PYBIND11_CONSTINIT to detail/common.h
Oct 9, 2023
d965f29
Move code to the right places, rename new class and some variables.
Oct 9, 2023
398a42c
Fix oversight: update tests/extra_python_package/test_files.py
Oct 9, 2023
ab2cf8e
Get the name right first.
Oct 9, 2023
6d5bdd8
Use `std::call_once`, `std::atomic`, following a pattern developed by…
Oct 9, 2023
4c5dd1b
Make the API more self-documenting (and possibly more easily reusable).
Oct 9, 2023
8633c5b
google-clang-tidy IWYU fixes
Oct 9, 2023
82f3efc
Rewrite comment as suggested by @tkoeppe
Oct 10, 2023
3ebd139
Update test_exceptions.cpp and exceptions.rst
Oct 10, 2023
c33712d
Fix oversight in previous commit: add `PYBIND11_CONSTINIT`
Oct 10, 2023
dcf2b92
Make `get_stored()` non-const for simplicity.
Oct 10, 2023
4557dce
Add comment regarding `KeyboardInterrupt` behavior, based heavily on …
Oct 10, 2023
704fe13
Add `assert(PyGILState_Check())` in `gil_scoped_release` ctor (simple…
Oct 10, 2023
b2f87a8
Fix oversight in previous commit (missing include cassert).
Oct 10, 2023
fad1017
Remove use of std::atomic, leaving comments with rationale, why it is…
Oct 10, 2023
8453302
Rewrite comment re `std:optional` based on deeper reflection (aka 2nd…
Oct 10, 2023
66bbc67
Additional comment with the conclusion of a discussion under PR #4877.
Oct 11, 2023
7cd9390
Small comment changes suggested by @tkoeppe.
Oct 11, 2023
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 @@ -142,6 +142,7 @@ set(PYBIND11_HEADERS
include/pybind11/embed.h
include/pybind11/eval.h
include/pybind11/gil.h
include/pybind11/gil_safe_call_once.h
include/pybind11/iostream.h
include/pybind11/functional.h
include/pybind11/numpy.h
Expand Down
9 changes: 4 additions & 5 deletions docs/advanced/exceptions.rst
Original file line number Diff line number Diff line change
Expand Up @@ -141,15 +141,14 @@ standard python RuntimeError:

.. code-block:: cpp
// This is a static object, so we must leak the Python reference:
// It is undefined when the destructor will run, possibly only after the
// Python interpreter is finalized already.
static py::handle exc = py::exception<MyCustomException>(m, "MyCustomError").release();
PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store<py::object> exc_storage;
exc_storage.call_once_and_store_result(
[&]() { return py::exception<MyCustomException>(m, "MyCustomError"); });
py::register_exception_translator([](std::exception_ptr p) {
try {
if (p) std::rethrow_exception(p);
} catch (const MyCustomException &e) {
py::set_error(exc, e.what());
py::set_error(exc_storage.get_stored(), e.what());
} catch (const OtherException &e) {
py::set_error(PyExc_RuntimeError, e.what());
}
Expand Down
8 changes: 8 additions & 0 deletions include/pybind11/detail/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,14 @@
# endif
#endif

#if defined(PYBIND11_CPP20)
# define PYBIND11_CONSTINIT constinit
# define PYBIND11_DTOR_CONSTEXPR constexpr
#else
# define PYBIND11_CONSTINIT
# define PYBIND11_DTOR_CONSTEXPR
#endif

// Compiler version assertions
#if defined(__INTEL_COMPILER)
# if __INTEL_COMPILER < 1800
Expand Down
10 changes: 9 additions & 1 deletion include/pybind11/gil.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

#include "detail/common.h"

#include <cassert>

#if defined(WITH_THREAD) && !defined(PYBIND11_SIMPLE_GIL_MANAGEMENT)
# include "detail/internals.h"
#endif
Expand Down Expand Up @@ -137,7 +139,9 @@ class gil_scoped_acquire {

class gil_scoped_release {
public:
// PRECONDITION: The GIL must be held when this constructor is called.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be a good idea to add a debug assertion that the GIL is actually held here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, thanks! I actually globally tested that change in isolation about a week ago already and it passed. Which makes me think: if the precondition is not met the failures are critical, therefore such code never actually makes it into the code base. However, the extra assert() is a great and easy way to guard against accidents when developing new code.

explicit gil_scoped_release(bool disassoc = false) : disassoc(disassoc) {
assert(PyGILState_Check());
// `get_internals()` must be called here unconditionally in order to initialize
// `internals.tstate` for subsequent `gil_scoped_acquire` calls. Otherwise, an
// initialization race could occur as multiple threads try `gil_scoped_acquire`.
Expand Down Expand Up @@ -201,7 +205,11 @@ class gil_scoped_release {
PyThreadState *state;

public:
gil_scoped_release() : state{PyEval_SaveThread()} {}
// PRECONDITION: The GIL must be held when this constructor is called.
gil_scoped_release() {
assert(PyGILState_Check());
state = PyEval_SaveThread();
}
gil_scoped_release(const gil_scoped_release &) = delete;
gil_scoped_release &operator=(const gil_scoped_release &) = delete;
~gil_scoped_release() { PyEval_RestoreThread(state); }
Expand Down
93 changes: 93 additions & 0 deletions include/pybind11/gil_safe_call_once.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
// Copyright (c) 2023 The pybind Community.

#pragma once

#include "detail/common.h"
#include "gil.h"

#include <cassert>
#include <mutex>

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)

// Use the `gil_safe_call_once_and_store` class below instead of the naive
//
// static auto imported_obj = py::module_::import("module_name"); // BAD, DO NOT USE!
//
// which has two serious issues:
//
// 1. Py_DECREF() calls potentially after the Python interpreter was finalized already, and
// 2. deadlocks in multi-threaded processes (because of missing lock ordering).
//
// The following alternative avoids both problems:
//
// PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store<py::object> storage;
// auto &imported_obj = storage // Do NOT make this `static`!
// .call_once_and_store_result([]() {
// return py::module_::import("module_name");
// })
// .get_stored();
//
// The parameter of `call_once_and_store_result()` must be callable. It can make
// CPython API calls, and in particular, it can temporarily release the GIL.
//
// `T` can be any C++ type, it does not have to involve CPython API types.
//
// The behavior with regard to signals, e.g. `SIGINT` (`KeyboardInterrupt`),
// is not ideal. If the main thread is the one to actually run the `Callable`,
// then a `KeyboardInterrupt` will interrupt it if it is running normal Python
// code. The situation is different if a non-main thread runs the
// `Callable`, and then the main thread starts waiting for it to complete:
// a `KeyboardInterrupt` will not interrupt the non-main thread, but it will
// get processed only when it is the main thread's turn again and it is running
// normal Python code. However, this will be unnoticeable for quick call-once
// functions, which is usually the case.
template <typename T>
class gil_safe_call_once_and_store {
public:
// PRECONDITION: The GIL must be held when `call_once_and_store_result()` is called.
template <typename Callable>
gil_safe_call_once_and_store &call_once_and_store_result(Callable &&fn) {
if (!is_initialized_) { // This read is guarded by the GIL.
// Multiple threads may enter here, because CPython API calls in the
// `fn()` call below may release and reacquire the GIL.
gil_scoped_release gil_rel; // Needed to establish lock ordering.
std::call_once(once_flag_, [&] {
// Only one thread will ever enter here.
gil_scoped_acquire gil_acq;
::new (storage_) T(fn());
is_initialized_ = true; // This write is guarded by the GIL.
});
// The C++ standard guarantees that all threads entering above will observe
// `is_initialized_` as true here.
}
// Intentionally not returning `T &` to ensure the calling code is self-documenting.
return *this;
}

// This must only be called after `call_once_and_store_result()` was called.
// Not const for simplicity. (Could be made const if there is an unforeseen need.)
T &get_stored() {
assert(is_initialized_);
PYBIND11_WARNING_PUSH
#if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5
// Needed for gcc 4.8.5
PYBIND11_WARNING_DISABLE_GCC("-Wstrict-aliasing")
#endif
return *reinterpret_cast<T *>(storage_);
PYBIND11_WARNING_POP
}

constexpr gil_safe_call_once_and_store() = default;
PYBIND11_DTOR_CONSTEXPR ~gil_safe_call_once_and_store() = default;

private:
alignas(T) char storage_[sizeof(T)] = {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would make this a std::optional and avoid both the atomic flag and the storage here.

You don't need the atomic since call_once is an inherent mutex that asserts ordering.

Copy link
Contributor

@tkoeppe tkoeppe Oct 10, 2023

Choose a reason for hiding this comment

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

It's not the access inside the call_once that the atomic is for, but rather, the earlier access (the first check for the fast path).

However, I think you have a point that because we assume that the GIL is held on function entry, the boolean doesn't need to be atomic: the GIL serializes access to it (both read and write). The code with the atomic variable would work even without any external synchronisation, but since we already have the GIL on entry, I agree that we should be able to use a non-atomic boolean.

(And then the boolean + placement new could be replaced by std::optional, indeed.)

Copy link
Contributor

Choose a reason for hiding this comment

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

And just to make this point clearly: the first, outer boolean check provides a fast path on which we don't interact with the GIL any further. We only do the release and reacquire dance on the (hopefully rare) slow path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks Thomas! I'll definitely make the change to the simple bool then.

could be replaced by std::optional

Except that pybind11 still supports C++11, and std::optional was added only with C++17.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case leave it as bool + placement new :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, and also we do actually want the "constant-initializable, trivial destructible" behaviour of bool + placement new. Not only do we not want to destroy this object, but this also gives us cheaper static variables (no static guard, and no global destructructor list entry).

std::once_flag once_flag_ = {};
bool is_initialized_ = false;
// The `is_initialized_`-`storage_` pair is very similar to `std::optional`,
// but the latter does not have the triviality properties of former,
// therefore `std::optional` is not a viable alternative here.
};

PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
19 changes: 13 additions & 6 deletions include/pybind11/numpy.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@
#pragma once

#include "pybind11.h"
#include "detail/common.h"
#include "complex.h"
#include "gil_safe_call_once.h"
#include "pytypes.h"

#include <algorithm>
#include <array>
Expand Down Expand Up @@ -206,8 +209,8 @@ struct npy_api {
};

static npy_api &get() {
static npy_api api = lookup();
return api;
PYBIND11_CONSTINIT static gil_safe_call_once_and_store<npy_api> storage;
return storage.call_once_and_store_result(lookup).get_stored();
}

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

private:
static object _dtype_from_pep3118() {
module_ m = detail::import_numpy_core_submodule("_internal");
static PyObject *obj = m.attr("_dtype_from_pep3118").cast<object>().release().ptr();
return reinterpret_borrow<object>(obj);
static object &_dtype_from_pep3118() {
PYBIND11_CONSTINIT static gil_safe_call_once_and_store<object> storage;
return storage
.call_once_and_store_result([]() {
return detail::import_numpy_core_submodule("_internal")
.attr("_dtype_from_pep3118");
})
.get_stored();
}

dtype strip_padding(ssize_t itemsize) {
Expand Down
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 @@ -35,6 +35,7 @@
"include/pybind11/eval.h",
"include/pybind11/functional.h",
"include/pybind11/gil.h",
"include/pybind11/gil_safe_call_once.h",
"include/pybind11/iostream.h",
"include/pybind11/numpy.h",
"include/pybind11/operators.h",
Expand Down
8 changes: 6 additions & 2 deletions tests/test_exceptions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
All rights reserved. Use of this source code is governed by a
BSD-style license that can be found in the LICENSE file.
*/
#include <pybind11/gil_safe_call_once.h>

#include "test_exceptions.h"

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

// PLEASE KEEP IN SYNC with docs/advanced/exceptions.rst
static py::handle ex = py::exception<MyException>(m, "MyException").release();
PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store<py::object> ex_storage;
ex_storage.call_once_and_store_result(
[&]() { return py::exception<MyException>(m, "MyException"); });
py::register_exception_translator([](std::exception_ptr p) {
try {
if (p) {
std::rethrow_exception(p);
}
} catch (const MyException &e) {
// Set MyException as the active python error
py::set_error(ex, e.what());
py::set_error(ex_storage.get_stored(), e.what());
}
});

Expand Down