Clear existing patients when ownership is reclaimed by pybind.

Author: EricCousineau-TRI

docs/advanced/smart_ptrs.rst

@@ -55,6 +55,10 @@ calling. You *may* return ownership back to pybind by casting the object, as so:
 
 If this is done, then you may continue referencing the object in Python.
 
+When Pybind regains ownership of a Python object, it will detach any existing
+``keep_alive`` behavior, since this is commonly used for containers that
+must be kept alive because they would destroy the object that they owned.
+
 std::shared_ptr
 ===============
 

include/pybind11/detail/class.h

@@ -296,6 +296,8 @@ inline void add_patient(PyObject *nurse, PyObject *patient) {
 
 inline void clear_patients(PyObject *self) {
     auto instance = reinterpret_cast<detail::instance *>(self);
+    if (!instance->has_patients)
+        return;
     auto &internals = get_internals();
     auto pos = internals.patients.find(self);
     assert(pos != internals.patients.end());

include/pybind11/pybind11.h

@@ -1092,6 +1092,9 @@ class class_ : public detail::generic_type {
         new (&v_h.holder<holder_type>()) holder_type(std::move(external_holder));
         v_h.set_holder_constructed();
         v_h.inst->owned = true;
+        // If this instance is now owend by pybind, release any existing
+        // patients (owners for `reference_internal`).
+        detail::clear_patients((PyObject*)v_h.inst);
     }
 
     template <typename Base, detail::enable_if_t<is_base<Base>::value, int> = 0>

tests/test_smart_ptr.cpp

@@ -56,6 +56,48 @@ class custom_unique_ptr {
 PYBIND11_DECLARE_HOLDER_TYPE(T, custom_unique_ptr<T>);
 
 
+enum class KeepAliveType : int {
+    Plain = 0,
+    KeepAlive,
+};
+
+template <
+    typename T,
+    KeepAliveType keep_alive_type>
+class Container {
+public:
+    using Ptr = std::unique_ptr<T>;
+    Container(Ptr ptr)
+        : ptr_(std::move(ptr)) {
+        print_created(this);
+    }
+    ~Container() {
+        print_destroyed(this);
+    }
+    T* get() const { return ptr_.get(); }
+    Ptr release() {
+        return std::move(ptr_);
+    }
+    void reset(Ptr ptr) {
+        ptr_ = std::move(ptr);
+    }
+
+    static void def(py::module &m, const std::string& name) {
+        py::class_<Container> cls(m, name.c_str());
+        if (keep_alive_type == KeepAliveType::KeepAlive) {
+            cls.def(py::init<Ptr>(), py::keep_alive<2, 1>());
+        } else {
+            cls.def(py::init<Ptr>());
+        }
+        // TODO: Figure out why reference_internal does not work???
+        cls.def("get", &Container::get, py::keep_alive<0, 1>()); //py::return_value_policy::reference_internal);
+        cls.def("release", &Container::release);
+        cls.def("reset", &Container::reset);
+    }
+private:
+    Ptr ptr_;
+};
+
 TEST_SUBMODULE(smart_ptr, m) {
 
     // test_smart_ptr
@@ -336,4 +378,9 @@ TEST_SUBMODULE(smart_ptr, m) {
         [](std::unique_ptr<UniquePtrHeld> obj) {
             return py::cast(std::move(obj));
         });
+
+    Container<UniquePtrHeld, KeepAliveType::Plain>::def(
+        m, "ContainerPlain");
+    Container<UniquePtrHeld, KeepAliveType::KeepAlive>::def(
+        m, "ContainerKeepAlive");
 }

tests/test_smart_ptr.py

@@ -1,4 +1,5 @@
 import pytest
+import weakref
 from pybind11_tests import smart_ptr as m
 from pybind11_tests import ConstructorStats
 
@@ -250,3 +251,66 @@ def test_unique_ptr_arg():
 
     assert m.unique_ptr_pass_through(None) is None
     m.unique_ptr_terminal(None)
+
+
+def test_unique_ptr_keep_alive():
+    obj_stats = ConstructorStats.get(m.UniquePtrHeld)
+    c_plain_stats = ConstructorStats.get(m.ContainerPlain)
+    c_keep_stats = ConstructorStats.get(m.ContainerKeepAlive)
+
+    # Try with plain container.
+    obj = m.UniquePtrHeld(1)
+    c_plain = m.ContainerPlain(obj)
+    c_plain_wref = weakref.ref(c_plain)
+    assert obj_stats.alive() == 1
+    assert c_plain_stats.alive() == 1
+    del c_plain
+    pytest.gc_collect()
+    # Everything should have died.
+    assert c_plain_wref() is None
+    assert c_plain_stats.alive() == 0
+    assert obj_stats.alive() == 0
+    del obj
+
+    # Ensure keep_alive via `reference_internal` still works.
+    obj = m.UniquePtrHeld(2)
+    c_plain = m.ContainerPlain(obj)
+    assert c_plain.get() is obj  # Trigger keep_alive
+    assert obj_stats.alive() == 1
+    assert c_plain_stats.alive() == 1
+    del c_plain
+    pytest.gc_collect()
+    assert obj_stats.alive() == 1
+    assert c_plain_stats.alive() == 1
+    del obj
+    pytest.gc_collect()
+    assert obj_stats.alive() == 0
+    assert c_plain_stats.alive() == 0
+
+    # Now try with keep-alive container.
+    # Primitive, very non-conservative.
+    obj = m.UniquePtrHeld(3)
+    c_keep = m.ContainerKeepAlive(obj)
+    c_keep_wref = weakref.ref(c_keep)
+    assert obj_stats.alive() == 1
+    assert c_keep_stats.alive() == 1
+    del c_keep
+    pytest.gc_collect()
+    # Everything should have stayed alive.
+    assert c_keep_wref() is not None
+    assert c_keep_stats.alive() == 1
+    assert obj_stats.alive() == 1
+    # Now release the object. This should have released the container as a patient.
+    c_keep_wref().release()
+    pytest.gc_collect()
+    assert obj_stats.alive() == 1
+    assert c_keep_stats.alive() == 0
+
+    # Check with nullptr.
+    c_keep = m.ContainerKeepAlive(None)
+    assert c_keep_stats.alive() == 1
+    obj = c_keep.get()
+    assert obj is None
+    del c_keep
+    pytest.gc_collect()
+    assert c_keep_stats.alive() == 0