From 48543b12258cdea34ea3283432b261e8a27b4121 Mon Sep 17 00:00:00 2001 From: Charlie Dyson Date: Sun, 14 Oct 2018 14:27:37 +0100 Subject: [PATCH 1/4] Add failing test for #1546 --- tests/test_virtual_functions.cpp | 44 +++++++++++++++++++++++++++++++- tests/test_virtual_functions.py | 15 +++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/tests/test_virtual_functions.cpp b/tests/test_virtual_functions.cpp index c9a561c09f..8788319a34 100644 --- a/tests/test_virtual_functions.cpp +++ b/tests/test_virtual_functions.cpp @@ -475,4 +475,46 @@ void initialize_inherited_virtuals(py::module &m) { // Fix issue #1454 (crash when acquiring/releasing GIL on another thread in Python 2.7) m.def("test_gil", &test_gil); m.def("test_gil_from_thread", &test_gil_from_thread); -}; + + // Fix issue #1546 (Python object not kept alive by shared_ptr) + struct SharedPtrBase + { + virtual void f (int i) const = 0; + virtual ~SharedPtrBase () { }; + }; + + struct Trampoline : SharedPtrBase + { + void f (int i) const override + { + PYBIND11_OVERLOAD_PURE_NAME ( + void, + SharedPtrBase, + "f", + impl, + /* args */ + i); + } + }; + + struct SharedPtrHolder + { + SharedPtrHolder (std::shared_ptr ptr) + : ptr (std::move (ptr)) + { } + + void run (int i) + { + ptr->f (i); + } + + std::shared_ptr ptr; + }; + + py::class_> (m, "SharedPtrBase") + .def (py::init<> ()); + + py::class_ (m, "SharedPtrHolder") + .def (py::init> ()) + .def ("run", &SharedPtrHolder::run); +} diff --git a/tests/test_virtual_functions.py b/tests/test_virtual_functions.py index 5ce9abd355..06cf93021b 100644 --- a/tests/test_virtual_functions.py +++ b/tests/test_virtual_functions.py @@ -375,3 +375,18 @@ def test_issue_1454(): # Fix issue #1454 (crash when acquiring/releasing GIL on another thread in Python 2.7) m.test_gil() m.test_gil_from_thread() + +def test_issue_1546(): + # Fix issue #1546 (Python object not kept alive by shared_ptr) + somelist = [] + class Derived(m.SharedPtrBase): + def __init__(self): + super(Derived, self).__init__() + + def f(self, value): + somelist.append (value) + print("Right one", 42) + + holder = m.SharedPtrHolder(Derived()) + holder.run(42); + assert somelist == [42] From 2a097740ecac207d53c24bbe5315717007773b62 Mon Sep 17 00:00:00 2001 From: Charlie Dyson Date: Sun, 14 Oct 2018 15:13:36 +0100 Subject: [PATCH 2/4] Staw man solution to #1546 --- docs/changelog.rst | 3 +++ include/pybind11/cast.h | 44 +++++++++++++++++++++++++++++++-- tests/test_virtual_functions.py | 2 ++ 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 606be413aa..18963ea7a0 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -50,6 +50,9 @@ v2.3.0 (Not yet released) * ``pybind11/stl.h`` does not convert strings to ``vector`` anymore. `#1258 `_. +* fixed shared_ptrs to Python-derived instances outliving the associated Python object + `#1546 `_. + v2.2.4 (September 11, 2018) ----------------------------------------------------- diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 5529768b38..419b7aa08e 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1496,9 +1496,46 @@ struct copyable_holder_caster : public type_caster_base { holder_type holder; }; -/// Specialize for the common std::shared_ptr, so users don't need to +/// Specialize type_caster for std::shared_ptr. +/// This is the same as copyable_holder_caster, except that when casting to C++ +/// we keep the Python object alive through the shared_ptr as e.g. virtual +/// functions and derived state might be defined there. template -class type_caster> : public copyable_holder_caster> { }; +class type_caster> +{ + PYBIND11_TYPE_CASTER (std::shared_ptr, _(PYBIND11_STRING_NAME)); + + // Re-use copyable_holder_caster + using BaseCaster = copyable_holder_caster>; + + bool load (pybind11::handle src, bool b) + { + BaseCaster bc; + bool success = bc.load (src, b); + if (!success) + { + return false; + } + + // * Get src as a py::object + // * Construct a shared_ptr to the py::object + auto py_obj = reinterpret_borrow (src); + auto py_obj_ptr = std::make_shared (py_obj); + + // * Use BaseCaster to get it as the shared_ptr + // * Use this to make an aliased shared_ptr that keeps the py::object alive + auto base_ptr = static_cast> (bc); + value = std::shared_ptr (py_obj_ptr, base_ptr.get ()); + return true; + } + + static handle cast (std::shared_ptr sp, + return_value_policy rvp, + handle h) + { + return BaseCaster::cast (sp, rvp, h); + } +}; template struct move_only_holder_caster { @@ -1540,6 +1577,9 @@ template struct is_holder_type : template struct is_holder_type> : std::true_type {}; +template +struct is_holder_type> : std::true_type {}; + template struct handle_type_name { static constexpr auto name = _(); }; template <> struct handle_type_name { static constexpr auto name = _(PYBIND11_BYTES_NAME); }; template <> struct handle_type_name { static constexpr auto name = _("*args"); }; diff --git a/tests/test_virtual_functions.py b/tests/test_virtual_functions.py index 06cf93021b..79677a3bd2 100644 --- a/tests/test_virtual_functions.py +++ b/tests/test_virtual_functions.py @@ -388,5 +388,7 @@ def f(self, value): print("Right one", 42) holder = m.SharedPtrHolder(Derived()) + # At this point, our 'Derived' instance has gone out of scope in Python but + # is being kept alive by a shared_ptr inside 'holder'. holder.run(42); assert somelist == [42] From 7749760451bce92ea5733a28b1973b6730986abb Mon Sep 17 00:00:00 2001 From: Charlie Dyson Date: Sun, 14 Oct 2018 19:02:14 +0100 Subject: [PATCH 3/4] Attempt to appease flake8 and check_style.sh --- include/pybind11/cast.h | 25 ++++++++++++------------- tests/test_virtual_functions.py | 6 ++++-- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 419b7aa08e..8ce5caab2a 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1503,37 +1503,36 @@ struct copyable_holder_caster : public type_caster_base { template class type_caster> { - PYBIND11_TYPE_CASTER (std::shared_ptr, _(PYBIND11_STRING_NAME)); + PYBIND11_TYPE_CASTER(std::shared_ptr, _(PYBIND11_STRING_NAME)); // Re-use copyable_holder_caster using BaseCaster = copyable_holder_caster>; - bool load (pybind11::handle src, bool b) + bool load(pybind11::handle src, bool b) { BaseCaster bc; - bool success = bc.load (src, b); - if (!success) - { + bool success = bc.load(src, b); + if (!success) { return false; } // * Get src as a py::object // * Construct a shared_ptr to the py::object - auto py_obj = reinterpret_borrow (src); - auto py_obj_ptr = std::make_shared (py_obj); + auto py_obj = reinterpret_borrow(src); + auto py_obj_ptr = std::make_shared(py_obj); // * Use BaseCaster to get it as the shared_ptr // * Use this to make an aliased shared_ptr that keeps the py::object alive - auto base_ptr = static_cast> (bc); - value = std::shared_ptr (py_obj_ptr, base_ptr.get ()); + auto base_ptr = static_cast>(bc); + value = std::shared_ptr(py_obj_ptr, base_ptr.get()); return true; } - static handle cast (std::shared_ptr sp, - return_value_policy rvp, - handle h) + static handle cast(std::shared_ptr sp, + return_value_policy rvp, + handle h) { - return BaseCaster::cast (sp, rvp, h); + return BaseCaster::cast(sp, rvp, h); } }; diff --git a/tests/test_virtual_functions.py b/tests/test_virtual_functions.py index 79677a3bd2..eff2a05c6f 100644 --- a/tests/test_virtual_functions.py +++ b/tests/test_virtual_functions.py @@ -376,19 +376,21 @@ def test_issue_1454(): m.test_gil() m.test_gil_from_thread() + def test_issue_1546(): # Fix issue #1546 (Python object not kept alive by shared_ptr) somelist = [] + class Derived(m.SharedPtrBase): def __init__(self): super(Derived, self).__init__() def f(self, value): - somelist.append (value) + somelist.append(value) print("Right one", 42) holder = m.SharedPtrHolder(Derived()) # At this point, our 'Derived' instance has gone out of scope in Python but # is being kept alive by a shared_ptr inside 'holder'. - holder.run(42); + holder.run(42) assert somelist == [42] From 89a2e4925b3f233af51d65fd94dcbb3def9e4973 Mon Sep 17 00:00:00 2001 From: Charlie Dyson Date: Sat, 9 Feb 2019 18:24:20 +0000 Subject: [PATCH 4/4] Acquire gil when deleting object --- include/pybind11/cast.h | 17 ++++++++++++++--- include/pybind11/pytypes.h | 2 ++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 8ce5caab2a..e976129c06 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -10,6 +10,7 @@ #pragma once +#include "pybind11.h" #include "pytypes.h" #include "detail/typeid.h" #include "detail/descr.h" @@ -1516,10 +1517,20 @@ class type_caster> return false; } - // * Get src as a py::object - // * Construct a shared_ptr to the py::object + // Get src as a py::object auto py_obj = reinterpret_borrow(src); - auto py_obj_ptr = std::make_shared(py_obj); + + // Construct a shared_ptr to the py::object + auto py_obj_ptr = std::shared_ptr{ + new object{py_obj}, + [](auto py_object_ptr) { + // It's possible that when the shared_ptr dies we won't have the + // gil (if the last holder is in a non-Python thread), so we + // make sure to acquire it in the deleter. + gil_scoped_acquire gil; + delete py_object_ptr; + } + }; // * Use BaseCaster to get it as the shared_ptr // * Use this to make an aliased shared_ptr that keeps the py::object alive diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index fa5ed7cbd4..fe88fcc4c9 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -20,6 +20,8 @@ NAMESPACE_BEGIN(PYBIND11_NAMESPACE) class handle; class object; class str; class iterator; struct arg; struct arg_v; +class gil_scoped_acquire; +class gil_scoped_release; NAMESPACE_BEGIN(detail) class args_proxy;