Fix for reference leak issue (#1029)

[wjakob]

See issue #1029

[wjakob]

@dean0x7d, @jagerman: This patch fixes the reference leak for "normal" py::init<> construction, but it seems to cause with the new factory initialization (some kind of double free?). Would you mind taking a look?

[dean0x7d]

As far as I can see, the new negative refcount issue has to do with types derived in Python:

class PyDog(m.Dog):
    pass

instance = PyDog("Molly")
del instance  # `m.Dog` is decremented twice

Looks like the issue is here in subtype_dealloc() which is the dealloc used by types created in Python. It calls basedealloc(self) and then Py_DECREF(type). But since basedealloc == pybind11_object_dealloc, it calls another Py_DECREF(type) and we end up with a negative refcount.

The solution could be to replace the Py_DECREF(type) in pybind11_object_dealloc with:

if (type->tp_dealloc == pybind11_object_dealloc)
    Py_DECREF(type);

Unfortunately, while this does fix everything in test_class, I get additional errors in test_sequences_and_iterators and test_virtual_functions, so there's more to this.

[dean0x7d]

That should do it. One issue was types derived in Python as mentioned above. The other issue had to do with types which are defined without a scope, e.g. like py:: make_iterator. The patch above fixes unscoped types by leaking the reference so they end up living forever. The only alternative I can think of is to ban unscoped types.

[wjakob]

I think that is a very reasonable fix (thank you for tracking down the details)!

[wjakob]

By the way: I noticed this issue in one of my projects where I had been using a py::capule-based module shutdown handler similar to what we currently describe in the documentation. The trouble with that approach is that instances of classes defined within the module may still be alive when the shutdown handler is called. (-> kaboom in the case of my project)

I thus switched to something like the following

py::cpp_function cleanup_callback(
    [](py::handle weakref) {
        /* ... shutdown logic .. */
        weakref.dec_ref();
    }
);
(void) py::weakref(m.attr("BaseClass"), cleanup_callback).release();

and was surprised that it stopped getting called as soon as any BaseClass-derived instance was created.

In any case, now that this is fixed, it might be worth mentioning this alternative destruction logic in the docs. What do you think?

PS: Looks like we're on hacker news front page (https://news.ycombinator.com/item?id=15095757), cool!

[wjakob]

Fantastic, all tests check out 👍

[dean0x7d]

In any case, now that this is fixed, it might be worth mentioning this alternative destruction logic in the docs. What do you think?

Seems like it would go along nicely with module destructors. As long as the refcounts are working, I think a simple setattr should work as well:

m.attr("BaseClass").attr("_cleanup") = py::capsule(cleanup_callback);

PS: Looks like we're on hacker news front page (https://news.ycombinator.com/item?id=15095757), cool!

Neat. It's certainly an interesting spike in traffic :)

[wjakob]

Seems like it would go along nicely with module destructors. As long as the refcounts are working, I think a simple setattr should work as well.

Indeed, putting the capsule into the BaseClass is actually much simpler, never mind the weak reference suggestion. I'll make a PR for the precise wording.

[wjakob]

I went ahead and squash-merged this.

[wjakob]

Upon further thought, I think it's helpful to document the weakref-based approach as well. The big downside of the capsule approach is that a _cleanup attribute is exposed on the Python side. Calling that may lead to any number of unintended & undefined behaviors/crashes. The weakref is invisible and does not suffer from that problem.

[jagerman]

I know I'm a little late (I'm in the middle of a move, and will probably be mostly inactive for the next 2-3 days as well), but the fix looks good; 👍 to @dean0x7d for tracking that down!