Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support transfer of ownership of Python subclasses of pybind11 C++ classes with shared_ptr<>, avoid slicing #1145

Closed
EricCousineau-TRI opened this issue Oct 14, 2017 · 5 comments
Labels
holders Issues and PRs regarding holders

Comments

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Oct 14, 2017

Update (2020-12-22): Simplest reproduction can be seen in #1333


If a Python subclass of a pybind11 C++ class, with holder_type = shared_ptr<T>, has a shared_ptr<> that exists in C++ when its Python ref_count() goes to zero, Python will destroy the Python extension, and leave the C++ Alias type (e.g. PyBase, using PYBIND11_OVERLOAD*) functionality bare (and actually aliased / truncated affected by object slicing).

This could be prevented if a custom __del__, PyTypeObject::tp_del, or PyTypeObject::tp_dealloc is installed such that the instance could be resurrected if holder.use_count() > 1 for the given value-holder, and the ownership is released to C++.
(__del__ is the easiest, as Python explicitly mentions that you can increase the refcount of a destructing object and save it from death.)

This is related to #1132.

@ppt-adsk
Copy link

ppt-adsk commented Nov 7, 2017

Hello,

I am part of the Autodesk Maya team. We are using pybind11 for an upcoming project, and are hitting this exact problem (Python subclass of a pybind11 C++ class, holder_type = shared_ptr, shared_ptr in C++ with Python ref_count of 0, resulting in an object without the Python derived functionality).

We are not familiar with the details of your proposal, but are extremely interested in a solution to the problem, as for us it currently involves kludgey Python-side referencing of objects to keep their ref_count above 0). Thanks for trying to move this forward.

@EricCousineau-TRI
Copy link
Collaborator Author

Thanks for posting your use case!

I have tried to explain how the main portion of the WIP PR would work here:
https://github.com/EricCousineau-TRI/pybind11/blob/feature/py_move_shared-wip/docs/advanced/classes.rst#virtual-inheritance-and-lifetime
Please let me know if you have any questions about that, and please do let me know if you happen to find any better solutions.

@cgerum
Copy link

cgerum commented Feb 15, 2018

I also ran into this Problem. For now i think I can work around this problem. But I would by Interested in an easier solution. Your proposal seems fine with me.

Can I be of any assistance moving this forward?

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Feb 17, 2018

Can I be of any assistance moving this forward?

Most certainly! If you're up to it, could you try using this branch, and provide your feedback?
https://github.com/RobotLocomotion/pybind11/tree/drake

It incorporates a working prototype of this feature (with a few other features). If this implementation is something that is actually useful for you, it can help make a better case for upstreaming the feature.

If you diff the docs against the merge-base of the upstream master, you'll see what it introduces:
RobotLocomotion/pybind11@upstream...RobotLocomotion:drake#diff-47ee721743764a0358552e687404eb8e

This code is presently being used as part of an online course related to the software we are developing, and it seems to be treating us pretty well:
http://underactuated.csail.mit.edu/Spring2018/

EDIT: Note that the changes I've made to this fork breaks the AppVeyor Windows builds. Since I don't have a Windows machine setup and since we aren't using Windows for our applications, I haven't tried delving in to fixing this. Do you use Windows?

@EricCousineau-TRI
Copy link
Collaborator Author

Closing this in lieu of #1333 for tracking purposes.

(This issue has scope creep from bad wording hehe 😬)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
holders Issues and PRs regarding holders
Projects
None yet
Development

No branches or pull requests

3 participants