Skip to content
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

bpo-36124: Add PyInterpreterState.dict. #12132

Merged
merged 4 commits into from
Mar 15, 2019

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