diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 7930fb994b..4337ab5c86 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,50 @@ 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 { + // If there's no trampoline class, nothing special needed + if (!v_h.inst->is_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 + // 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 +728,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/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 b08bbc5590..f8b6c204d0 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -580,6 +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, 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/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/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..39a41703bc --- /dev/null +++ b/tests/test_trampoline_shared_ptr_cpp_arg.cpp @@ -0,0 +1,79 @@ +// 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; } + + // 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; +}; + +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(); } + 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; +}; + +// 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) + .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 + // 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 new file mode 100644 index 0000000000..555dcfc1a0 --- /dev/null +++ b/tests/test_trampoline_shared_ptr_cpp_arg.py @@ -0,0 +1,131 @@ +# -*- 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.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(): + 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() + assert tester.has_python_instance() is True + + # python reference disappears once the C++ object releases it + tester.set_object(None) + pytest.gc_collect() + 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 + + 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