[BUG]: intrusive counter errors on reacquiring Python-side of object

[tdavidovicNV]

Problem description

I have a piece of C++ code, where a ref-counted class (Outter) holds another ref-counted class (Inner) by nb::ref. The Inner class has a method get_name() that just returns a string. The Outter class has a method get_inner that returns an Inner&.

When I then do two consecutive calls to get_inner like so:

outter = Outter()
print(outter.get_inner().get_name())
print(outter.get_inner().get_name()) # hits abort here

it hits the abort in intrusive_counter::set_self_py.

If the get_inner() returns ref<Inner> instead of Inner&, things work fine.

My assumption is that the code that creates a Python object for Inner gets invoked twice, once for each get_inner() call. Instead it could first try to call self_py to check if the object exists, and avoid setting it the second time.

Reproducible example code

This is referenced in #1112

[tdavidovicNV]

I dag a bit deeper and if I bind the example class Outter like this:

struct Outter : public nb::intrusive_base
{
     Inner& get_inner();
     ...
};

    nb::class_<Outter, nb::intrusive_base>(m, "Outter")
        .def(nb::init<>())
        .def("get_inner", [](Outter* self_) { return self->get_inner(); }); 

the lamba will have a return type of Outter rather than Outter&, it will create a copy of Outter and that gets passed into type_caster_base::from_cpp where the ptr is every time different, and thus the inst_c2p.find(value); in nb_type_put_p fails to find it by the pointer.

Explicitly specifying the return type as a reference like so:

        .def("get_inner", [](Outter* self_) -> Inner& { return self->get_inner(); }); 

actually fixes the issue, no copy is made, and we get the correct behavior. This also explains why returning a pointer or a ref solves the issues. I do not know if this can be helped with any kind of super smart magic, probably not.

[tdavidovicNV]

The other part of the issue (with the same symptoms) is this implementation:

.def("get_inner", &Outter::get_inner)

This seems to be much simpler. The default polity is automatic, this line policy = infer_policy<T>(policy); in type_caster_base::from_cpp turns it to copy.

This prevents the code in nb_type_put_p from looking up exiting value in nb_ptr_map::iterator it = inst_c2p.find(value); because that is only done when if (rvp != rv_policy::copy) {.

But the override to rv_policy::take_ownership; when the type is intrusive happens only in nb_type_put_common, after the opportunity to look pointer up in inst_c2p has passed.

This does not happen when the Inner is returned as pointer, as the default policy for pointers is take_ownership and it goes into the lookup code. This feels like a rather easy fix, to hoist the policy override for intrusive higher up the call chain.

[wjakob]

Hi @tdavidovicNV,

moving the check as you suggested would come at a quite noticeable overhead for all other bindings. So I am actually leaning towards adding this as an exception in the docs, e.g. "If you use intrusive reference counting, and you return an object by reference, then you must add rv_policy::reference"

[tdavidovicNV]

Is it because you don't have to call lookup_type if the value is in the cache?

Because to me it looked like you lookup_type all the time anyway (no extra cost there), and once you have that, it is a single flag check.