diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index d7a608666..dc9570f92 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; + #ifdef PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT holder_enum_t holder_enum_v = holder_enum_t::undefined; #endif @@ -607,6 +615,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 ae3baefaf..a09aafec6 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1909,7 +1909,6 @@ public: record.type_align = alignof(conditional_t &); record.holder_size = sizeof(holder_type); record.init_instance = init_instance; - record.dealloc = dealloc; // A more fitting name would be uses_unique_ptr_holder. record.default_holder = detail::is_instantiation::value; @@ -1934,6 +1933,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) { @@ -2323,15 +2328,14 @@ private: #endif // PYBIND11_SMART_HOLDER_ENABLED - /// 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); @@ -2342,6 +2346,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 67fbcc890..e2fab9c13 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_class_sh_basic test_class_sh_disowning test_class_sh_disowning_mi 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