Fix Python object lifetimes associated with shared_ptrs

[cdyson37]

This is a straw man solution to #1546, based on the patch included in the report. It also fixes #1333 and #1389 (note that "I was deleted" doesn't appear unless you conduct the test inside function scope).

Probably a pre-requisite for #1552 but does not fix it (I have to be careful wording this or github gets the wrong idea).

Questions:

• This means that every py::object->std::shared_ptr cast keeps the py::object alive. This appears to be sufficient to solve the problem, but it might not be entirely necessary. For instance, some shared_ptr casts might be associated with the original base class, and not require the py::object to be kept around. Suggestions for limiting the scope of the fix welcome.
• Does the test look okay? I'm happy to rename, move or change it in whatever way people think best.

[iwanders]

Thanks for this solution to the problem, I used it in a multithreaded situation and ran into segfaults during destruction from the C++ side. Turns out that if the last pointer gets destroyed from another thread, the Python object may be cleaned up without having the GIL. This will caused reproducible segfaults in my case. Modifying line 1552 to provide a de-allocator lambda like:

 auto py_obj_ptr = std::shared_ptr<py::object> (new py::object{py_obj}, [](auto py_object_ptr) {
  pybind11::gil_scoped_acquire gil;
  delete py_object_ptr;
});

Will ensure we hold the GIL when we delete the Python object. In my case it did not matter whether that object had a __del__ method, the segfault occurs regardless. With this small modification this solution works great. 🎉

[cdyson37]

@iwanders - good point! I've incorporated that. For a horrible moment I thought I would have to move gil_scoped_acquire to its own header to work around a circular dependency, but at instantiation time the definition of that class has already been seen, so I settled for a forward declaration.

[mmodenesi]

Is this line of work still active? I'm really interested in a solution to this problem and before I copy paste or taylor my own hack, I would like to know what are the chances of something like this being incorporated to mainstream. Thanks so much!

[eacousineau]

Just to be sure, could you add a pytest.gc_collect() here?

[eacousineau]

Is there a way that you can check and ensure that all objects get destroyed when the go out of scope?

See usages of ConstructorStats for how to do so - I'm happy to point out more examples!

[eacousineau]

(This is to address your point on limiting scope, or at least documenting the scope in code)

I'm still not sure of is if this creates a sort of reference cycle, and effectively cannot be GC'd until the interpreter dies... If it does this, it'd be nice to show it by testing the number of instances alive?

[eacousineau]

This looks great!
Is there anything I can do to help?

FWIW I had started out writing a repro on master here (no fix):
master...EricCousineau-TRI:issue-1389-alias-repro
This was before I actually read the issue thread and found this PR :P I dunno if it's helpful, as it's basically a longer version of this (but just based on the doc example of Cat / Animal).

[eacousineau]

@wjakob @rwgk Any chance we could snag some of y'all's time to look at this PR?

[eacousineau]

FTR Here are some (if not all?) of the issues that could be truly fixed by this PR (I think)

#1120
#1145
#1333
#1389
#1546
#1552
#1774
#1902
#1937

\cc @pvallet

[wjakob]

I agree that this is a severe problem that we should definitely fix. One aspect to consider is that only a small subset of users that rely on std::shared_ptr will also provide trampolines (I would generally consider this a fairly advanced feature of pybind11). The current PR penalizes everyone, and will therefore need further adjustments.

I think it may be possible to find out whether the underlying type uses a trampoline or not and only run the special case then. See in particular the has_alias variable in the pybind11::class_ constructor. This might need to be stashed so that it's available when you need it.

Other aspects: running the full base caster to make a std::shared_ptr that is then destructed later strikes me as inefficient about the current implementation.

Quite a bit of this code doesn’t depend on the template type. One issue with a template library like pybind11 is that this code will be replicated thousands of times when creating bindings for large codebases. Thus, whenever possible, it’s very useful to factor out things into non-templated functions that are called from more specific template classes.

[cdyson37]

Hi @wjakob thanks for taking a look! As I say in the original PR, it's just a test and a straw man solution; no doubt it can be much improved. Likewise thank you to @eacousineau for taking a look and for suggestions.

Where should we go from here? Do you or one of the more regular developers want to have a go at improving this fix, or coming up with a different one? Or I can have a go at improving it? I'm more than happy to leave it to an expert though.

[virtuald]

Hadn't noticed this previously, but superceded by #2839

[cdyson37]

Let's close this in favour of #2839 - thanks to everyone who commented, hopefully the other PR will be merged soon!