From 34a118fd36e10702fcc4e8dcc53751de070dc315 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 16 Feb 2025 11:01:41 -0800 Subject: [PATCH] Add `release_gil_before_calling_cpp_dtor` annotation for `class_` (#5522) * Backport of https://github.com/google/pybind11clif/pull/30088 (main PR) and https://github.com/google/pybind11clif/pull/30092 (minor fixes). Note for completeness: These are identical to the current versions on the pybind11clif main branch (@ commit 4841661df5daf26ecdedaace54e64d0782e63f64): * test_class_release_gil_before_calling_cpp_dtor.cpp * test_class_release_gil_before_calling_cpp_dtor.py * Fix potential data race in test_class_release_gil_before_calling_cpp_dtor.cpp The original intent was to let the singleton leak, but making that tread-safe is slightly more involved than this solution. It's totally fine in this case if the RegistryType destructor runs on process teardown. See also: https://github.com/pybind/pybind11/pull/5522#issuecomment-2661068351 --------- Co-authored-by: Ralf W. Grosse-Kunstleve --- include/pybind11/attr.h | 18 ++++++- include/pybind11/pybind11.h | 50 +++++++++++++---- tests/CMakeLists.txt | 1 + ...ss_release_gil_before_calling_cpp_dtor.cpp | 53 +++++++++++++++++++ ...ass_release_gil_before_calling_cpp_dtor.py | 21 ++++++++ 5 files changed, 132 insertions(+), 11 deletions(-) create mode 100644 tests/test_class_release_gil_before_calling_cpp_dtor.cpp create mode 100644 tests/test_class_release_gil_before_calling_cpp_dtor.py diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index 1044db94d..c863d8be5 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -81,6 +81,10 @@ struct dynamic_attr {}; /// Annotation which enables the buffer protocol for a type struct buffer_protocol {}; +/// Annotation which enables releasing the GIL before calling the C++ destructor of wrapped +/// instances (pybind/pybind11#1446). +struct release_gil_before_calling_cpp_dtor {}; + /// Annotation which requests that a special metaclass is created for a type struct metaclass { handle value; @@ -272,7 +276,8 @@ struct function_record { struct type_record { PYBIND11_NOINLINE type_record() : multiple_inheritance(false), dynamic_attr(false), buffer_protocol(false), - default_holder(true), module_local(false), is_final(false) {} + default_holder(true), module_local(false), is_final(false), + release_gil_before_calling_cpp_dtor(false) {} /// Handle to the parent scope handle scope; @@ -331,6 +336,9 @@ struct type_record { /// Is the class inheritable from python classes? bool is_final : 1; + /// Solves pybind/pybind11#1446 + bool release_gil_before_calling_cpp_dtor : 1; + PYBIND11_NOINLINE void add_base(const std::type_info &base, void *(*caster)(void *) ) { auto *base_info = detail::get_type_info(base, false); if (!base_info) { @@ -603,6 +611,14 @@ struct process_attribute : process_attribute_default static void init(const module_local &l, type_record *r) { r->module_local = l.value; } }; +template <> +struct process_attribute + : process_attribute_default { + static void init(const release_gil_before_calling_cpp_dtor &, type_record *r) { + r->release_gil_before_calling_cpp_dtor = true; + } +}; + /// Process a 'prepend' attribute, putting this at the beginning of the overload chain template <> struct process_attribute : process_attribute_default { diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 655bec398..8a1739e19 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1665,7 +1665,6 @@ public: record.type_align = alignof(conditional_t &); record.holder_size = sizeof(holder_type); record.init_instance = init_instance; - record.dealloc = dealloc; record.default_holder = detail::is_instantiation::value; set_operator_new(&record); @@ -1676,6 +1675,12 @@ public: /* Process optional arguments, if any */ process_attributes::init(extra..., &record); + if (record.release_gil_before_calling_cpp_dtor) { + record.dealloc = dealloc_release_gil_before_calling_cpp_dtor; + } else { + record.dealloc = dealloc_without_manipulating_gil; + } + generic_type::initialize(record); if (has_alias) { @@ -1996,15 +2001,14 @@ private: init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr()); } - /// Deallocates an instance; via holder, if constructed; otherwise via operator delete. - static void dealloc(detail::value_and_holder &v_h) { - // We could be deallocating because we are cleaning up after a Python exception. - // If so, the Python error indicator will be set. We need to clear that before - // running the destructor, in case the destructor code calls more Python. - // If we don't, the Python API will exit with an exception, and pybind11 will - // throw error_already_set from the C++ destructor which is forbidden and triggers - // std::terminate(). - error_scope scope; + // Deallocates an instance; via holder, if constructed; otherwise via operator delete. + // NOTE: The Python error indicator needs to cleared BEFORE this function is called. + // This is because we could be deallocating while cleaning up after a Python exception. + // If the error indicator is not cleared but the C++ destructor code makes Python C API + // calls, those calls are likely to generate a new exception, and pybind11 will then + // throw `error_already_set` from the C++ destructor. This is forbidden and will + // trigger std::terminate(). + static void dealloc_impl(detail::value_and_holder &v_h) { if (v_h.holder_constructed()) { v_h.holder().~holder_type(); v_h.set_holder_constructed(false); @@ -2015,6 +2019,32 @@ private: v_h.value_ptr() = nullptr; } + static void dealloc_without_manipulating_gil(detail::value_and_holder &v_h) { + error_scope scope; + dealloc_impl(v_h); + } + + static void dealloc_release_gil_before_calling_cpp_dtor(detail::value_and_holder &v_h) { + error_scope scope; + // Intentionally not using `gil_scoped_release` because the non-simple + // version unconditionally calls `get_internals()`. + // `Py_BEGIN_ALLOW_THREADS`, `Py_END_ALLOW_THREADS` cannot be used + // because those macros include `{` and `}`. + PyThreadState *py_ts = PyEval_SaveThread(); + try { + dealloc_impl(v_h); + } catch (...) { + // This code path is expected to be unreachable unless there is a + // bug in pybind11 itself. + // An alternative would be to mark this function, or + // `dealloc_impl()`, with `nothrow`, but that would be a subtle + // behavior change and could make debugging more difficult. + PyEval_RestoreThread(py_ts); + throw; + } + PyEval_RestoreThread(py_ts); + } + static detail::function_record *get_function_record(handle h) { h = detail::get_function(h); if (!h) { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 315a5bad0..da396f098 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -115,6 +115,7 @@ set(PYBIND11_TEST_FILES test_callbacks test_chrono test_class + test_class_release_gil_before_calling_cpp_dtor test_const_name test_constants_and_functions test_copy_move diff --git a/tests/test_class_release_gil_before_calling_cpp_dtor.cpp b/tests/test_class_release_gil_before_calling_cpp_dtor.cpp new file mode 100644 index 000000000..a4869a846 --- /dev/null +++ b/tests/test_class_release_gil_before_calling_cpp_dtor.cpp @@ -0,0 +1,53 @@ +#include + +#include "pybind11_tests.h" + +#include +#include + +namespace pybind11_tests { +namespace class_release_gil_before_calling_cpp_dtor { + +using RegistryType = std::unordered_map; + +static RegistryType &PyGILState_Check_Results() { + static RegistryType singleton; // Local static variables have thread-safe initialization. + return singleton; +} + +template // Using int as a trick to easily generate a series of types. +struct ProbeType { +private: + std::string unique_key; + +public: + explicit ProbeType(const std::string &unique_key) : unique_key{unique_key} {} + + ~ProbeType() { + RegistryType ® = PyGILState_Check_Results(); + assert(reg.count(unique_key) == 0); + reg[unique_key] = PyGILState_Check(); + } +}; + +} // namespace class_release_gil_before_calling_cpp_dtor +} // namespace pybind11_tests + +TEST_SUBMODULE(class_release_gil_before_calling_cpp_dtor, m) { + using namespace pybind11_tests::class_release_gil_before_calling_cpp_dtor; + + py::class_>(m, "ProbeType0").def(py::init()); + + py::class_>(m, "ProbeType1", py::release_gil_before_calling_cpp_dtor()) + .def(py::init()); + + m.def("PopPyGILState_Check_Result", [](const std::string &unique_key) -> std::string { + RegistryType ® = PyGILState_Check_Results(); + if (reg.count(unique_key) == 0) { + return "MISSING"; + } + int res = reg[unique_key]; + reg.erase(unique_key); + return std::to_string(res); + }); +} diff --git a/tests/test_class_release_gil_before_calling_cpp_dtor.py b/tests/test_class_release_gil_before_calling_cpp_dtor.py new file mode 100644 index 000000000..0b1f246bb --- /dev/null +++ b/tests/test_class_release_gil_before_calling_cpp_dtor.py @@ -0,0 +1,21 @@ +from __future__ import annotations + +import gc + +import pytest + +from pybind11_tests import class_release_gil_before_calling_cpp_dtor as m + + +@pytest.mark.parametrize( + ("probe_type", "unique_key", "expected_result"), + [ + (m.ProbeType0, "without_manipulating_gil", "1"), + (m.ProbeType1, "release_gil_before_calling_cpp_dtor", "0"), + ], +) +def test_gil_state_check_results(probe_type, unique_key, expected_result): + probe_type(unique_key) + gc.collect() + result = m.PopPyGILState_Check_Result(unique_key) + assert result == expected_result