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

git merge smart_holder #30069

Merged
merged 17 commits into from
Oct 12, 2023
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
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1053,7 +1053,7 @@ jobs:
uses: jwlawson/actions-setup-cmake@v1.14

- name: Install ninja-build tool
uses: seanmiddleditch/gha-setup-ninja@v3
uses: seanmiddleditch/gha-setup-ninja@v4

- name: Run pip installs
run: |
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/ci_sh_def.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1080,7 +1080,7 @@ jobs:
uses: jwlawson/actions-setup-cmake@v1.14

- name: Install ninja-build tool
uses: seanmiddleditch/gha-setup-ninja@v3
uses: seanmiddleditch/gha-setup-ninja@v4

- name: Run pip installs
run: |
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/ci_sh_def.yml.patch
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
--- ci.yml 2023-09-10 10:49:38.655616237 -0700
+++ ci_sh_def.yml 2023-09-10 10:50:17.855597833 -0700
--- ci.yml 2023-10-11 21:07:36.619964634 -0700
+++ ci_sh_def.yml 2023-10-11 21:10:12.551698440 -0700
@@ -1,4 +1,16 @@
-name: CI
+# PLEASE KEEP THIS GROUP OF FILES IN SYNC AT ALL TIMES:
Expand Down
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,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/native_enum.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.
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
91 changes: 91 additions & 0 deletions include/pybind11/gil_safe_call_once.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
// 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 the GIL is released in the next line and
// 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()); // fn may release, but will reacquire, the GIL.
is_initialized_ = true; // This write is guarded by the GIL.
});
// All threads 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.
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)] = {};
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/native_enum.h",
"include/pybind11/numpy.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