Skip to content

Document using atexit for module destructors #1169

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

Merged
merged 2 commits into from
Nov 24, 2017

Conversation

bmerry
Copy link
Contributor

@bmerry bmerry commented Nov 6, 2017

None of the three currently recommended approaches works on PyPy, due to
it not garbage collecting things when you want it to. Added an example
showing how to get interpreter shutdown callbacks using the Python
atexit module, and added a note that the other approaches are not
portable.

None of the three currently recommended approaches works on PyPy, due to
it not garbage collecting things when you want it to. Added an example
showing how to get interpreter shutdown callbacks using the Python
atexit module, and added a note that the other approaches are not
portable.
@wjakob
Copy link
Member

wjakob commented Nov 7, 2017

Objects not being garbage collected at interpreter shutdown sounds like a serious PyPy issue to me (which will also manifest itself in other situations). Rather than rewriting the current documentation, I'd prefer it note that you add a ".. note" about this flaw in PyPy, pointing out your proposed alternative approach.

The atexit example code is now only in the ..note for PyPy.
@bmerry
Copy link
Contributor Author

bmerry commented Nov 7, 2017

Okay, I've taken another stab at it (not flattened yet).

In case you want to file a bug against PyPy, here's the code I used to check the support for the different approaches in each interpreter:

#include <pybind11/pybind11.h>
#include <iostream>

namespace py = pybind11;

class A {};

PYBIND11_MODULE(destructor, m) {
    m.add_object("_cleanup", py::capsule([] () { std::cerr << "Cleanup 1\n"; }));

    py::class_<A> A_class(m, "A");
    A_class.attr("_cleanup") = py::capsule([] () { std::cerr << "Cleanup 2\n"; });

    py::cpp_function cleanup_callback(
        [](py::handle weakref) {
            std::cerr << "Cleanup 3\n";
            weakref.dec_ref();
        }
    );
    (void) py::weakref(A_class, cleanup_callback).release();

    auto atexit = py::module::import("atexit");
    atexit.attr("register")(py::cpp_function([]() {
        std::cerr << "Cleanup 4\n";
    }));
}

@wjakob
Copy link
Member

wjakob commented Nov 16, 2017

The updated PR looks good to me. Regarding PyPy: my experience has been that they much prefer bug reports which use pure CPython API.

@bmerry
Copy link
Contributor Author

bmerry commented Nov 24, 2017

@wjakob are you waiting for something before merging?

@jagerman jagerman merged commit 3b26578 into pybind:master Nov 24, 2017
@jagerman
Copy link
Member

The note seems fine to me, as well. Merged.

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.

3 participants