From 52fda9827b44b06b222adf33d8dd888fd3f37da6 Mon Sep 17 00:00:00 2001 From: Dustin Spicuzza Date: Sun, 31 Jan 2021 21:57:45 -0500 Subject: [PATCH 1/3] Attach python lifetime to shared_ptr passed to C++ - Reference cycles are possible as a result, but shared_ptr is already susceptible to this in C++ --- include/pybind11/cast.h | 39 +++++++++++- tests/CMakeLists.txt | 1 + tests/test_trampoline_shared_ptr_cpp_arg.cpp | 55 ++++++++++++++++ tests/test_trampoline_shared_ptr_cpp_arg.py | 66 ++++++++++++++++++++ 4 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 tests/test_trampoline_shared_ptr_cpp_arg.cpp create mode 100644 tests/test_trampoline_shared_ptr_cpp_arg.py diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 7930fb994b..4bb8f1e96b 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -10,6 +10,7 @@ #pragma once +#include "gil.h" #include "pytypes.h" #include "detail/common.h" #include "detail/descr.h" @@ -641,6 +642,42 @@ struct holder_helper { static auto get(const T &p) -> decltype(p.get()) { return p.get(); } }; +/// Another helper class for holders that helps construct derivative holders from +/// the original holder +template +struct holder_retriever { + static auto get_derivative_holder(const value_and_holder &v_h) -> decltype(v_h.template holder()) { + return v_h.template holder(); + } +}; + +template +struct holder_retriever> { + struct shared_ptr_deleter { + // Note: deleter destructor fails on MSVC 2015 and GCC 4.8, so we manually + // call dec_ref here instead + handle ref; + void operator()(T *) { + gil_scoped_acquire gil; + ref.dec_ref(); + } + }; + + static auto get_derivative_holder(const value_and_holder &v_h) -> std::shared_ptr { + // The shared_ptr is always given to C++ code, so construct a new shared_ptr + // that is given a custom deleter. The custom deleter increments the python + // reference count to bind the python instance lifetime with the lifetime + // of the shared_ptr. + // + // This enables things like passing the last python reference of a subclass to a + // C++ function without the python reference dying. + // + // Reference cycles will cause a leak, but this is a limitation of shared_ptr + return std::shared_ptr((T*)v_h.value_ptr(), + shared_ptr_deleter{handle((PyObject*)v_h.inst).inc_ref()}); + } +}; + /// Type caster for holder types like std::shared_ptr, etc. /// The SFINAE hook is provided to help work around the current lack of support /// for smart-pointer interoperability. Please consider it an implementation @@ -683,7 +720,7 @@ struct copyable_holder_caster : public type_caster_base { bool load_value(value_and_holder &&v_h) { if (v_h.holder_constructed()) { value = v_h.value_ptr(); - holder = v_h.template holder(); + holder = holder_retriever::get_derivative_holder(v_h); return true; } throw cast_error("Unable to cast from non-held to held instance (T& to Holder) " diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b00c043ac3..84e31db965 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -132,6 +132,7 @@ set(PYBIND11_TEST_FILES test_stl_binders.cpp test_tagbased_polymorphic.cpp test_thread.cpp + test_trampoline_shared_ptr_cpp_arg.cpp test_union.cpp test_virtual_functions.cpp) diff --git a/tests/test_trampoline_shared_ptr_cpp_arg.cpp b/tests/test_trampoline_shared_ptr_cpp_arg.cpp new file mode 100644 index 0000000000..000124ced2 --- /dev/null +++ b/tests/test_trampoline_shared_ptr_cpp_arg.cpp @@ -0,0 +1,55 @@ +// Copyright (c) 2021 The Pybind Development Team. +// All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +#include "pybind11_tests.h" + +namespace { + +// For testing whether a python subclass of a C++ object dies when the +// last python reference is lost +struct SpBase { + // returns true if the base virtual function is called + virtual bool is_base_used() { return true; } + + SpBase() = default; + SpBase(const SpBase &) = delete; + virtual ~SpBase() = default; +}; + +struct PySpBase : SpBase { + bool is_base_used() override { PYBIND11_OVERRIDE(bool, SpBase, is_base_used); } +}; + +struct SpBaseTester { + std::shared_ptr get_object() const { return m_obj; } + void set_object(std::shared_ptr obj) { m_obj = std::move(obj); } + bool is_base_used() { return m_obj->is_base_used(); } + std::shared_ptr m_obj; +}; + +// For testing that a C++ class without an alias does not retain the python +// portion of the object +struct SpGoAway {}; + +struct SpGoAwayTester { + std::shared_ptr m_obj; +}; + +} // namespace + +TEST_SUBMODULE(trampoline_shared_ptr_cpp_arg, m) { + // For testing whether a python subclass of a C++ object dies when the + // last python reference is lost + + py::class_, PySpBase>(m, "SpBase") + .def(py::init<>()) + .def("is_base_used", &SpBase::is_base_used); + + py::class_(m, "SpBaseTester") + .def(py::init<>()) + .def("get_object", &SpBaseTester::get_object) + .def("set_object", &SpBaseTester::set_object) + .def("is_base_used", &SpBaseTester::is_base_used) + .def_readwrite("obj", &SpBaseTester::m_obj); +} diff --git a/tests/test_trampoline_shared_ptr_cpp_arg.py b/tests/test_trampoline_shared_ptr_cpp_arg.py new file mode 100644 index 0000000000..5fec4366c0 --- /dev/null +++ b/tests/test_trampoline_shared_ptr_cpp_arg.py @@ -0,0 +1,66 @@ +# -*- coding: utf-8 -*- +import pytest + +import pybind11_tests.trampoline_shared_ptr_cpp_arg as m + + +def test_shared_ptr_cpp_arg(): + import weakref + + class PyChild(m.SpBase): + def is_base_used(self): + return False + + tester = m.SpBaseTester() + + obj = PyChild() + objref = weakref.ref(obj) + + # Pass the last python reference to the C++ function + tester.set_object(obj) + del obj + pytest.gc_collect() + + # python reference is still around since C++ has it now + assert objref() is not None + assert tester.is_base_used() is False + assert tester.obj.is_base_used() is False + assert tester.get_object() is objref() + + +def test_shared_ptr_cpp_prop(): + class PyChild(m.SpBase): + def is_base_used(self): + return False + + tester = m.SpBaseTester() + + # Set the last python reference as a property of the C++ object + tester.obj = PyChild() + pytest.gc_collect() + + # python reference is still around since C++ has it now + assert tester.is_base_used() is False + assert tester.obj.is_base_used() is False + + +def test_shared_ptr_arg_identity(): + import weakref + + tester = m.SpBaseTester() + + obj = m.SpBase() + objref = weakref.ref(obj) + + tester.set_object(obj) + del obj + pytest.gc_collect() + + # python reference is still around since C++ has it + assert objref() is not None + assert tester.get_object() is objref() + + # python reference disappears once the C++ object releases it + tester.set_object(None) + pytest.gc_collect() + assert objref() is None From 9b0f75c9448883538837a45b10d03be856fb911a Mon Sep 17 00:00:00 2001 From: Dustin Spicuzza Date: Wed, 3 Feb 2021 02:18:36 -0500 Subject: [PATCH 2/3] Only keep python portion around if there's an alias class --- include/pybind11/cast.h | 8 ++++++++ include/pybind11/detail/common.h | 2 ++ include/pybind11/pybind11.h | 1 + tests/test_trampoline_shared_ptr_cpp_arg.cpp | 9 +++++++++ tests/test_trampoline_shared_ptr_cpp_arg.py | 20 ++++++++++++++++++++ 5 files changed, 40 insertions(+) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 4bb8f1e96b..9ec1d8d40d 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -664,6 +664,14 @@ struct holder_retriever> { }; static auto get_derivative_holder(const value_and_holder &v_h) -> std::shared_ptr { + // If there's no trampoline class, nothing special needed + if (!v_h.inst->has_alias) { + return v_h.template holder>(); + } + + // If there's a trampoline class, ensure the python side of the object doesn't + // die until the C++ portion also dies + // // The shared_ptr is always given to C++ code, so construct a new shared_ptr // that is given a custom deleter. The custom deleter increments the python // reference count to bind the python instance lifetime with the lifetime diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index b08bbc5590..942ce9196b 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -580,6 +580,8 @@ struct instance { bool simple_instance_registered : 1; /// If true, get_internals().patients has an entry for this object bool has_patients : 1; + /// If true, created with an associated alias class (set via `init_instance`) + bool has_alias : 1; /// Initializes all of the above type/values/holders data (but not the instance values themselves) void allocate_layout(); diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 8c7545ba3a..68a91ac31c 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1630,6 +1630,7 @@ class class_ : public detail::generic_type { /// optional pointer to an existing holder to use; if not specified and the instance is /// `.owned`, a new holder will be constructed to manage the value pointer. static void init_instance(detail::instance *inst, const void *holder_ptr) { + inst->has_alias = has_alias; auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(type))); if (!v_h.instance_registered()) { register_instance(inst, v_h.value_ptr(), v_h.type); diff --git a/tests/test_trampoline_shared_ptr_cpp_arg.cpp b/tests/test_trampoline_shared_ptr_cpp_arg.cpp index 000124ced2..754a3dffbf 100644 --- a/tests/test_trampoline_shared_ptr_cpp_arg.cpp +++ b/tests/test_trampoline_shared_ptr_cpp_arg.cpp @@ -52,4 +52,13 @@ TEST_SUBMODULE(trampoline_shared_ptr_cpp_arg, m) { .def("set_object", &SpBaseTester::set_object) .def("is_base_used", &SpBaseTester::is_base_used) .def_readwrite("obj", &SpBaseTester::m_obj); + + // For testing that a C++ class without an alias does not retain the python + // portion of the object + + py::class_>(m, "SpGoAway").def(py::init<>()); + + py::class_(m, "SpGoAwayTester") + .def(py::init<>()) + .def_readwrite("obj", &SpGoAwayTester::m_obj); } diff --git a/tests/test_trampoline_shared_ptr_cpp_arg.py b/tests/test_trampoline_shared_ptr_cpp_arg.py index 5fec4366c0..8d1c39859b 100644 --- a/tests/test_trampoline_shared_ptr_cpp_arg.py +++ b/tests/test_trampoline_shared_ptr_cpp_arg.py @@ -64,3 +64,23 @@ def test_shared_ptr_arg_identity(): tester.set_object(None) pytest.gc_collect() assert objref() is None + + +def test_shared_ptr_goaway(): + import weakref + + tester = m.SpGoAwayTester() + + obj = m.SpGoAway() + objref = weakref.ref(obj) + + assert tester.obj is None + + tester.obj = obj + del obj + pytest.gc_collect() + + # python reference is no longer around + assert objref() is None + # C++ reference is still around + assert tester.obj is not None From 20bcb610287d68974db02b62dc97971160b85e05 Mon Sep 17 00:00:00 2001 From: Dustin Spicuzza Date: Tue, 23 Feb 2021 01:33:30 -0500 Subject: [PATCH 3/3] Only keep alive objects that have an alias class and were constructed by python --- include/pybind11/cast.h | 2 +- include/pybind11/detail/class.h | 2 + include/pybind11/detail/common.h | 4 +- tests/test_trampoline_shared_ptr_cpp_arg.cpp | 17 +++++++- tests/test_trampoline_shared_ptr_cpp_arg.py | 45 ++++++++++++++++++++ 5 files changed, 67 insertions(+), 3 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 9ec1d8d40d..4337ab5c86 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -665,7 +665,7 @@ struct holder_retriever> { static auto get_derivative_holder(const value_and_holder &v_h) -> std::shared_ptr { // If there's no trampoline class, nothing special needed - if (!v_h.inst->has_alias) { + if (!v_h.inst->is_alias) { return v_h.template holder>(); } diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index b9376b4c0b..80ee602513 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -178,6 +178,8 @@ extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, P // This must be a pybind11 instance auto instance = reinterpret_cast(self); + // Mark this instance as an instance of an alias class + instance->is_alias = instance->has_alias; // Ensure that the base __init__ function(s) were called for (const auto &vh : values_and_holders(instance)) { diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 942ce9196b..f8b6c204d0 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -580,8 +580,10 @@ struct instance { bool simple_instance_registered : 1; /// If true, get_internals().patients has an entry for this object bool has_patients : 1; - /// If true, created with an associated alias class (set via `init_instance`) + /// If true, the type of this instance has an associated alias class (set via `init_instance`) bool has_alias : 1; + /// If true, this instance has an associated alias class and was constructed by Python + bool is_alias : 1; /// Initializes all of the above type/values/holders data (but not the instance values themselves) void allocate_layout(); diff --git a/tests/test_trampoline_shared_ptr_cpp_arg.cpp b/tests/test_trampoline_shared_ptr_cpp_arg.cpp index 754a3dffbf..39a41703bc 100644 --- a/tests/test_trampoline_shared_ptr_cpp_arg.cpp +++ b/tests/test_trampoline_shared_ptr_cpp_arg.cpp @@ -12,6 +12,12 @@ struct SpBase { // returns true if the base virtual function is called virtual bool is_base_used() { return true; } + // returns true if there's an associated python instance + bool has_python_instance() { + auto tinfo = py::detail::get_type_info(typeid(SpBase)); + return (bool)py::detail::get_object_handle(this, tinfo); + } + SpBase() = default; SpBase(const SpBase &) = delete; virtual ~SpBase() = default; @@ -25,6 +31,11 @@ struct SpBaseTester { std::shared_ptr get_object() const { return m_obj; } void set_object(std::shared_ptr obj) { m_obj = std::move(obj); } bool is_base_used() { return m_obj->is_base_used(); } + bool has_instance() { return (bool)m_obj; } + bool has_python_instance() { return m_obj && m_obj->has_python_instance(); } + void set_nonpython_instance() { + m_obj = std::make_shared(); + } std::shared_ptr m_obj; }; @@ -44,13 +55,17 @@ TEST_SUBMODULE(trampoline_shared_ptr_cpp_arg, m) { py::class_, PySpBase>(m, "SpBase") .def(py::init<>()) - .def("is_base_used", &SpBase::is_base_used); + .def("is_base_used", &SpBase::is_base_used) + .def("has_python_instance", &SpBase::has_python_instance); py::class_(m, "SpBaseTester") .def(py::init<>()) .def("get_object", &SpBaseTester::get_object) .def("set_object", &SpBaseTester::set_object) .def("is_base_used", &SpBaseTester::is_base_used) + .def("has_instance", &SpBaseTester::has_instance) + .def("has_python_instance", &SpBaseTester::has_python_instance) + .def("set_nonpython_instance", &SpBaseTester::set_nonpython_instance) .def_readwrite("obj", &SpBaseTester::m_obj); // For testing that a C++ class without an alias does not retain the python diff --git a/tests/test_trampoline_shared_ptr_cpp_arg.py b/tests/test_trampoline_shared_ptr_cpp_arg.py index 8d1c39859b..555dcfc1a0 100644 --- a/tests/test_trampoline_shared_ptr_cpp_arg.py +++ b/tests/test_trampoline_shared_ptr_cpp_arg.py @@ -41,7 +41,9 @@ def is_base_used(self): # python reference is still around since C++ has it now assert tester.is_base_used() is False + assert tester.has_python_instance() is True assert tester.obj.is_base_used() is False + assert tester.obj.has_python_instance() is True def test_shared_ptr_arg_identity(): @@ -59,6 +61,7 @@ def test_shared_ptr_arg_identity(): # python reference is still around since C++ has it assert objref() is not None assert tester.get_object() is objref() + assert tester.has_python_instance() is True # python reference disappears once the C++ object releases it tester.set_object(None) @@ -66,6 +69,48 @@ def test_shared_ptr_arg_identity(): assert objref() is None +def test_shared_ptr_alias_nonpython(): + tester = m.SpBaseTester() + + # C++ creates the object, a python instance shouldn't exist + tester.set_nonpython_instance() + assert tester.is_base_used() is True + assert tester.has_instance() is True + assert tester.has_python_instance() is False + + # Now a python instance exists + cobj = tester.get_object() + assert cobj.has_python_instance() + assert tester.has_instance() is True + assert tester.has_python_instance() is True + + # Now it's gone + del cobj + pytest.gc_collect() + assert tester.has_instance() is True + assert tester.has_python_instance() is False + + # When we pass it as an arg to a new tester the python instance should + # disappear because it wasn't created with an alias + new_tester = m.SpBaseTester() + + cobj = tester.get_object() + assert cobj.has_python_instance() + + new_tester.set_object(cobj) + assert tester.has_python_instance() is True + assert new_tester.has_python_instance() is True + + del cobj + pytest.gc_collect() + + # Gone! + assert tester.has_instance() is True + assert tester.has_python_instance() is False + assert new_tester.has_instance() is True + assert new_tester.has_python_instance() is False + + def test_shared_ptr_goaway(): import weakref