Attach python lifetime to shared_ptr passed to C++

[virtuald]

trafficstars

Description

Modifies the base type caster to ensure that python instances don't die when passed to C++.

The shared_ptr is always given to C++ code, so we construct a new shared_ptr that is given a custom deleter. The custom deleter increments the python reference count to bind the python instance lifetime with the lifetime of the shared_ptr. This behavior is only applied to instances that have an associated trampoline class

This enables things like passing the last python reference of a subclass to a C++ function without the python reference dying -- which means custom virtual function overrides will still work.

Reference cycles will cause a leak, but this is a limitation of shared_ptr

Thanks to @YannickJadoul for the inspiration putting me on this path. Fixes #1333 and #1120, #1145, #1389, #1552, #1546, #1566, #1774, #1902, #1937

Other inspiration from Boost::Python. Quoting from @rwgk at https://github.com/pybind/pybind11/issues/2646:

When passing to shared_ptr arguments, Boost.Python always constructs a new shared_ptr, using a properly cast raw pointer, and a custom deleter keeping the PyObject owning the raw pointer alive: https://github.com/boostorg/python/blob/5e4f6278e01f018c32cebc4ffc3b75a743fb1042/include/boost/python/converter/shared_ptr_from_python.hpp#L52 Note that this code uses the shared_ptr aliasing constructor. In this way shared_ptr upcasts and downcasts work correctly, although the argument shared_ptr::use_count results tend to surprise users. Note that this mechanism is independent of the HeldType, in other words, no matter what the HeldType, it is possible to pass to shared_ptr arguments.

Downsides:

• Reference cycles are possible as a result, but shared_ptr is already susceptible to this in C++. #2757 discusses this topic a bit
• Might have a small negative effect on performance?
• Probably things I haven't thought of yet

Potential improvements:

• ~Right now this does this for all shared_ptr, might make sense to only do this for things that are python instances of python classes?~ Now only does this for things that have trampolines
• Should we document how to do this for other holders? Not sure if the value_and_holder class is something we want to expose...

Suggested changelog entry:

* Objects that are wrapped with a shared_ptr holder and a trampoline class (or alias class) are now kept alive until all shared_ptr references are deleted

[YannickJadoul]

Hi, @virtuald. Thanks for trying this out in more detail than just the slightly crazy "hack" suggested on Gitter! :-)

I'm not sure I meant for this hack to be the default way shared_ptr is always handled, though. In it's current state, I can see a few reasons why this solution could wreak (massive?) havoc:

• Decreasing the reference count needs to hold the GIL, as it can call back into the Python interpreter (e.g. when deallocating). An straightforward fix would be to acquire the GIL in that deleter, of course. Doing so might still be tricky, as it's completely hiding a call into Python and could cause hard-to-fix deadlocks, while it seems like it's a pure C++ construct. On the other hand, arguably PYBIND11_OVERRIDE already does this as well? There's also only a point in doing this if there's a trampoline class, right? Checking that might be another part of making this safer. We could add a test to demonstrate this issue?
• This will only work for shared_ptr. That's already better than it was of course, but I wonder how much confusion and bug reports this might cause ("This (unrelated) thing works with shared_ptr but not with unique_ptr.").

Starting from these two concerns and thinking further, I see a number of alternative approaches we could think about:

1. Document this hack/workaround, and maybe provide some kind of utility function.
2. Add some kind of a tag (similar to e.g. py::keep_alive) which applies this to selected arguments.
3. Come up with a small wrapper type (e.g. py::shared_python_object<T> or something like that), which uses this caster and has some kind of std::shared_ptr<T> get() method. This would make things explicit in the signature, and still make things easy by wrapping with a lambda.
4. Who knows what else there is. But given the issues from above, I'd think we need to be careful about doing things implicitly.

Reference cycles are possible as a result, but shared_ptr is already susceptible to this in C++. #2757 discusses this topic a bit

This PR doesn't really suffer from the specific reference cycles I mentioned to you on Gitter. The reference cycles are an issue when the Python object keeps a C++ object alive (which it does) while the C++ would keep the Python object alive. In this case, that's not really an issue, because there's no reference from the C++ object to the Python object; the shared_ptr is basically serving as an alias to a py::object, so we should be good.

I also know that @bstaletic was interested in pushing pybind11 towards using full-on garbage-collected heap types (as CPython is apparently pushing people away from non-GC heap types). A radically different solution, which would also work more generally, would be to do something similar to @EricCousineau-TRI's wrapper type, where the C++ trampoline class keeps a reference to the Python object and make this cycle visible to Python's GC (with tp_traverse).

I'll also tag @EricCousineau-TRI, since he will be the one who has been thinking about this by far the longest of all of us.

[YannickJadoul]

Also, I had forgotten about this, but I think I found where I got the original idea. This issue is a very interesting read: https://github.com/pybind/pybind11/issues/1049#issuecomment-326688270

[virtuald]

This will only work for shared_ptr. That's already better than it was of course, but I wonder how much confusion and bug reports this might cause ("This (unrelated) thing works with shared_ptr but not with unique_ptr.").

I believe you get an error if you pass something with a unique_ptr holder to something with a shared_ptr argument? Or at least, I tried it and I got an error, will have to check again tonight. If there was an automatic unique -> shared conversion going on somewhere, we could add this there as well.

I agree that it would be ideal if this could only be done for instances of trampoline classes. Any idea how to determine that?

[YannickJadoul]

I believe you get an error if you pass something with a unique_ptr holder to something with a shared_ptr argument? Or at least, I tried it and I got an error, will have to check again tonight. If there was an automatic unique -> shared conversion going on somewhere, we could add this there as well.

Yep, you're right, that's true; currently, you can't take away ownership from Python (though there's some work happening on that).

I agree that it would be ideal if this could only be done for instances of trampoline classes. Any idea how to determine that?

Good question. I don't have time for a thorough search, currently, but this might be harder than I thought. It might be that all this information is lost after declaring a class and its __init__ :-/

[rwgk]

Currently I don't have the free bandwidth to fully page in here, but some quick remarks:

I believe you get an error if you pass something with a unique_ptr holder to something with a shared_ptr argument? Or at least, I tried it and I got an error, will have to check again tonight. If there was an automatic unique -> shared conversion going on somewhere, we could add this there as well.

https://github.com/pybind/pybind11/pull/2672#issuecomment-748392993

pass_shared_pointee  RuntimeError: Unable to load a custom holder type from a default-holder instance

("This (unrelated) thing works with shared_ptr but not with unique_ptr.").

• unique_ptr with custom deleter: possible, if the deleter owns the py::object. This has only limited use because the unique_ptr deleter is part of the type. Additionally, the optimization compared to working with shared_ptr is probably negligible: for the extra trouble working with the unique_ptr custom deleter you only get a very small benefit in return.

• unique_ptr with default deleter: strongly on my do-not-even-try list. It's breaking the "you fully own the resources" contract that comes with unique_ptr: the new owner has no way of taking ownership of the py::object. For that to be safe, you'd need access to the std::default_delete internals, which is practically impossible as far as I know.

[YannickJadoul]

• unique_ptr with default deleter: strongly on my do-not-even-try list. It's breaking the "you fully own the resources" contract that comes with unique_ptr: the new owner has no way of taking ownership of the py::object. For that to be safe, you'd need access to the std::default_delete internals, which is practically impossible as far as I know.

Nonono, let's not go that way. Completely agreed!

The GC-type solution (similar to @EricCousineau-TRI's current solution) would solve this independently of holders, though (so also for non-shared_ptr copyable holders). But I understand if @virtuald doesn't want to dive in that deeply :-) (It's also not a guaranteed success, and comes with its own caveats on non-deterministic GC.) So that's why my other suggestion is making things more explicit, here.

[virtuald]

Unfortunately, gil_scoped_acquire is in pybind11.h, so I can't use it here. There's also a note there about how the GIL shouldn't be acquired if the interpreter is shutting down, but it's not clear to me how to avoid that problem here.

[virtuald]

Regarding grabbing the GIL in the deleter, it appears that the std::function wrapper does this already so it's not without precedent.

[virtuald]

I think this is worth doing by default, and I'm -1 on requiring a user to enable it via a tag or other hack. The current behavior of potentially losing the python half of an object when passing it to a C++ function is (while rare) clearly incorrect, unexpected, and very difficult to root cause without digging into the C++ code.

Evidence that users agree with me: #1120, #1145, #1333, #1389, #1552, #1546 (which has a similar implementation as this as a workaround!), #1566, #1774, #1902, #1937, and probably others.

I've looked for a bit, and as far as I can tell there is not currently a mechanism to determine whether a class was constructed by python or not. To record that information:

• Add a bool has_alias to type_record, set that from class_::has_alias
• Add a bool has_alias to type_info
  • Requires bumping PYBIND11_INTERNALS_VERSION
• Set type_info.has_alias in make_new_python_type

Even then, that just tells you there's an alias class associated with the python type, not whether the constructed python type was created from an alias. You might be able to add a check for that in pybind11_meta_call and set a flag in the instance. Not sure it's worth the hassle.

[virtuald]

This will only work for shared_ptr. That's already better than it was of course, but I wonder how much confusion and bug reports this might cause ("This (unrelated) thing works with shared_ptr but not with unique_ptr.").

Looking at this more, you could enable upgrading a unique holder to shared_ptr by providing an implementation of holder_retriever<std::unique_ptr> which does exactly the same thing, and removing the check_holder_compat.

Edit: ah, this is what rwgk was referring to, I think

[virtuald]

Added the gil release, and a test inspired by #2844

[virtuald]

This has been updated to only use the special deleter if an alias class is used.

I found a place to store the alias indicator that isn't currently covered by the PYBIND11_INTERNALS_VERSION guarantee -- in py::instance! ... which upon reflection, probably should be covered by it? I'm guessing that if objects from two different versions of pybind11 that did and didn't support this feature, you'd sometimes get the broken behavior and sometimes wouldn't. But that would probably be the case anyways unless we bumped PYBIND11_INTERNALS_VERSION?

Anyways, the last commit here changes init_instance to vary based on whether an alias class was used. If an alias class is used, a version of init_instance is used which sets a flag on the py::instance. This flag is used to determine whether the special deleter that keeps the python portion alive should be used or not.

[cdyson37]

Sorry I'm a bit late to the party and haven't read the above fully, but I think this does a similar thing to my (now quite stale) PR https://github.com/pybind/pybind11/pull/1566 . I never worked out how to detect if it was an alias though, and in my version I did the shared_ptr dance a bit differently, haven't had a close look so don't know which is better, but I suspect yours. Would be great to get this problem fixed once and for all!

[virtuald]

@cdyson37 yep, this solves the same problem yours does, but takes care of only doing this for aliased shared_ptrs.

[rwgk]

I found a place to store the alias indicator that isn't currently covered by the PYBIND11_INTERNALS_VERSION guarantee -- in py::instance! ... which upon reflection, probably should be covered by it? I'm guessing that if objects from two different versions of pybind11 that did and didn't support this feature, you'd sometimes get the broken behavior and sometimes wouldn't. But that would probably be the case anyways unless we bumped PYBIND11_INTERNALS_VERSION?

This is an awesome PR! My only significant concern is exactly this question. @wjakob what's the right thing to do?

I tested this PR in the Google environment with all sanitizers (ASAN, MSAN, TSAN, UBSAN) and didn't find any issues (except a couple known TSAN, UBSAN failures that already exist on master).

I think the suggested changelog is a bit unclear. It doesn't mention alias classes. Is the following true? "For classes wrapped with (A) shared_ptr as the holder, and (B) having a trampoline (aka alias class), shared_ptrs passed from Python to C++ keep the Python object alive."

[virtuald]

I think the suggested changelog is a bit unclear. It doesn't mention alias classes.

Thanks for catching that, I'll update the changelog entry. The alias class thing was added later.

[rwgk]

Hi Dustin, I will merge #2841 in a minute, which will generate a merge conflict here I think. Sorry, but it's probably easy to resolve.

If it wasn't for the pesky PYBIND11_INTERNALS_VERSION question I think this PR could get merged very soon, but we needs Wenzel's input about it, and depending on what he recommends, we may need to wait. I'm not sure.

In the meantime, what do you think about splitting out a separate PR, just for factoring out gil.h? That would make this PR easier to review (for Wenzel) and rebase.

[virtuald]

Already did that :) https://github.com/pybind/pybind11/pull/2845

[virtuald]

Rebased 2845 and this on master to make it easier.

[bstaletic]

I think this PR could get merged very soon

In that case I believe I should restate @YannickJadoul's concern.

Decreasing the reference count needs to hold the GIL, as it can call back into the Python interpreter (e.g. when deallocating). An straightforward fix would be to acquire the GIL in that deleter, of course.

I still think this is a very valid concern and I'll try to explain why I don't think the following statement from @virtuald, while true, isn't really a precedent.

Regarding grabbing the GIL in the deleter, it appears that the std::function wrapper does this already so it's not without precedent.

That is true, however, a std::function owning a python callable also needs GIL for operator(). If I'm not mistaken, it automatically acquires the GIL there too. Still, it seems obvious that, if a call is reaching into the python interpreter, you'll need GIL for one reason or another. On the other hand, if we silently start shipping a reference to self in every std::shared_ptr, what used to be a pure C++ block, suddenly might need to acquire GIL to destruct a shared pointer. If this happens in a loop... oh, boy... While std::shared_ptr's ref count is atomic and thus isn't the fastest thing in the world, it's still not GIL. The atomic ref count is local to the control block. GIL is global to the whole process and could effectively turn someone's multithreaded code into effectively single threaded. Granted, this means someone is destroying shared pointers in a tight loop, which is bad.

If my reasoning is, for whatever reason, invalid, feel free to correct me. Or call me an idiot.

To conclude, I'm not saying this isn't worth doing, but an opt-in mechanism might be a good idea.

[virtuald]

GIL is global to the whole process and could effectively turn someone's multithreaded code into effectively single threaded.

You could use this argument to justify not having trampoline classes. A python object that inherits from a C++ class and overrides a virtual function is effectively forcing C++ code that calls the virtual function to be single threaded because of the GIL. If you don't want single-threaded behavior, don't mix C++ and python, or be careful when you do.

I'll repeat what I said a few comments before and add emphasis:

The current behavior of potentially losing the python half of an object when passing it to a C++ function is (while rare) clearly incorrect, unexpected, and very difficult to root cause without digging into the C++ code.

The current behavior is just wrong. Consider this C++ class:

struct C {
  virtual std::string getLanguage() { return "C++"; }
}; 

And this python class that overrides some trampoline:

class PyC(mod.C):
  def getLanguage(self):
    return "python"

Now consider this C++ function:

std::string getLanguage(std::shared_ptr<C> c) {
  return c->getLanguage();
}

Without this patch, mod.getLanguage(PyC()) returns C++. Who would ever want that behavior?

[bstaletic]

You could use this argument to justify not having trampoline classes.

Trampolines are very explicit and boilerplate heavy, even with all the macros. You do have to opt-in. It's the implicitness of this solution that doesn't sit right with me.

The current behavior is just wrong.

Agreed, but not everyone who uses std::shared_ptr or trampolines holders is affected. I did say that a solution is worth pursuing.

[virtuald]

Trampolines are very explicit and boilerplate heavy, even with all the macros. You do have to opt-in. It's the implicitness of this solution that doesn't sit right with me.

This solution only affects users that have already opted into trampolines, and it does not affect any other users. Why should there be an additional step to opt-in?

[virtuald]

I've tweaked it slightly and added an additional test for the new conditions. Now, the keep alive feature will only be applied IFF:

• Object type has an associated alias class
• Object was constructed by python (so objects constructed by C++ will never have this applied to them)

... I'm not totally sold on the necessity of this, but perhaps this will satisfy your objections for opting in @bstaletic and @YannickJadoul

[virtuald]

One of my users has reported that this fix isn't quite there yet, they ran into an issue today where the python object disappeared again -- I wonder if the shared_ptr transfers the deleter as it's being moved around? I'll see if I can write up a duplicating test this weekend.

[rwgk]

One of my users has reported that this fix isn't quite there yet, they ran into an issue today where the python object disappeared again -- I wonder if the shared_ptr transfers the deleter as it's being moved around? I'll see if I can write up a duplicating test this weekend.

Thanks for letting us now. That sounds odd though, shared_ptr definitely remembers the deleter (I think in the control block shared by all copies of the same shared_ptr).

On another note, in the meantime Wenzel confirmed (via a chat today) that PYBIND11_INTERNALS_VERSION needs to be incremented for changes like you're making to detail::instance. I asked questions about handling the version increment, but haven't gotten feedback yet.

Yet another item: my PR #2875, which conflicts with your changes to test_smart_ptr.cpp. The main trouble is that it is messy for me to apply your PR here to the smart_holder branch, which I'd like to do, to run it through the Google global testing. Is there a chance you could move your SP structs out of TEST_MODULE, into the anonymous namespace? Or maybe even better, simply create a new test_alias_shared_ptr? I think that's best even if we didn't have the conflict between our PRs. For compatibility with the smart_holder branch it's still best to have the structs before TEST_MODULE (I'm not sure if your change also works with smart_holder as holder for you test class_es, it's something I'd like to find out, too).

[virtuald]

If PYBIND11_INTERNALS_VERSION needs to be incremented, I might want to tweak this then to push has_alias into the per-type information instead of the per-instance information. There was another approach that I was contemplating also that might be better if I can remember what it was.

... I'm happy to make the test changes you suggested, but I don't quite understand what you want? I have $DAYJOB things to do, so I won't be able to get to this before the weekend. Feel free to make the changes you want and overwrite this branch?

[rwgk]

If PYBIND11_INTERNALS_VERSION needs to be incremented, I might want to tweak this then to push has_alias into the per-type information instead of the per-instance information. There was another approach that I was contemplating also that might be better if I can remember what it was.

FWIW: I floated a suggestion to introduce something like -DPYBIND11_INTERNALS_VERSION_FUTURE that let's you jump to an #ifdefed future in situations where new features are more important than ABI stability. Being pinned down by compatibility concerns Is an issue that's popping up regularly. I think in general it's very undesirable that implementations are compromised long-term, to not have to increment the internals version (shorter-term). That may or may not apply to your case here, I don't know.

... I'm happy to make the test changes you suggested, but I don't quite understand what you want? I have $DAYJOB things to do, so I won't be able to get to this before the weekend. Feel free to make the changes you want and overwrite this branch?

Thanks!

[rwgk]

Just a quick remark: I just merged #2875. I'll rebase this branch asap (it may take me a few hours).

[rwgk]

Me not knowing better right now, I created https://github.com/pybind/pybind11/pull/2877, purely for running the CI. In the past I saw people pushing changes to my own PRs, but I don't know how they did it. @virtuald, could you please help me doing this right?