Towards a solution to move holder pointers from Python to C++

[rhaschke]

This PR is reopening #2046, which was closed by @wjakob due to security objections moving objects from Python to C++.
However, recently there was new interest in this topic, see e.g. #2583, #2672 and issue summary in #2646.
I still believe moving can be safely done with some precautions. @rwgk, please feel free to augment this PR.

[YannickJadoul]

Can we please take things step by step, and keep a bit of order? As you said, there's a plethora of other existing issues and PRs in various states, and no further discussion in #2646 - let alone agreement - on where to go with this. Duplicating a PR (closed by @wjakob) is not going to give a better overview of the possibility; to be fair, it just makes the whole topic even more daunting.

[rhaschke]

Sorry, I was explicitly asked by @rwgk to reopen this PR as a basis for his work in #2672. I fully agree with you, that we should tackle steps 1+2 of #2646 first. If time permits, I will continue on #2047 and related issues.
By the way, step 1 is - in my humble opinion - solved: #2644 is awaiting a review 😉

[YannickJadoul]

Sorry, I had absolutely no clue there. Do you know why? #2046 still contains the old code and ideas (I presume?) and doesn't "pollute" (well, it's not that bad, but it's getting confusing when a dozen of similar PRs and issues on the same topic are opened) the list of PRs.

[YannickJadoul]

By the way, step 1 is - in my humble opinion - solved: #2644 is awaiting a review wink

About this: I don't really have time over the next 1.5 weeks, but I can have a look at it after. Quickly glancing at it, do be warned that I'm not sure how happy all users (and @wjakob) will be to pay a memory/runtime overhead for all instances/accesses, though.

[rwgk]

Sorry, I had absolutely no clue there. Do you know why? #2046 still contains the old code and ideas (I presume?) and doesn't "pollute" (well, it's not that bad, but it's getting confusing when a dozen of similar PRs and issues on the same topic are opened) the list of PRs.

Sorry @YannickJadoul, end of last week I looked if I could reopen #2046, but the button was grayed out, with hover-over "The rvalue-argument-fix2 branch was force-pushed or recreated."

I'm experimenting myself a bit and want to refer back to Robert's work, hence my suggestion to open a new draft PR.
I realize it's messy at the moment, but with all the loose ends hanging around for years, and even in @eacousineau's fork for years, I figure it's unlikely to be easy to tie them all together.

[rwgk]

Hi Robert, here is a demo for the "premature delete" I mentioned under #2672 (comment):

rwgk@0c395cf (this includes a snapshot of your change here)

The demo (test_premature_delete.py) writes output to std::cout. I get:

pointee::get_int()
before give up
~pointee()
after give up
before del
after del

The problem is that ~pointee() appears before after give up. I'm expecting it only between before del and after del.

[rwgk]

Hi Robert, does 6b3c9b5 fix the "premature delete" issue (#2687 (comment))?

[rhaschke]

Hi Robert, does 6b3c9b5 fix the "premature delete" issue (#2687 (comment))?

No, sorry. I didn't have time to progress on your problem. The additional commit solves an issue when you have a complex chain of argument passing:

• Creating a class instance from C++, passed to Python
• Moving this instance to another C++ class, invalidating it in Python
• Fetching a reference from C++ to the originally moved instance

As pybind11 stores a map instance -> Python wrapper (in get_internals().registered_instances), the 3rd step was finding the registered instance and re-using the already invalidated wrapper...

[rhaschke]

Trying to progress on my own challenges, I noticed that unregistering the instance after moving essentially disables the possibility to use python-inherited classes in the above-mentioned pipeline. So, we need another mechanism to indicate that an instance has been moved from Python to C++.

[rhaschke]

Hi Robert, does 6b3c9b5 fix the "premature delete" issue (#2687 (comment))?

Hi @rwgk, I just looked into your test case and indeed this last commit solves that issue as well. See here:
https://github.com/rhaschke/pybind11/tree/pr2687_move_holder

[rhaschke]

Hi @rwgk, in order to be able to pass around python-defined classes, we must not deregister the instance in pybind11. This mechanism is required to associate the raw pointer with the wrapper pointer and thus corresponding python extension. Not sure, how that worked in boost::python.
My next idea is to change the unique_ptr's deleter during the move, to actually call tp_dealloc of the python (wrapper) instance. Maybe you can try to get this running?

[rwgk]

Hi @rhaschke, not directly related to your work here, except for me to understand the current relevant pybind11 behavior, I created this:

https://gist.github.com/rwgk/4fe600fad45442a5d790b53195a4da51

Sharing it here JIC someone finds it useful. It's basically just a detailed look at the issue reported under #1138.

Not sure, how that worked in boost::python.

I don't know, too.

My next idea is to change the unique_ptr's deleter during the move, to actually call tp_dealloc of the python (wrapper) instance.

For unique_ptr, the deleter is part of the type (unlike smart_ptr), therefore my first thought is that this is tricky if not infeasible.

[EricCousineau-TRI]

Yeah, we had run into this as well for our fork :(
We fixed it here: RobotLocomotion#11

Basically, I think you can just add a destructor which will allow pybind11 to reclaim ownership?