-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Free tstate on python 3.7+ on finalize_interpreter #2020
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
Conversation
269327b
to
5eb948e
Compare
Rebased after travis fixes. Tests are now green. |
Hi @bstaletic, indeed I think that it would be more appropriate to put this into the destructor. Consequently, the PYBIND11 ABI version should be updated. (there is a constant indicating this in one of the header files). |
We'd need to make sure |
If |
We might get away with it, but it wouldn't be very clean. Also, another reminder: there's an explicit comment saying But actually, are we sure none of the objects That being said, it would be nice to have the |
Is that allowed? If |
5eb948e
to
3bcb93a
Compare
@wjakob I've updated the pull request. It turns out that using the destructor to clean up |
3bcb93a
to
16fa1f1
Compare
Ok, looks good to me -- thank you. |
Right now pybind leaks a single pointer worth of memory when embedding the interpreter. This is because, with the new python 3.7 thread C API,
tstate
in theinternals
struct needs to be freed with a call toPyThread_tss_free()
.This PR frees the block inside the
finalize_interpreter
, where all the cleanup seems to be located.An alternative solution is adding
~internals() { PyThread_tss_free(tstate); }
, which would make it impossible for pybind to forget to freetstate
ever again. However this solution has ABI implications. A destructor that isn't implicitly generated or explicitly defaulted makes the type non-trivial, which changes the calling convention rules. I'm fine with moving the code to the destructor if @wjakob thinks that is more appropriate.Finally, whatever soluton we settle on, this only affects python 3.7+.
Ping @YannickJadoul
EDIT: The valgrind reported stack trace of the leak: