Skip to content

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

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.

Sign up for GitHub

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 31 commits into from
Oct 12, 2023
Merged

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

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

#include <algorithm>
#include <array>
#include <cassert>
#include <cstdint>
#include <cstdlib>
#include <cstring>
Expand Down Expand Up @@ -42,6 +43,46 @@ class array; // Forward declaration

PYBIND11_NAMESPACE_BEGIN(detail)

// Main author of this class: jbms@
template <typename T>
class LazyInitializeAtLeastOnceDestroyNever {
public:
// PRECONDITION: The GIL must be held when `Get()` is called.
// It is possible that multiple threads execute `Get()` with `initialized_`
// still being false, and thus proceed to execute `initialize()`. This can
// happen if `initialize()` releases and reacquires the GIL internally.
// We accept this, and expect the operation to be both idempotent and cheap.
template <typename Initialize>
Copy link
Contributor

Choose a reason for hiding this comment

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

Document that you expect the GIL to be held, or in any case that calls are serialized. (Otherwise the mutating accesses would cause races.)

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: 1ce2715

T &Get(Initialize &&initialize) {
if (!initialized_) {
assert(PyGILState_Check());
auto value = initialize();
if (!initialized_) {
new (reinterpret_cast<T *>(value_storage_)) T(std::move(value));
Copy link
Contributor

Choose a reason for hiding this comment

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

The inner cast is bogus. Placement-new takes a void*, so you should at best cast to void*. However, this conversion is also available implicitly, and so unless you want to avoid hitting an overloaded placement new operator, you can just drop it:

new (value_storage_) T(args...);   // OK
::new (static_cast<void*>(value_storage_)) T(args...);  // Paranoidly correct

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will be commit a864f21 (see below). Sorry I missed this before triggering the currently running GHA.

I'll work on moving things to the right places before running GHA again.

    Semi-paranoid placement new (based on https://github.com/pybind/pybind11/pull/4877#discussion_r1350573114).

diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h
index 47050a36..1c76177f 100644
--- a/include/pybind11/numpy.h
+++ b/include/pybind11/numpy.h
@@ -66,7 +66,7 @@ public:
             assert(PyGILState_Check());
             auto value = initialize();
             if (!initialized_) {
-                new (reinterpret_cast<T *>(value_storage_)) T(std::move(value));
+                ::new (value_storage_) T(std::move(value));
                 initialized_ = true;
             }
         }

initialized_ = true;
}
}
PYBIND11_WARNING_PUSH
#if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5
// Needed for gcc 4.8.5
PYBIND11_WARNING_DISABLE_CLANG("-Wstrict-aliasing")
#endif
return *reinterpret_cast<T *>(value_storage_);
PYBIND11_WARNING_POP
}

constexpr LazyInitializeAtLeastOnceDestroyNever() = default;
#if __cplusplus >= 202002L
constexpr
#endif // C++20
~LazyInitializeAtLeastOnceDestroyNever()
= default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this go on a single line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only with

// clang-format off
...
/// clang-format on

unfortunately. I feel it's better to just let clang-format do what it wants, but let me know if you prefer the override.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh OK, sure, never mind!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found a nicer solution: PYBIND11_DTOR_CONSTEXPR


private:
alignas(T) char value_storage_[sizeof(T)];
bool initialized_ = false;
};

template <>
struct handle_type_name<array> {
static constexpr auto name = const_name("numpy.ndarray");
Expand Down Expand Up @@ -206,8 +247,8 @@ struct npy_api {
};

static npy_api &get() {
static npy_api api = lookup();
return api;
static LazyInitializeAtLeastOnceDestroyNever<npy_api> api_init;
return api_init.Get(lookup);
}

bool PyArray_Check_(PyObject *obj) const {
Expand Down Expand Up @@ -643,10 +684,11 @@ 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() {
static detail::LazyInitializeAtLeastOnceDestroyNever<object> imported_obj;
return imported_obj.Get([]() {
return detail::import_numpy_core_submodule("_internal").attr("_dtype_from_pep3118");
});
}

dtype strip_padding(ssize_t itemsize) {
Expand Down