diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index daf5c2c5..0b8dffda 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -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: | diff --git a/.github/workflows/ci_sh_def.yml b/.github/workflows/ci_sh_def.yml index ddf4b065..404a0515 100644 --- a/.github/workflows/ci_sh_def.yml +++ b/.github/workflows/ci_sh_def.yml @@ -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: | diff --git a/.github/workflows/ci_sh_def.yml.patch b/.github/workflows/ci_sh_def.yml.patch index c70e8b9f..f550cd95 100644 --- a/.github/workflows/ci_sh_def.yml.patch +++ b/.github/workflows/ci_sh_def.yml.patch @@ -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: diff --git a/CMakeLists.txt b/CMakeLists.txt index 6321d6b1..fe0594d7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -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 diff --git a/docs/advanced/exceptions.rst b/docs/advanced/exceptions.rst index 616e2d77..e20f42b5 100644 --- a/docs/advanced/exceptions.rst +++ b/docs/advanced/exceptions.rst @@ -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(m, "MyCustomError").release(); + PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store exc_storage; + exc_storage.call_once_and_store_result( + [&]() { return py::exception(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()); } diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index d31454c2..addaf91a 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -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 diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h index 570a5581..da22f48d 100644 --- a/include/pybind11/gil.h +++ b/include/pybind11/gil.h @@ -11,6 +11,8 @@ #include "detail/common.h" +#include + #if defined(WITH_THREAD) && !defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) # include "detail/internals.h" #endif @@ -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`. @@ -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); } diff --git a/include/pybind11/gil_safe_call_once.h b/include/pybind11/gil_safe_call_once.h new file mode 100644 index 00000000..eaf84d16 --- /dev/null +++ b/include/pybind11/gil_safe_call_once.h @@ -0,0 +1,91 @@ +// Copyright (c) 2023 The pybind Community. + +#pragma once + +#include "detail/common.h" +#include "gil.h" + +#include +#include + +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 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 +class gil_safe_call_once_and_store { +public: + // PRECONDITION: The GIL must be held when `call_once_and_store_result()` is called. + template + 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(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) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 23c38660..8551aa26 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -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 #include @@ -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 storage; + return storage.call_once_and_store_result(lookup).get_stored(); } bool PyArray_Check_(PyObject *obj) const { @@ -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().release().ptr(); - return reinterpret_borrow(obj); + static object &_dtype_from_pep3118() { + PYBIND11_CONSTINIT static gil_safe_call_once_and_store 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) { diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index 24359343..02f23359 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -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", diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index f6d9c215..d3af7ab7 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -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 + #include "test_exceptions.h" #include "local_bindings.h" @@ -114,7 +116,9 @@ 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(m, "MyException").release(); + PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store ex_storage; + ex_storage.call_once_and_store_result( + [&]() { return py::exception(m, "MyException"); }); py::register_exception_translator([](std::exception_ptr p) { try { if (p) { @@ -122,7 +126,7 @@ TEST_SUBMODULE(exceptions, m) { } } 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()); } });