From be78aca5cb6c59492e1948b6586c640638bbbfc5 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Tue, 17 Oct 2023 16:01:10 -0400 Subject: [PATCH] show unique_ptr and shared_ptr gc behavior with containers and keep_alive --- tests/test_smart_ptr.cpp | 81 ++++++++++++++++++++++++++++++++++------ tests/test_smart_ptr.py | 58 ++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 12 deletions(-) diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 38b941d1a2..b75963d33f 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -468,9 +468,30 @@ TEST_SUBMODULE(smart_ptr, m) { int value_{}; }; py::class_(m, "UniquePtrHeld") - .def(py::init()) + .def(py::init(), py::arg("value")) .def("value", &UniquePtrHeld::value); + class SharedPtrHeld { + public: + SharedPtrHeld() = delete; + SharedPtrHeld(const SharedPtrHeld&) = delete; + SharedPtrHeld(SharedPtrHeld&&) = delete; + + SharedPtrHeld(int value) + : value_(value) { + print_created(this, value); + } + ~SharedPtrHeld() { + print_destroyed(this); + } + int value() const { return value_; } + private: + int value_{}; + }; + py::class_>(m, "SharedPtrHeld") + .def(py::init(), py::arg("value")) + .def("value", &SharedPtrHeld::value); + class UniquePtrOther {}; py::class_(m, "UniquePtrOther") .def(py::init<>()); @@ -534,40 +555,76 @@ TEST_SUBMODULE(smart_ptr, m) { return out; }); - // Ensure class is non-empty, so it's easier to detect double-free - // corruption. (If empty, this may be harder to see easily.) - struct SharedPtrHeld { int value = 10; }; - py::class_>(m, "SharedPtrHeld") - .def(py::init<>()); m.def("shared_ptr_held_in_unique_ptr", []() { - return std::unique_ptr(new SharedPtrHeld()); + return std::unique_ptr(new SharedPtrHeld(10)); }); m.def("shared_ptr_held_func", [](std::shared_ptr obj) { - return obj != nullptr && obj->value == 10; + return obj != nullptr && obj->value() == 10; }); // Test passing ownership of registered, but unowned, C++ instances back to // Python. This happens when a raw pointer is passed first, and then // ownership is transfered. struct UniquePtrHeldContainer { + using Ptr = std::unique_ptr; + UniquePtrHeldContainer() { - value_.reset(new UniquePtrHeld(10)); + value_ = std::make_unique(10); } + UniquePtrHeldContainer(Ptr value) : value_(std::move(value)) {} + UniquePtrHeld* get() const { return value_.get(); } - using Ptr = std::unique_ptr; Ptr reset(Ptr to) { Ptr from = std::move(value_); value_ = std::move(to); return from; } - std::unique_ptr value_; + + Ptr value_; }; py::class_(m, "UniquePtrHeldContainer") .def(py::init()) + // Note: Keep alive at this point is only important if we care about the *Python* portion + // of an object (e.g., maintaining identity, or preserving keep alive relationships). + // For vanilla C++ behavior, having the Python view of the object be deleted should be "ok". + .def(py::init>(), py::arg("value")) .def("get", &UniquePtrHeldContainer::get, py::return_value_policy::reference_internal) - .def("reset", &UniquePtrHeldContainer::reset); + .def("reset", &UniquePtrHeldContainer::reset, py::arg("to"), py::keep_alive<2, 1>()); + + struct SharedPtrHeldContainer { + using Ptr = std::shared_ptr; + + SharedPtrHeldContainer() { + value_ = std::make_shared(10); + } + SharedPtrHeldContainer(Ptr value) : value_(std::move(value)) {} + + Ptr get() const { + return value_; + } + Ptr reset(Ptr to) { + Ptr from = std::move(value_); + value_ = std::move(to); + return from; + } + Ptr value_; + }; + py::class_(m, "SharedPtrHeldContainer") + .def(py::init()) + // Same as above - without an explicit keep_alive here, the Python portion of the object + // can be refcount gc'd, and thus cause keep_alive() against that object to be lost. + .def(py::init>(), py::arg("value")) + .def("get", &SharedPtrHeldContainer::get) + .def("reset", &SharedPtrHeldContainer::reset, py::arg("to")); + + m.def( + "keep_alive_impl", + [](py::handle nurse, py::handle patient) { + keep_alive_impl(nurse, patient); + }, + py::arg("nurse"), py::arg("patient")); } diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index ce07ecf952..a10ce39e33 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- import pytest +import weakref m = pytest.importorskip("pybind11_tests.smart_ptr") from pybind11_tests import ConstructorStats # noqa: E402 @@ -381,3 +382,60 @@ def check_reset(obj_new): check_reset(obj_new=m.UniquePtrHeld(10)) check_reset(obj_new=None) + + +def test_unique_ptr_held_container_from_cpp_refcount(): + + def make_container(): + # N.B. This only tests the general behavior of a C++-only view of a + # Python object. + obj = m.UniquePtrHeld(50) + c = m.UniquePtrHeldContainer(obj) + obj_ref = weakref.ref(obj) + return c, obj_ref + + c, obj_ref = make_container() + assert obj_ref() is None + obj = c.get() + assert obj is c.get() + + +class CustomPatient: + pass + + +def test_shared_ptr_held_container_from_cpp_refcount_with_keep_alive(): + + def make_container(): + # This tests the general behavior of losing a Python view of a C++ + # object, in addition to confirming that existing keep_alive + # relationships are lost once that Python view is gc'd. + obj = m.SharedPtrHeld(50) + c = m.SharedPtrHeldContainer(obj) + obj_ref = weakref.ref(obj) + obj_id = id(obj) + + # Simulate `py::keep_alive<>` annotation. + patient = CustomPatient() + m.keep_alive_impl(nurse=obj, patient=patient) + patient_ref = weakref.ref(patient) + + return c, obj_ref, obj_id, patient_ref + + c, obj_ref, obj_id_original, patient_ref = make_container() + # Expected. The Python view of `obj` is gc'd, but the actual C++ object is + # kept. + assert obj_ref() is None + # Show that the keep alive relationship is lost, thus the patient itself is + # gc'd. + assert patient_ref() is None + + # Show that we can get a new Python view of the object, and that if we keep + # a reference to that object, the identity is preserved. + obj = c.get() + assert obj is c.get() + # Reinforce that this is a new view of the original object by showing that + # the id() (in this case, the address) of the Python view is not the same + # as it originally was. + # (The C++ pointer value, however, should be the same.) + assert id(obj) != obj_id_original