pytypes: Resurrect via leak instead of un-finalize in Python 3.8

Author: EricCousineau-TRI

include/pybind11/pytypes.h

@@ -1422,7 +1422,7 @@ class wrapper : public Base {
       }
       holder_type_id_ = holder_type_id;
       patient_ = std::move(patient);
-      // @note It would be nice to put `revive_python3` here, but this is called by
+      // @note It would be nice to put `resurrect_python3` here, but this is called by
       // `PyObject_CallFinalizer`, which will end up reversing its effect anyways.
   }
 
@@ -1431,7 +1431,7 @@ class wrapper : public Base {
       if (!lives_in_cpp()) {
           throw std::runtime_error("Instance does not live in C++");
       }
-      revive_python3();
+      resurrect_python3();
       // Remove existing reference.
       object tmp = std::move(patient_);
       assert(!patient_);
@@ -1462,16 +1462,27 @@ class wrapper : public Base {
       }
   }
 
-  // Python3 unfortunately will not implicitly call `__del__` multiple times,
-  // even if the object is resurrected. This is a dirty workaround.
-  // @see https://bugs.python.org/issue32377
-  inline void revive_python3() {
-#if PY_VERSION_HEX >= 0x03000000
-      // Reverse single-finalization constraint in Python3.
-      if (_PyGC_FINALIZED(patient_.ptr())) {
+  // Handle PEP 442, implemented in Python3, where resurrection more than once
+  // is a bit more dicey.
+  inline void resurrect_python3() {
+#if PY_VERSION_HEX >= 0x03080000
+    // Leak it as a means to stay alive for now.
+    // See: https://bugs.python.org/issue40240
+    if (_PyGC_FINALIZED(patient_.ptr())) {
+        if (leaked_) {
+          throw std::runtime_error("__del__ called twice in Python 3.8+?");
+        }
+        leaked_ = true;
+        patient_.inc_ref();
+    }
+#elif PY_VERSION_HEX >= 0x03000000
+    // Reverse single-finalization constraint in Python3.
+    // This was a really dirty workaround:
+    // See: https://bugs.python.org/issue32377
+    if (_PyGC_FINALIZED(patient_.ptr())) {
         _PyGC_SET_FINALIZED(patient_.ptr(), 0);
-      }
-#endif  // PY_VERSION_HEX >= 0x03000000
+    }
+#endif  // PY_VERSION_HEX >= 0x03080000
   }
 
  private:
@@ -1483,6 +1494,9 @@ class wrapper : public Base {
 
   object patient_;
   detail::HolderTypeId holder_type_id_{detail::HolderTypeId::Unknown};
+#if PY_VERSION_HEX >= 0x03080000
+  bool leaked_{false};
+#endif  // PY_VERSION_HEX >= 0x03080000
 };
 
 NAMESPACE_BEGIN(detail)

tests/test_ownership_transfer.py

@@ -2,6 +2,7 @@
 from pybind11_tests import ConstructorStats
 
 import pytest
+import sys
 import weakref
 
 
@@ -44,7 +45,15 @@ def get_cstats():
 # capturing deletion properly.
 @pytest.unsupported_on_pypy
 def test_shared_ptr_derived_slicing(capture):
-    from sys import getrefcount
+    leaked_count = [0]
+    is_py38 = sys.version_info[:2] >= (3, 8)
+
+    def py38_leak():
+        if is_py38:
+            leaked_count[0] += 1
+
+    def cstats_alive_except_leaked():
+        return cstats.alive() - leaked_count[0]
 
     # [ Bad ]
     cstats = ChildBad.get_cstats()
@@ -75,42 +84,48 @@ def test_shared_ptr_derived_slicing(capture):
     assert cstats.alive() == 1
     assert obj.value() == 100
     del obj
-    assert cstats.alive() == 0
+    py38_leak()
+    assert cstats_alive_except_leaked() == 0
     # Use something more permanent.
     obj = Child()  # Use factory method.
     obj_weak = weakref.ref(obj)
-    assert cstats.alive() == 1
+    assert cstats_alive_except_leaked() == 1
     c = m.BaseContainer(obj)
     del obj
-    assert cstats.alive() == 1
+    assert cstats_alive_except_leaked() == 1
     # We now still have a reference to the object. py::wrapper<> will intercept Python's
     # attempt to destroy `obj`, is aware the `shared_ptr<>.use_count() > 1`, and will increase
     # the ref count by transferring a new reference to `py::wrapper<>` (thus reviving the object,
     # per Python's documentation of __del__).
     assert obj_weak() is not None
-    assert cstats.alive() == 1
+    assert cstats_alive_except_leaked() == 1
     # This goes from C++ -> Python, and then Python -> C++ once this statement has finished.
     assert c.get().value() == 100
-    assert cstats.alive() == 1
+    assert cstats_alive_except_leaked() == 1
     # Destroy references (effectively in C++), and ensure that we have the desired behavior.
     del c
-    assert cstats.alive() == 0
+    py38_leak()
+    assert cstats_alive_except_leaked() == 0
 
     # Ensure that we can pass it from Python -> C++ -> Python, and ensure that C++ does not think
     # that it has ownership.
     obj = Child(10)
     c = m.BaseContainer(obj)
     del obj
-    assert cstats.alive() == 1
+    assert cstats_alive_except_leaked() == 1
     obj = c.get()
     # Now that we have it in Python, there should only be 1 Python reference, since
     # py::wrapper<> in C++ should have released its reference.
-    assert getrefcount(obj) == 2
+    expected_refcount = 2
+    if is_py38:
+        expected_refcount += 1
+    assert sys.getrefcount(obj) == expected_refcount
     del c
-    assert cstats.alive() == 1
+    assert cstats_alive_except_leaked() == 1
     assert obj.value() == 100
     del obj
-    assert cstats.alive() == 0
+    py38_leak()
+    assert cstats_alive_except_leaked() == 0
 
 
 @pytest.unsupported_on_pypy