Skip to content

Conversation

@ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Mar 1, 2019

This parallels PyThreadState.dict. Likewise we add PyInterpreterState_GetDict().

https://bugs.python.org/issue36124

@ericsnowcurrently
Copy link
Member Author

FWIW, I'm not convinced we should add this (as I noted on the tracker issue). So I've marked the PR DO-NOT-MERGE until that question has been resolved.

@arigo
Copy link
Contributor

arigo commented Mar 2, 2019

interp->dict leaks when the PyInterpreterState is destroyed. Please Py_CLEAR() it! Also, make sure it is done at the very last moment; otherwise, race conditions can recreate interp->dict after it was cleared, and then that would leak.

@ericsnowcurrently
Copy link
Member Author

@arigo, thanks for noticing that. Something was nagging at me and I couldn't figure out what. You saved me some time. :) That's what I get for hurrying out a PR at the end of the day!

When you are talking about "at the very last moment", you mean after everything has been cleared which might cause interp->dict to get re-created during its __del__()? Isn't that mostly true of everything that gets cleared in PyInterpreterState_Clear()? Regardless, I'm clearing it almost at the end. Only the "at-fork" handlers are later.

@ericsnowcurrently ericsnowcurrently force-pushed the capi-interp-dict branch 2 times, most recently from 8aca567 to 5e576bb Compare March 15, 2019 23:20
@ericsnowcurrently ericsnowcurrently merged commit d2fdd1f into python:master Mar 15, 2019
@ericsnowcurrently ericsnowcurrently deleted the capi-interp-dict branch March 15, 2019 23:47
@arigo
Copy link
Contributor

arigo commented Mar 16, 2019

Thanks! At least, the cffi tests pass with PyInterpreter_GetDict().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants