shared_ptrs to Python-derived instances outlive the associated Python object

[cdyson37]

Issue description

Storing shared_ptrs to Python-derived instances for later execution causes virtual functions to fail if the Python instance has subsequently been destroyed.

Tested on master: e7761e3 / Python 3.6 / gcc 7.2

Reproducible example code

C++

#include <pybind11/pybind11.h>

namespace py = pybind11;

struct Base
{
  virtual void f (int i) const = 0;
  virtual ~Base () { };
};

struct Trampoline : Base
{
  void f (int i) const override
  {
    PYBIND11_OVERLOAD_PURE_NAME (
      void,
      Base,
      "f",
      impl,
      /* args */
      i);
  }
};

struct Holder
{
  Holder (std::shared_ptr<Base> ptr)
  : ptr (std::move (ptr))
  { }

  std::shared_ptr<Base> ptr;
};

void takes_holder (const Holder &h)
{
  h.ptr->f (42);
}

PYBIND11_MODULE (testmodule, m)
{
  py::class_<Base, Trampoline, std::shared_ptr<Base>>
    base_wrap (m, "Base");

  base_wrap.def (py::init<> ());

  py::class_<Holder> holder_wrap (m, "Holder");
  holder_wrap.def (py::init<std::shared_ptr<Base>> ());

  m.def ("takes_holder", &takes_holder);
}

Python

import testmodule

class PythonDerived (testmodule.Base):
    def __init__ (self):
        testmodule.Base.__init__ (self)

    def f (self, i):
        print ("Called with: {}".format (i))


pd = PythonDerived ()
holder = testmodule.Holder (pd)
del pd # Comment this out and it works
testmodule.takes_holder (holder)

Output

Traceback (most recent call last):
  File "./test.py", line 16, in <module>
    testmodule.takes_holder (holder)
RuntimeError: Tried to call pure virtual function "Base::f"

Output (with 'del' removed)

Called with: 42

Workaround:

Add the following (tweaked by @iwanders - thanks)

namespace pybind11::detail {

  template<>
  struct type_caster<std::shared_ptr<Base>>
  {
    PYBIND11_TYPE_CASTER (std::shared_ptr<Base>, _("Base"));

    using BaseCaster = copyable_holder_caster<Base, std::shared_ptr<Base>>;

    bool load (pybind11::handle src, bool b)
    {
      BaseCaster bc;
      bool success = bc.load (src, b);
      if (!success)
      {
        return false;
      }

      auto py_obj = py::reinterpret_borrow<py::object> (src);
      auto base_ptr = static_cast<std::shared_ptr<Base>> (bc);

      // Construct a shared_ptr to the py::object
      auto py_obj_ptr = std::shared_ptr<object>{
          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;
         }
      };

      value = std::shared_ptr<Base> (py_obj_ptr, base_ptr.get ());
      return true;
    }

    static handle cast (std::shared_ptr<Base> base,
                        return_value_policy rvp,
                        handle h)
    {
      return BaseCaster::cast (base, rvp, h);
    }
  };

  template <>
  struct is_holder_type<Base, std::shared_ptr<Base>> : std::true_type {};
}

This replaces the default shared_ptr caster with one that keeps the Python object alive, using the aliasing constructor of shared_ptr.

Comment

Ideally, whenever there's a shared_ptr to a Python-derived object, that shared_ptr should keep the Python side of the object alive. The above seems to do that, but there may be a more sensible approach!

I also tried a workaround using

  python_wrap.def ("__init__", [] (py::detail::value_and_holder &v_h) {

and constructing a std::shared_ptr in a similar fashion, but I found that I couldn't capture "self" without passing it in as an additional argument (i.e. testmodule.Base.__init__ (self, self)).

I've also tried patching cast.h (below) as a proof-of-concept, and that appears to work (thanks @iwanders for the tweak)

diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h
index 5529768..e976129 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"
@@ -1496,9 +1497,55 @@ protected:
     holder_type holder;
 };
 
-/// Specialize for the common std::shared_ptr, so users don't need to
+/// Specialize type_caster for std::shared_ptr<T>.
+/// 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 <typename T>
-class type_caster<std::shared_ptr<T>> : public copyable_holder_caster<T, std::shared_ptr<T>> { };
+class type_caster<std::shared_ptr<T>>
+{
+    PYBIND11_TYPE_CASTER(std::shared_ptr<T>, _(PYBIND11_STRING_NAME));
+
+    // Re-use copyable_holder_caster
+    using BaseCaster = copyable_holder_caster<T, std::shared_ptr<T>>;
+
+    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
+        auto py_obj = reinterpret_borrow<object>(src);
+
+        // Construct a shared_ptr to the py::object
+        auto py_obj_ptr = std::shared_ptr<object>{
+            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<T>
+        // * Use this to make an aliased shared_ptr<T> that keeps the py::object alive
+        auto base_ptr = static_cast<std::shared_ptr<T>>(bc);
+        value = std::shared_ptr<T>(py_obj_ptr, base_ptr.get());
+        return true;
+    }
+
+    static handle cast(std::shared_ptr<T> sp,
+                       return_value_policy rvp,
+                       handle h)
+    {
+        return BaseCaster::cast(sp, rvp, h);
+    }
+};
 
 template <typename type, typename holder_type>
 struct move_only_holder_caster {
@@ -1540,6 +1587,9 @@ template <typename base, typename holder> struct is_holder_type :
 template <typename base, typename deleter> struct is_holder_type<base, std::unique_ptr<base, deleter>> :
     std::true_type {};
 
+template <typename T>
+struct is_holder_type<T, std::shared_ptr<T>> : std::true_type {};
+
 template <typename T> struct handle_type_name { static constexpr auto name = _<T>(); };
 template <> struct handle_type_name<bytes> { static constexpr auto name = _(PYBIND11_BYTES_NAME); };
 template <> struct handle_type_name<args> { static constexpr auto name = _("*args"); };
diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h
index fa5ed7c..fe88fcc 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;

[yungyuc]

This looks like a better description and solution than the existing #1145 (which is the same issue).

[iwanders]

See my comment on the associated PR when using the workaround proposed here in a multithreaded environment.

[cdyson37]

Thanks @iwanders - have updated the PR.

[iwanders]

Thanks for incorporating the addition so quickly @cdyson37 , it may be a good idea to also edit your workaround at the top of this issue such that people who copy paste that have the fix as well.

[cdyson37]

I've tweaked the workaround + patch above, but it would be good to get this merged, or at least looked at. Maybe I can call in a favour from @wjakob? :) We had a chat about the custodian_and_ward thing a while ago by email.

[asadchev]

There is a bug when c++ object is implicitly converted from src as src is separate from C++ instance then. Following fixes it:

auto base_ptr = static_cast<std::shared_ptr<T>> (bc);
auto h = BaseCaster::cast(base_ptr, return_value_policy(), handle());
auto py_obj = reinterpret_borrow<object>(h);

[EricCousineau-TRI]

Just to consolidate, I'm closing this in lieu of #1333. Feel free to reopen if you think this addresses a new / niche case.