Don't construct unique_ptr around unowned pointers #478
Conversation
If we need to initialize a holder around an unowned instance, and the holder type is non-copyable (i.e. a unique_ptr), we currently construct the holder type around the value pointer, but then never actually destruct the holder: the holder destructor is called only for the instance that actually has `inst->owned = true` set. This seems no pointer, however, in creating such a holder around an unowned instance: we never actually intend to use anything that the unique_ptr gives us: and, in fact, do not want the unique_ptr (because if it ever actually got destroyed, it would cause destruction of the wrapped pointer, despite the fact that that wrapped pointer isn't owned). This commit changes the logic to only create a unique_ptr holder if we actually own the instance, and to destruct via the constructed holder whenever we have a constructed holder--which will now only be the case for owned-unique-holder or shared-holder types.
jagerman
force-pushed
the
no-unowned-unique-holder
branch
from
9a5a089 to
79e55b5
Compare
|
It would probably be best to add a test which specifically shows the problem (and will prevent regressions in the future). |
|
I should add: I share your concern about invoking a constructor and never matching it with a destructor. It's not an issue for move-only |
Well, anything that had an stl container or an I'll write up a test for it. |
The three alive assertions now pass, before pybind#478 they fail with counts of 2/2/1 respectively, because of the unique_ptr that we don't want and don't destroy (because we don't *want* its destructor to run).
|
Test added. |
Small cleanup to the pybind#478 test code, and fix to the ConstructStats documentation (the static method definition should use `reference` not `reference_internal`).
|
Looks good to me. |
| if (inst->owned) { | ||
| new (&inst->holder) holder_type(inst->value); | ||
| inst->constructed = true; | ||
| } | ||
| } | ||
|
|
||
| /// Initialize holder object of an instance, possibly given a pointer to an existing holder | ||
| static void init_holder(PyObject *inst_, const void *holder_ptr) { | ||
| auto inst = (instance_type *) inst_; | ||
| init_holder_helper(inst, (const holder_type *) holder_ptr, inst->value); |
There was a problem hiding this comment.
Wouldn't it be more logical to remove all the conditionals and flag-setting from the init_holder_helpers and rewrite this function as:
auto inst = (instance_type *) inst_;
if (inst->owned)
init_holder_helper(inst, (const holder_type *) holder_ptr, inst->value);
inst->constructed = true;
There was a problem hiding this comment.
No: we want inst->constructed set if and only if we actually constructed a holder, and we construct a holder if:
- The holder type is copyable (i.e. shared_ptr)
- The holder type is not copyable and we own the instance
But in order to determine whether we're in case 1 or 2 depends on which init_holder_helper is actually invoked.
There was a problem hiding this comment.
i.e. we want constructed = false when we end up in variant 3 with owned = false, but we still want to go into variant 1 and 2 and construct a shared holder even if owned = false.
There was a problem hiding this comment.
Though it might be worth renaming ->constructed to ->holder_constructed to make the code slightly more self-documenting.
This makes it clearer exactly what it's referring to.
|
This looks all good then, thank you. It's been so long since I wrote this code, I got confused by it myself. |
|
Yeah--I tried to look at the git history for it when looking at this, but it has been basically exactly as is since the initial git commit. |
This is a continuation of the discussion in #471 re: unique pointers. I may be wrong about this, but the more I think about it, the more I think there is a problem here to be fixed w.r.t. non-copyable holders (i.e.
unique_ptrs). It isn't wrong in terms of when it destructs the pointed-at object, but it is wrong in that it is constructing holder types that are freed without having been destructed.Right now, if we need to initialize a holder around an unowned instance, and the type's holder type is a
unique_ptr(or similar, non-copyable holder), we currently construct the holder around the value pointer, but then never actually destruct that holder. Wheninst->owned = false, we never invoke the part ofdeallocthat calls the holder destructor: that's called only for the instance that actually hasinst->owned = trueset, which it isn't in this case.Moreover we don't want it to be destructed: as I found out by modifying
dealloc, if we actually invoke its destructor we get multiple frees because we have multipleunique_ptrs around the same pointer: and of course we only want the destructor to happen for the owned one.This seems like a bug, though probably not one that matters with
unique_ptr: we're creating a holder type, but then never destroying it (instead we end up just freeing its memory (viatp_free) without calling the destructor when we dealloc the instance). For unique_ptr this probaby doesn't matter in practice, but it's not hard to imagine some custom non-copyable holder type where freeing the type without invoking the destructor is a problem (and even if it happens to work forstd::unique_ptrin practice, it still seems pretty wrong).This commit changes the
init_holder_helperlogic to only construct aunique_ptr-type holder only when we actually want one--i.e. when we actually own the instance--and changesdeallocto destruct the constructed holder whenever we have one. This still does the right thing w.r.t. pointed-at instances--the difference now is that onlyownedinstances get aunique_ptrholder: for unowned instances (with unique holder types) we just keep a pointer and leave the holder type uninitialized. Thus this should result in the same logic we have at present, but avoids the problem of allocating holder instances that we never properly destruct.