diff --git a/include/pybind11/class_support.h b/include/pybind11/class_support.h index b6065bd588..2bfc5a8dd6 100644 --- a/include/pybind11/class_support.h +++ b/include/pybind11/class_support.h @@ -280,6 +280,29 @@ extern "C" inline int pybind11_object_init(PyObject *self, PyObject *, PyObject return -1; } +inline void add_patient(PyObject *nurse, PyObject *patient) { + auto &internals = get_internals(); + auto instance = reinterpret_cast(nurse); + instance->has_patients = true; + Py_INCREF(patient); + internals.patients[nurse].push_back(patient); +} + +inline void clear_patients(PyObject *self) { + auto instance = reinterpret_cast(self); + auto &internals = get_internals(); + auto pos = internals.patients.find(self); + assert(pos != internals.patients.end()); + // Clearing the patients can cause more Python code to run, which + // can invalidate the iterator. Extract the vector of patients + // from the unordered_map first. + auto patients = std::move(pos->second); + internals.patients.erase(pos); + instance->has_patients = false; + for (PyObject *&patient : patients) + Py_CLEAR(patient); +} + /// Clears all internal data from the instance and removes it from registered instances in /// preparation for deallocation. inline void clear_instance(PyObject *self) { @@ -304,6 +327,9 @@ inline void clear_instance(PyObject *self) { PyObject **dict_ptr = _PyObject_GetDictPtr(self); if (dict_ptr) Py_CLEAR(*dict_ptr); + + if (instance->has_patients) + clear_patients(self); } /// Instance destructor function for all pybind11 types. It calls `type_info.dealloc` diff --git a/include/pybind11/common.h b/include/pybind11/common.h index 928205193f..626178e4f2 100644 --- a/include/pybind11/common.h +++ b/include/pybind11/common.h @@ -405,6 +405,8 @@ struct instance { bool simple_layout : 1; /// For simple layout, tracks whether the holder has been constructed bool simple_holder_constructed : 1; + /// If true, get_internals().patients has an entry for this object + bool has_patients : 1; /// Initializes all of the above type/values/holders data void allocate_layout(); @@ -469,6 +471,7 @@ struct internals { std::unordered_multimap registered_instances; // void * -> instance* std::unordered_set, overload_hash> inactive_overload_cache; type_map> direct_conversions; + std::unordered_map> patients; std::forward_list registered_exception_translators; std::unordered_map shared_data; // Custom data to be shared across extensions PyTypeObject *static_property_type; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 40b32a83e9..973dea9241 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1328,20 +1328,30 @@ template struct init_alias { inline void keep_alive_impl(handle nurse, handle patient) { - /* Clever approach based on weak references taken from Boost.Python */ if (!nurse || !patient) pybind11_fail("Could not activate keep_alive!"); if (patient.is_none() || nurse.is_none()) return; /* Nothing to keep alive or nothing to be kept alive by */ - cpp_function disable_lifesupport( - [patient](handle weakref) { patient.dec_ref(); weakref.dec_ref(); }); + auto tinfo = all_type_info(Py_TYPE(nurse.ptr())); + if (!tinfo.empty()) { + /* It's a pybind-registered type, so we can store the patient in the + * internal list. */ + add_patient(nurse.ptr(), patient.ptr()); + } + else { + /* Fall back to clever approach based on weak references taken from + * Boost.Python. This is not used for pybind-registered types because + * the objects can be destroyed out-of-order in a GC pass. */ + cpp_function disable_lifesupport( + [patient](handle weakref) { patient.dec_ref(); weakref.dec_ref(); }); - weakref wr(nurse, disable_lifesupport); + weakref wr(nurse, disable_lifesupport); - patient.inc_ref(); /* reference patient and leak the weak reference */ - (void) wr.release(); + patient.inc_ref(); /* reference patient and leak the weak reference */ + (void) wr.release(); + } } PYBIND11_NOINLINE inline void keep_alive_impl(size_t Nurse, size_t Patient, function_call &call, handle ret) { diff --git a/tests/test_call_policies.cpp b/tests/test_call_policies.cpp index 33a3f15eeb..15275923eb 100644 --- a/tests/test_call_policies.cpp +++ b/tests/test_call_policies.cpp @@ -24,6 +24,13 @@ class Parent { Child *returnNullChild() { return nullptr; } }; +#if !defined(PYPY_VERSION) +class ParentGC : public Parent { +public: + using Parent::Parent; +}; +#endif + test_initializer keep_alive([](py::module &m) { py::class_(m, "Parent") .def(py::init<>()) @@ -34,6 +41,11 @@ test_initializer keep_alive([](py::module &m) { .def("returnNullChildKeepAliveChild", &Parent::returnNullChild, py::keep_alive<1, 0>()) .def("returnNullChildKeepAliveParent", &Parent::returnNullChild, py::keep_alive<0, 1>()); +#if !defined(PYPY_VERSION) + py::class_(m, "ParentGC", py::dynamic_attr()) + .def(py::init<>()); +#endif + py::class_(m, "Child") .def(py::init<>()); }); diff --git a/tests/test_call_policies.py b/tests/test_call_policies.py index 1cb11c222f..de4ec9690e 100644 --- a/tests/test_call_policies.py +++ b/tests/test_call_policies.py @@ -2,21 +2,22 @@ def test_keep_alive_argument(capture): - from pybind11_tests import Parent, Child + from pybind11_tests import Parent, Child, ConstructorStats + n_inst = ConstructorStats.detail_reg_inst() with capture: p = Parent() assert capture == "Allocating parent." with capture: p.addChild(Child()) - pytest.gc_collect() + assert ConstructorStats.detail_reg_inst() == n_inst + 1 assert capture == """ Allocating child. Releasing child. """ with capture: del p - pytest.gc_collect() + assert ConstructorStats.detail_reg_inst() == n_inst assert capture == "Releasing parent." with capture: @@ -24,11 +25,11 @@ def test_keep_alive_argument(capture): assert capture == "Allocating parent." with capture: p.addChildKeepAlive(Child()) - pytest.gc_collect() + assert ConstructorStats.detail_reg_inst() == n_inst + 2 assert capture == "Allocating child." with capture: del p - pytest.gc_collect() + assert ConstructorStats.detail_reg_inst() == n_inst assert capture == """ Releasing parent. Releasing child. @@ -36,21 +37,22 @@ def test_keep_alive_argument(capture): def test_keep_alive_return_value(capture): - from pybind11_tests import Parent + from pybind11_tests import Parent, ConstructorStats + n_inst = ConstructorStats.detail_reg_inst() with capture: p = Parent() assert capture == "Allocating parent." with capture: p.returnChild() - pytest.gc_collect() + assert ConstructorStats.detail_reg_inst() == n_inst + 1 assert capture == """ Allocating child. Releasing child. """ with capture: del p - pytest.gc_collect() + assert ConstructorStats.detail_reg_inst() == n_inst assert capture == "Releasing parent." with capture: @@ -58,11 +60,74 @@ def test_keep_alive_return_value(capture): assert capture == "Allocating parent." with capture: p.returnChildKeepAlive() - pytest.gc_collect() + assert ConstructorStats.detail_reg_inst() == n_inst + 2 assert capture == "Allocating child." with capture: del p - pytest.gc_collect() + assert ConstructorStats.detail_reg_inst() == n_inst + assert capture == """ + Releasing parent. + Releasing child. + """ + + +# https://bitbucket.org/pypy/pypy/issues/2447 +@pytest.unsupported_on_pypy +def test_alive_gc(capture): + from pybind11_tests import ParentGC, Child, ConstructorStats + + n_inst = ConstructorStats.detail_reg_inst() + p = ParentGC() + p.addChildKeepAlive(Child()) + assert ConstructorStats.detail_reg_inst() == n_inst + 2 + lst = [p] + lst.append(lst) # creates a circular reference + with capture: + del p, lst + assert ConstructorStats.detail_reg_inst() == n_inst + assert capture == """ + Releasing parent. + Releasing child. + """ + + +def test_alive_gc_derived(capture): + from pybind11_tests import Parent, Child, ConstructorStats + + class Derived(Parent): + pass + + n_inst = ConstructorStats.detail_reg_inst() + p = Derived() + p.addChildKeepAlive(Child()) + assert ConstructorStats.detail_reg_inst() == n_inst + 2 + lst = [p] + lst.append(lst) # creates a circular reference + with capture: + del p, lst + assert ConstructorStats.detail_reg_inst() == n_inst + assert capture == """ + Releasing parent. + Releasing child. + """ + + +def test_alive_gc_multi_derived(capture): + from pybind11_tests import Parent, Child, ConstructorStats + + class Derived(Parent, Child): + pass + + n_inst = ConstructorStats.detail_reg_inst() + p = Derived() + p.addChildKeepAlive(Child()) + # +3 rather than +2 because Derived corresponds to two registered instances + assert ConstructorStats.detail_reg_inst() == n_inst + 3 + lst = [p] + lst.append(lst) # creates a circular reference + with capture: + del p, lst + assert ConstructorStats.detail_reg_inst() == n_inst assert capture == """ Releasing parent. Releasing child. @@ -70,18 +135,19 @@ def test_keep_alive_return_value(capture): def test_return_none(capture): - from pybind11_tests import Parent + from pybind11_tests import Parent, ConstructorStats + n_inst = ConstructorStats.detail_reg_inst() with capture: p = Parent() assert capture == "Allocating parent." with capture: p.returnNullChildKeepAliveChild() - pytest.gc_collect() + assert ConstructorStats.detail_reg_inst() == n_inst + 1 assert capture == "" with capture: del p - pytest.gc_collect() + assert ConstructorStats.detail_reg_inst() == n_inst assert capture == "Releasing parent." with capture: @@ -89,11 +155,11 @@ def test_return_none(capture): assert capture == "Allocating parent." with capture: p.returnNullChildKeepAliveParent() - pytest.gc_collect() + assert ConstructorStats.detail_reg_inst() == n_inst + 1 assert capture == "" with capture: del p - pytest.gc_collect() + assert ConstructorStats.detail_reg_inst() == n_inst assert capture == "Releasing parent."