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

WIP Add a consume call policy #971

Closed
wants to merge 2 commits into from

Conversation

torokati44
Copy link

I tried to interface with a C++ library that has a couple of functions which take ownership of some of their pointer parameters.
I could not find a way to call them from Python using pybind11, because they kept being either double-freed or leaked.
An included test demonstrates this use case with a minimal, and not very elegant example.

This is my attempt to add a call policy named consume. It means that the ownership of any parameters marked by it is taken by the called C++ function, and must not be accessed or deleted by Python code or pybind11 after the call.
It is incomplete, because I could not figure out how to make it work in the general case, with custom holder types. I have marked the code with ??? where I tried to do something that did not work out.

Any help is much appreciated.

@jagerman
Copy link
Member

jagerman commented Aug 2, 2017

If I understand correctly, essentially what you're trying to do here is to convert an owned, held reference into an unowned, unheld reference—essentially to turn a regular object into one that would have been created had it been returned with return_value_policy::reference. There are, of course, some dangers involved in doing that (e.g. if a Python instance stays around past the point where the pointer becomes invalid), but that's just the same danger you'd get with rvp::reference in the fist place.

Some comments for fixing this up:

  • In addition to releasing the holder, you should invoke its destructor, and call set_holder_constructed(false) on the value_and_holder. (That call isn't in master yet, but will be soon as part of Allow binding factory functions as constructors #805).
  • You need to set owned = false on the instance so that the pointed-to memory doesn't get deallocated when the Python variable gets deallocated.
  • Related to that, we're now going to be in a situation where owned applies at the pointer level rather than the instance level. (Currently that can't happen because the only way to get owned = false is through a cast, but a cast can only ever give you a single-pointer instance so this isn't an issue). With Python-side multiple inheritance, this would result in the consumed pointer being unowned while other, unconsumed pointers in the instance (for the other C++ types) are still owned. Allow binding factory functions as constructors #805 adds status bits for non-simple layouts, so moving this into value_and_holder (and renaming owned to simple_owned for the simple holder layout) would probably be fairly easy.
  • I think it makes more sense to propagate this feature through the py::arg mechanism rather than needing to specify the argument number. Thus you would do something like:
    m.def("consume", [](int a, Type *b, Type2 *c) { ... },
          py::arg(), py::arg(), py::arg().disown());
    to indicate that the argument should be converted to unowned before calling the function (if not already disowned). (And of course, you could add keyword names, e.g. py::arg("c").disown()).
  • You'll have to deal with the unique_ptr assumption, and also the assumption that .release() will work. Adding a static method to holder_helper would probably work (it could also have a static_assert fallback for holder types without a release() so that attempting to use the feature on a type without a .release() wouldn't allow the feature to be specified).

Another idea worth thinking about:

  • it might be nice for symmetry to be able to go the other way, i.e. to reclaim ownership after the call (so, essentially, to convert a reference into a take_ownership), via something like py::arg().take_ownership().

@torokati44
Copy link
Author

If I understand correctly, essentially what you're trying to do here is to convert an owned, held reference into an unowned, unheld reference

Yes, either that, or just completely getting rid of the reference to the object from the Python side, if that's possible at all. However, the solution you described would be just fine too, if not better.

Otherwise, thank you for your valuable insight, I will consider your comments thoroughly, and hopefully come up with a more acceptable solution based on them in the future.

@jagerman
Copy link
Member

jagerman commented Aug 2, 2017

just completely getting rid of the reference to the object from the Python side, if that's possible at all.

Unfortunately, it isn't: we can't do anything about references to the instance held elsewhere. At best you could replace the pointer with something else, but then the question is what "something else" should be--and since it has to be some sort of object that you're going to have to duplicate, you might as well just consume the duplicate. (I suppose you could add some sort of error detection for trying to use an instance that is already cleared, but that's going a bit far into esoteric territory).

@torokati44
Copy link
Author

torokati44 commented Aug 3, 2017

I meant removing just that single reference which was passed to the function. Something like an implicit del statement with that name. If other references are held to it... Well, I don't think we can/should do much about them. But never mind, deleting is really not that important.

And if I make the reference into a non-holding weak one, is that going to apply to all Python references? Is there only a single holder for a given instance that is used by all Python references, or all references have their own holders?

@jagerman
Copy link
Member

jagerman commented Aug 3, 2017

I think the key thing to realize is that every variable in Python is essentially a shared reference; the instance that gets passed into the function is the same instance held by other copies of that variable. So yes, any changes to one variable are going to affect every other reference to that variable. The holder itself is unique, by default, but the holder is inside the object; each python variable is simply a reference to that one object.

@torokati44
Copy link
Author

Awesome, thanks! I knew how Python references work, just wasn't sure if there isn't any black magic to somehow keep track of all Python references to an object, and assign a different holder to all of them. However, in hindsight, I'm not sure why this even came up in my mind, it would be kind of silly and pointless.

@torokati44
Copy link
Author

Superseded by #1226. See details for closing there.

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

Successfully merging this pull request may close these issues.

2 participants