Add release_gil_before_calling_cpp_dtor annotation for class_ (#30088)

* Add `release_gil_before_calling_cpp_dtor` annotation for `class_`. New tests still missing.

* UNRELATED: comment out mingw-w64-${{matrix.env}}-python-scipy

* UNRELATED: remove mingw-w64-${{matrix.env}}-python-scipy

* UNRELATED: remove mingw-w64-${{matrix.env}}-python-scipy (ci_sh_def.yml)

* Add test_class_release_gil_before_calling_cpp_dtor

* Revert "UNRELATED: remove mingw-w64-${{matrix.env}}-python-scipy (ci_sh_def.yml)"

This reverts commit d7a9411.

* Revert "UNRELATED: remove mingw-w64-${{matrix.env}}-python-scipy"

This reverts commit d541d1d.

* Revert "UNRELATED: comment out mingw-w64-${{matrix.env}}-python-scipy"

This reverts commit 7e57bd7.

* Improved code organization and more comments.

* Rewrite source code comment in response to review comment by @rainwoodman

Author: rwgk

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;
@@ -281,7 +285,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;
@@ -340,6 +345,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) {
@@ -636,6 +644,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> {

include/pybind11/pybind11.h

@@ -1831,7 +1831,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;
 
         // A more fitting name would be uses_unique_ptr_holder.
         record.default_holder = detail::is_instantiation<std::unique_ptr, holder_type>::value;
@@ -1844,6 +1843,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) {
@@ -2193,15 +2198,14 @@ class class_ : public detail::generic_type {
             inst, holder_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>().~holder_type();
             v_h.set_holder_constructed(false);
@@ -2212,6 +2216,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) {

tests/CMakeLists.txt

@@ -119,6 +119,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

tests/test_class_release_gil_before_calling_cpp_dtor.cpp

@@ -0,0 +1,51 @@
+#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>;
+
+RegistryType &PyGILState_Check_Results() {
+    static auto *singleton = new RegistryType();
+    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("GetPyGILState_Check_Result", [](const std::string &unique_key) -> std::string {
+        RegistryType &reg = PyGILState_Check_Results();
+        if (reg.count(unique_key) == 0) {
+            return "MISSING";
+        }
+        return std::to_string(reg[unique_key]);
+    });
+}

tests/test_class_release_gil_before_calling_cpp_dtor.py

@@ -0,0 +1,19 @@
+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.GetPyGILState_Check_Result(unique_key)
+    assert result == expected_result