No really, why is enable_shared_from_this forbidden?

[oremanj]

I've read the docs and thought about it fairly extensively. I'm failing to come up with a scenario where nanobind's guard against types that use std::enable_shared_from_this would actually prevent a dangling pointer or other form of UB.

The docs say:

nanobind refuses conversion of classes that derive from std::enable_shared_from_this. This is a fundamental limitation: nanobind instances do not create a base shared pointer that declares ownership of an object. Other parts of a C++ codebase might then incorrectly assume ownership and eventually try to delete a nanobind instance allocated using pymalloc (which is undefined behavior). A compile-time assertion catches this and warns about the problem.

I think of enable_shared_from_this as a meeting-place: if someone has created a shared_ptr for an object (whose lifetime hasn't ended yet), it provides a way for others to locate it and share ownership with the first person. If the initial shared_ptr creation was valid (made by someone with ownership of a resource -- even if that resource is "one refcount on the owning PyObject*" -- and made with a deleter that properly disposes of the resource), I don't see how letting more owners join in creates a problem. nanobind already has to assume when it passes a shared_ptr into C++ that it might be used to create multiple C++-side shared owners.

About the only way I can imagine enable_shared_from_this causing problems is if some C++ code gets a T*, tries to extract its shared_ptr<T>, finds that there isn't one, and then decides to create one that assumes the T* is heap-allocated and doesn't have any other owners. That seems like quite a large leap of faith, and I'd call it a bug in the code that's assuming ownership in that way -- a raw pointer doesn't offer that sort of license. If you need a shared_ptr and you don't have one, you can't just magic one into existence. Is there really enough code in the wild that does this that it's worth banning the whole pattern? Or is there some other situation that I'm missing here?

My current belief is that if I were to eliminate the shared_from_this assertion, eliminate the void* casting in the shared_ptr type caster that's commented as being designed to defeat enable_shared_from_this, and make sure enable_shared_from_this types were only passed across the C++/Python boundary when wrapped in shared_ptr, then everything would work fine as long as I don't have any code that does the sketchy ownership assumptions described in the previous paragraph. I understand that this is a higher degree of care than you might be willing to assume of the average nanobind user, but it seems like a far cry from the "enable_shared_from_this is fundamentally impossible to support" message I got from the documentation, so I wanted to double-check my understanding.

[wjakob]

That seems like quite a large leap of faith, and I'd call it a bug in the code that's assuming ownership in that way -- a raw pointer doesn't offer that sort of license. If you need a shared_ptr and you don't have one, you can't just magic one into existence. Is there really enough code in the wild that does this that it's worth banning the whole pattern? Or is there some other situation that I'm missing here?

Isn't that precisely what enable_shared_from_this is designed to enable? (Getting the shared pointer from a raw pointer and creating one if it it does not yet exist). If this was illegal, I would expect the latter operation to raise an exception or abort(), but that is IIRC not how this operation is implemented.

Besides this, a major change would be that the approach of creating multiple shared pointers with a custom deleter that holds a reference would no longer work. There would need to be a more advanced deleter replacement that handles everything with a single instance and while being compatible with the case that an object was not constructed by nanobind. It might be possible but surely sounds complicated.

One last constraint: nanobind is designed to pull in a truly minimal number of C++ headers. memory is not among them, and anything related to shared pointers should be be optional. This means that code in class_ to special-case classes that inherit from enable_from_this would be tricky.

Generally my idea with this library was that pybind11 is great for handling all kinds of use cases, but it ends up being pretty complex as a consequence. Nanobind is opinionated and ignores use cases to be simpler+faster, which will usually require adaptations in the c++ code to be bound. enable_shared_from_this did not make the cut.

If you can find a way to generalize the current approach while preserving its simplicity, I am definitely open to it.

FYI: I am on vacation for a couple of days. I will review your datetime PR when I am back.

[oremanj]

Isn't that precisely what enable_shared_from_this is designed to enable? (Getting the shared pointer from a raw pointer and creating one if it it does not yet exist). If this was illegal, I would expect the latter operation to raise an exception or abort(), but that is IIRC not how this operation is implemented.

Calling shared_from_this() on an enable_shared_from_this object that doesn't already have an associated shared_ptr will in fact throw an exception (and was undefined behavior until C++17). The only way to get the "create if it doesn't exist" behavior is by open-coding it:

T* obj = /* ... */;
std::shared_ptr<T> ptr = obj->weak_from_this().lock();
if (!ptr) {
    ptr = std::shared_ptr<T>{obj};
}

Obviously anything in the STL will be used in different ways by different people, but the only way I've personally used enable_shared_from_this is on an object that will always be constructed using make_shared; it provides a convenient way to get access to the shared_ptr (in order to share it with more people) if it exists but just isn't in a place you have convenient access to, such as from within a member function of the object itself -- I'm pretty sure that use case is the origin of the name shared_from_this.

Besides this, a major change would be that the approach of creating multiple shared pointers with a custom deleter that holds a reference would no longer work. There would need to be a more advanced deleter replacement that handles everything with a single instance and while being compatible with the case that an object was not constructed by nanobind. It might be possible but surely sounds complicated.

I actually think it would work fine? There is no need for the shared_ptr that enable_shared_from_this is aware of to be the One True shared_ptr for the object. The first time someone passes a nanobind-constructed object into C++ (assuming they pass it via shared_ptr<T>), the internal enable_shared_from_this weak_ptr will point at the reference-managing control block nanobind creates. If all C++-side references are dropped and the weak_ptr expires, it will just be reinstated the next time the object gets passed to C++ again. I'm not aware of any requirement that the shared_ptr control block referenced by an enable_shared_from_this instance actually destroy the instance when its deleter runs.

It would also be easy to change the shared_ptr caster to use the enable_shared_from_this context to join an existing reference-managing shared_ptr (if it exists) instead of creating a new one, which would be a nice optimization if the same object is passed from Python to C++ via shared_ptr many times.

Another way to think about it: enable_shared_from_this<T> is just a struct containing a weak_ptr<T>. The "magic" is in the shared_ptr constructors that create a new control block: they recognize the enable_shared_from_this base and set the contained weak_ptr to share ownership with the new shared_ptr if it wasn't already sharing ownership with someone else. I just did another pass through the cppreference docs and I don't see any problem with having multiple shared_ptr control blocks that manage the object (via the PyObject-reference-dropping deleter); enable_shared_from_this will pick up the first one it sees.

One last constraint: nanobind is designed to pull in a truly minimal number of C++ headers. memory is not among them, and anything related to shared pointers should be be optional. This means that code in class_ to special-case classes that inherit from enable_from_this would be tricky.

This is why I proposed adding the requirement that enable_shared_from_this types must still get passed across the Python/C++ boundary wrapped in shared_ptr. Nothing about the "normal" caster path would strictly need to change. It would be a nice improvement to detect and forbid returning a raw pointer to enable_shared_from_this type with rv_policy::take_ownership, since that was a supported use case in pybind (which would detect the existing shared_ptr and share ownership) while on nanobind it's setting up for a double-free scenario. But I think that can be a single flag in the type_data which is populated based on the existence of a member named shared_from_this regardless of its type; it wouldn't need to pull in an additional header.

Generally my idea with this library was that pybind11 is great for handling all kinds of use cases, but it ends up being pretty complex as a consequence. Nanobind is opinionated and ignores use cases to be simpler+faster, which will usually require adaptations in the c++ code to be bound. enable_shared_from_this did not make the cut.

I respect that approach, and I'm only interested in supporting enable_shared_from_this if it can be done with minimal-to-no impact on maintainability and on build time / performance / etc for people that don't use the feature. But I think it can, so I figured I'd open this discussion to see if I was missing anything.

Enjoy your vacation -- thanks for being open to discussing this!

[wjakob]

Calling shared_from_this() on an enable_shared_from_this object that doesn't already have an associated shared_ptr will in fact throw an exception (and was undefined behavior until C++17). The only way to get the "create if it doesn't exist" behavior is by open-coding it:

That's good to know! Perhaps we could relax the current behavior then and document this type of code as an example of something that is explicitly forbidden.

This is why I proposed adding the requirement that enable_shared_from_this types must still get passed across the Python/C++ boundary wrapped in shared_ptr.

That might be a bit restrictive, no? I think it's quite common to have helper routines with "borrowed reference" semantics that take a raw pointer. It would be limiting if those functoins cannot be bound by nanobind anymore. Maybe I misunderstood (since you said that the normal type caster wouldn't be changed).

But I think that can be a single flag in the type_data which is populated based on the existence of a member named shared_from_this regardless of its type; it wouldn't need to pull in an additional header.

That sounds like a reasonable option to me.

[wjakob]

Let me also CC @rwgk, who has worked on std::shared_ptr<..> on the pybind11 side. Nanobind has adopted some ideas from its smart_holder branch, which IIRC has the same limitation with regards to std::enable_shared_from_this (so this is perhaps interesting to you, Ralf)

[rwgk]

Long comments ... sorry I cannot read through all of it.

@oremanj did you already see the PR description here?

• smart_holder trampoline shared_from_this support pybind/pybind11#3023

Small rant: I think of enable_shared_from_this as a terrible "back door intruder" mis-feature.

But the pybind11 smart_holder branch supports it to the fullest extend possible, and the implementation has been completely stable since June/July 2021.

[oremanj]

@rwgk thank you for the link! I was in fact not familiar with the pybind11 smart_holder branch. I've now read the full diff against pybind11 master and I think I understand most of the subtleties around shared_from_this, but will need to give it another pass before I work on nanobind support.

@wjakob

That might be a bit restrictive, no? I think it's quite common to have helper routines with "borrowed reference" semantics that take a raw pointer. It would be limiting if those functoins cannot be bound by nanobind anymore. Maybe I misunderstood (since you said that the normal type caster wouldn't be changed).

Good point. What I mean is a little more subtle: assuming class T : public std::enable_shared_from_this<T> ...

• you can't return a bare T* obj to Python with rv_policy::take_ownership and expect the resulting Python instance to share ownership with obj->shared_from_this() (pybind11 supports this, but it requires detecting the enable_shared_from_this base; I'm dubious that it will be possible to do this robustly enough without naming std::enable_shared_from_this, but maybe there's some trick I'm not seeing; in general this sort of violates the idea of the shared_ptr type caster being isolated and self-contained)
• you can't accept a bare T* obj from Python and expect obj->shared_from_this() to work at all, let alone give you something that ultimately shares ownership of a reference to the Python object (base pybind11 "supports" this if the holder type is shared_ptr, but a Python object reference is not conveyed, so you get inheritance slicing for Python-side inheritance; shared_holder appears to support it for non-trampoline types)

But both of these will work if you return or accept std::shared_ptr<T>.

[oremanj]

enable_shared_from_this is now supported!

[rwgk]

To make it easier to backtrack in the future:

enable_shared_from_this is now supported!

that was #212