[FEAT] being able to enforce the dereferencing of the smart pointer in argument conversion

[t-vi]

Thank you for making PyBind11!

I have a relatively complex ("externally given") class with a funny ownership model, so things can go missing while Python has a reference. To wrap it in Python safely, I'm trying to hold instances in a smart pointer (a la shared_ptr) that checks on dereferencing whether the referred to object still exists. It does so by another pointer indirection.

The trouble is that while PyBind will create the shared ptr, I cannot seem to enforce that argument conversion always goes through the smart ptr and not the raw ptr held alongside.

I tried to do a custom caster, but that ran into "specialization after instantiation" error messages.

There is an "obvious path" to go through lambdas that all take the smart pointer and dereference themselves, but this is obviously a bit cumbersome when it needs to be done to a large number of methods.

@YannickJadoul looked at this on gitter to help clarify my thoughts (but all errors are mine). Thank you Yannick!

#include <pybind11/pybind11.h>
#include <iostream>

template <typename T>
struct Wrap {
  Wrap(T* p) : elem(p) {}
  void clear() { elem = nullptr; }
  T* elem;
};


class MyClass {
public:
  MyClass() : wrap_(std::make_shared<Wrap<MyClass>>(this)) {
  }
  std::shared_ptr<Wrap<MyClass>> wrap() {
    return wrap_;
  }
  ~MyClass() {
    wrap_->clear();
  }
private:
  std::shared_ptr<Wrap<MyClass>> wrap_;
};

template <typename T>
class unwrapping_shared_ptr {
 private:
  std::shared_ptr<Wrap<T>> impl;

 public:
  unwrapping_shared_ptr() = default;
  unwrapping_shared_ptr(std::shared_ptr<T> p) : impl(p) { std::cerr << "construct shared_ptr\n"; }
  unwrapping_shared_ptr(T* p) : impl(p->wrap()) { std::cerr << "construct T*\n"; }
  T* get() const {
    if (! impl->elem) {
      throw std::logic_error("has been invalidated");
    }
    std::cerr << "get deref!\n";
    return impl->elem;
  }
  T** operator&() {
    if (! impl->elem) {
      throw std::logic_error("has been invalidated");
    }
    std::cerr << "& deref!\n";
    return &(impl->elem);
  }
};

PYBIND11_DECLARE_HOLDER_TYPE(T, unwrapping_shared_ptr<T>, true);

namespace py = pybind11;

PYBIND11_MODULE(test_ext, m) {
  py::class_<MyClass, unwrapping_shared_ptr<MyClass>>(m, "MyClass")
    .def("__str__", [] (MyClass& m) { return "MyClass - jolly good"; });

  m.def("get_something", [] () { return new MyClass(); });
  m.def("delete_something", [] (MyClass* m) { delete m; });
  m.def("test_something", [] (unwrapping_shared_ptr<MyClass> m) {
    if (m.get()) {
      return "alive";
    } else {
      return "just resting";
    }});
}

and then in Python this gives me:

In [1]: import test_ext

In [2]: t = test_ext.get_something()
construct T*

In [4]: test_ext.test_something(t)
get deref!
Out[4]: 'alive'

In [5]: test_ext.delete_something(t)

In [6]: test_ext.test_something(t)
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-6-34740f9db26d> in <module>
----> 1 test_ext.test_something(t)

RuntimeError: has been invalidated

In [7]: str(t)
Out[7]: 'MyClass - jolly good'

In [8]: 

Ideally, in [7] it would try to dereference the smart pointer, but it does not.

[YannickJadoul]

To be clear, I'm not making any promises (it's very tricky to access the holder if you don't know what the holder is, since pybind11 type-erases the holder object in ), but it's an interesting angle to take into account when working on the holders: should we always try to access contents of an object through holders? Would that solve some other things?
So I'm giving it that label (ping @EricCousineau-TRI, @rwgk, @rhaschke).

Meanwhile, I'll look at the custom type_caster solution.

[rwgk]

should we always try to access contents of an object through holders? Would that solve some other things?

IIUC @rhaschke's comment from a couple weeks ago correctly, the raw pointer is also used in some registry, I hope Robert can chime in (I'd have to dig up that comment).

But assuming that can be resolved (if necessary), I think it's a great small-scale experiment to replace the raw pointer lookup with dereferencing the holder, and see if that has any measurable impact on runtime performance. (I feel like it will not, but you never know before you try.)

[YannickJadoul]

AFAIC, I'm not even sure this should be "fixed", because I'm not sure pybind11 promises anything like this (so it would need a discussion about expected behavior, etc). But it's interesting to keep in the back of our minds, when looking at the other holder types, right? Especially if we notice this results in further confusion, it's worth considering making a semi-breaking change.

small-scale experiment to replace the raw pointer lookup with dereferencing the holder

And yes, I was thinking that a pointer to the value most likely takes as much memory as a pointer to a function that extracts the pointer to the value from the holder (again, things are type-erased, so not that obvious at runtime, because you don't know the type of the holder, so you need some kind of function stored to do so, I think!).

[rhaschke]

I ran into this issue before as well. Having a custom holder type, you would actually expect that the holder's get() function is called each time its value is accessed. But that's not the case, because, in the value_and_holder struct, both the original pointer as well as the holder are stored and - as a shortcut - only the raw pointer is used in most cases.
However, that's not the only issue here: In @t-vi original post, a Python-managed object instance is passed to C++ and deleted there. This should not only delete the C++ part of the instance, but also the Python-part, i.e. the wrapper instance as well as its corresponding entry in pybind's internal registered_instances.
Furthermore, in the example code, ownership is not clear at all: Passing a raw pointer to delete_something() doesn't grant ownership and thus the right to delete the pointer!

[t-vi]

However, that's not the only issue here: In @t-vi original post, a Python-managed object instance is passed to C++ and deleted there. This should not only delete the C++ part of the instance, but also the Python-part, i.e. the wrapper instance as well as its corresponding entry in pybind's internal registered_instances.

Well, so I cannot destruct python instances, and my goal here is to invalidate them through the smart pointer. This would work if the holder's get() function were called each time its value is accessed.

Furthermore, in the example code, ownership is not clear at all: Passing a raw pointer to delete_something() doesn't grant ownership and thus the right to delete the pointer!

This is "just" the minimal example. So what I would like to do is call clear() on the wrapper (or I could make a clear method on the smart pointer) and then subsequent Python conversions should fail. (I don't expect a solution for the lifetime issue of clearing while someone else still has the pointer.)

[YannickJadoul]

@t-vi, soooo, it's harder than I thought it would be, because of an implementation detail: the default holder_type<T>'s type_caster<holder_type<T>> inherits type_caster<T>. And I wanted to try create type_caster<T> by subclassing (or at least using) type_caster<holder_type<T>>. That's a pretty circular dependency.

Good news is, it's possible, though, by cutting and pasting relevant parts of copyable_holder_caster<T>. Bad news is, it looks pretty horrible; please don't try this at home. It's also (probably?) slightly hacking into implementation details, but then the whole type_caster thing is in detail and (in principle) bound to change without warning, I think. Good (?) news on that level is: no one's going to want to touch that code, anyway, so it's pretty stable...

Now, without further ado, I reveal to you, the monster hack:

namespace pybind11 {
namespace detail {

template <>
struct type_caster<MyClass> : public type_caster_base<MyClass> {
public:
    using type = MyClass;
    using holder_type = unwrapping_shared_ptr<MyClass>;

    bool load(handle src, bool convert) {
        return load_impl<type_caster<MyClass>>(src, convert);
    }

    explicit operator type *() { return static_cast<type *>(value); }
    explicit operator type &() { return *static_cast<type *>(value); }

protected:
    friend class type_caster_generic;

    bool load_value(value_and_holder &&v_h) {
        if (v_h.holder_constructed()) {
            value = v_h.template holder<holder_type>().get();
            return true;
        } else {
            throw cast_error("Unable to cast from non-held to held instance (T& to Holder<T>) "
#if defined(NDEBUG)
                             "(compile in debug mode for type information)");
#else
                             "of type '" + type_id<holder_type>() + "''");
#endif
        }
    }
};

}
}

which gives you, I believe, the output you expected?

import example

t = example.get_something()
example.test_something(t)
example.delete_something(t)
try:
	example.test_something(t)
except Exception as e:
	print(e)
print(str(t))

gives output

get deref!
get deref!
has been invalidated
Traceback (most recent call last):
  File "/home/yannick/tmp/sandbox/test.py", line 10, in <module>
    print(str(t))
RuntimeError: has been invalidated

I want to say "you're welcome", but ... I don't know if this is such a nice present, actually 😅 So I'll just keep it at: "enjoy" ;-)

[t-vi]

Thank you! 🎁 🎄 This is exactly what I had in mind and it will be useful to me.

[t-vi]

It turns out that with @YannickJadoul 's neat hack, PyBind still appears to cache the "Raw Pointer -> Holder" resolution, so if I delete a C++ object and allocate a new one, the holder isn't obtained through the .get but through the cache, i.e. we get the wrong (old) one.
So I'm looking into finding either a way to remove an entry from the cache or to specify a custom casting method that avoids caching...

[YannickJadoul]

Copying from the Gitter discussion:

Yannick Jadoul
@YannickJadoul
21:56
Oh! I think I got it!
@t-vi, yes, this is what happens:
Thomas Viehmann

@t-vi
21:58
I'm on the version used by PyTorch (2.6.1 + a handful of cherry picked).

Yannick Jadoul
@YannickJadoul
22:00

    1. Create something with this nullable holder and return it to Python.
    2. The mapping from this C++ object's address to the Python object gets stored when first creating the Python object.
    3. Destroy the C++ object; the Python object stays in scope.
    4. Return a new C++ object with the same address as before. The mapping is still there (no one removed that cache, because ... who would? The Python object is still alive.), and you return the Python object wrapping the old (deleted) object.

Damn
Cfr. disabling that cache: that's not a pure optimization
It also makes sure that if you return the same C++ object multiple times, that you will get the same Python object

Thomas Viehmann
@t-vi
22:04
Yes, that matches my observations. so I would have a unique address that could be used (that of the wrapping object (not the holder)), but I cannot seem to get the casting to do that.

[t-vi]

Having wrap::clear remove the registered instances appears to work around this issue. So wrap::clear would call clear_registered_instances(elem) defined as follows:

void clear_registered_instances(void* ptr)
{
  auto &registered_instances = pybind11::detail::get_internals().registered_instances;
  auto range = registered_instances.equal_range(ptr);
  for (auto it = range.first; it != range.second; ++it) {
    auto vh = it->second->get_value_and_holder();
    vh.set_instance_registered(false);
  }
  registered_instances.erase(ptr);
}

I'll work more with this added tomorrow and am hopeful that it is indeed more stable.

Fingers crossed that the memory leaks aren't too large.

Thank you @YannickJadoul for the discussion online, errors are my own doing.

[YannickJadoul]

@t-vi That looks about what I had in mind to try, around the time you suggested it, indeed :-)

Fingers crossed that the memory leaks aren't too large.

I'm not really expecting memory leaks. Are you?
It is kind of ugly that this is in wrap, rather than the caster, though, but I don't think you can get around that? After all, the caster doesn't know if you're going to steal from it.

(@rwgk, this is going to be quite relevant for you, as well, I believe)

[rwgk]

(@rwgk, this is going to be quite relevant for you, as well, I believe)

FWIW: I looked at @YannickJadoul's trick a bit a couple weeks ago but gave up when I began too feel strongly it doesn't help me in my pursuit of a general solution for improved smart-pointer handling.

My work on that general solution is still very much incomplete (#2672), but the core of how it will work is already here:

https://github.com/pybind/pybind11/pull/2672/files#diff-27833a8008c9cd24dc1e2dcb96af0bd0fb1cbf68eaefde67593bea0661c5eb6bR166

Ignoring the guard rails it boils down to simply:

    template <typename T>
    T *as_raw_ptr_unowned() const {
        return static_cast<T *>(vptr.get());
    }

The vptr (v is for void) dereferenced there is created like this:

hld.vptr = std::static_pointer_cast<void>(shd_ptr);

That means there is a detour through void and back to T, but I believe that's mostly an implementation detail from a user's viewpoint. I'm thinking it will work for @t-vi 's situation but we'll need to try it out to be sure.

@t-vi I wonder if this could be interesting to you:
https://github.com/pybind/pybind11/pull/2672/files#diff-311f60a15aa4e63e4bd116b584ceaa5b2d1afd392d69c42dfb539eff459e4071

This demo works with pybind11 as-is. If you go back to that "bare" interface you can do pretty much anything you want, although you're also responsible for all the internals yourself, incl. how you're holding the C++ object in a Python object. If that's too cumbersome for your situation, I'd be happy to help you try the smart_holder code, when it's a little more mature.

[t-vi]

how you're holding the C++ object in a Python object.

I guess looking for pybind11 to help me with this is exactly the bit that got me into trouble...
If I had the opportunity to store some shared_ptr<T> or shared_ptr<void> somewhere, it'd be all I need.