Add release_gil_before_calling_cpp_dtor annotation for class_ (#5522

) * Backport of google/pybind11clif#30088 (main PR) and google/pybind11clif#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: #5522 (comment) --------- Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]>
pybind · Feb 16, 2025 · 34a118f · 34a118f
1 parent c316cf3
commit 34a118f
Show file tree

Hide file tree

Showing 5 changed files with 132 additions and 11 deletions.
diff --git 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<module_local> : process_attribute_default<module_local>
     static void init(const module_local &l, type_record *r) { r->module_local = l.value; }
 };
 
+template <>
+struct process_attribute<release_gil_before_calling_cpp_dtor>
+    : process_attribute_default<release_gil_before_calling_cpp_dtor> {
+    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<prepend> : process_attribute_default<prepend> {

diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
@@ -1665,7 +1665,6 @@ class class_ : public detail::generic_type {
         record.type_align = alignof(conditional_t<has_alias, type_alias, type> &);
         record.holder_size = sizeof(holder_type);
         record.init_instance = init_instance;
-        record.dealloc = dealloc;
         record.default_holder = detail::is_instantiation<std::unique_ptr, holder_type>::value;
 
         set_operator_new<type>(&record);
@@ -1676,6 +1675,12 @@ class class_ : public detail::generic_type {
         /* Process optional arguments, if any */
         process_attributes<Extra...>::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 @@ class class_ : public detail::generic_type {
         init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr<type>());
     }
 
-    /// 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>().~holder_type();
             v_h.set_holder_constructed(false);
@@ -2015,6 +2019,32 @@ class class_ : public detail::generic_type {
         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
@@ -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
@@ -0,0 +1,53 @@
+#include <pybind11/pybind11.h>
+
+#include "pybind11_tests.h"
+
+#include <string>
+#include <unordered_map>
+
+namespace pybind11_tests {
+namespace class_release_gil_before_calling_cpp_dtor {
+
+using RegistryType = std::unordered_map<std::string, int>;
+
+static RegistryType &PyGILState_Check_Results() {
+    static RegistryType singleton; // Local static variables have thread-safe initialization.
+    return singleton;
+}
+
+template <int> // 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 &reg = 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_<ProbeType<0>>(m, "ProbeType0").def(py::init<std::string>());
+
+    py::class_<ProbeType<1>>(m, "ProbeType1", py::release_gil_before_calling_cpp_dtor())
+        .def(py::init<std::string>());
+
+    m.def("PopPyGILState_Check_Result", [](const std::string &unique_key) -> std::string {
+        RegistryType &reg = 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
@@ -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