bpo-28411: Support other mappings in PyInterpreterState.modules. by ericsnowcurrently · Pull Request #3593 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation12 Commits30 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
The concrete PyDict_* API is used to interact with PyInterpreterState.modules in a number of places. This isn't compatible with all dict subclasses, nor with other Mapping implementations. This patch switches the concrete API usage to the corresponding abstract API calls.
We also add a PyImport_GetModule() function (and some other helpers) to reduce a bunch of code duplication.
Note that this code was already reviewed and merged as part of #1638. I reverted that and am now splitting it up into more focused parts.
https://bugs.python.org/issue28411
| if (PyErr_ExceptionMatches(PyExc_KeyError)) { |
|---|
| PyErr_Clear(); |
| } |
| return mod; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing the Py_XDECREF to return a borrowed reference instead of a new one.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gah! The docs for PyDict_GetItemWithError() before 3.7 don't mention it returning a borrowed reference. Thanks for catching that. I was looking for that refleak!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Looks like that wasn't the refleak I was looking for. :/ Back I go.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just pattern matching with the code above based on the assumption that PyDict_GetItem and PyDict_GetItemWithError exposed the same refcount semantics :)
However, it now occurs to me that these access patterns are thoroughly dubious if you genuinely want to allow dict subclasses and arbitrary mappings, as those may return dynamic references (e.g. from __missing__ methods) , in which case the Py_XDECREF() line may invalidate the reference entirely.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I'll fix that.
| if (PyErr_Occurred()) { |
|---|
| PyErr_Clear(); |
| } |
| Py_XDECREF(mod); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could do with a second comment saying "Return a borrowed reference instead of a new one"
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
FYI, I'm hunting down a refleak (probably in import.c):
$ ./python -m test -R 3:3 test_capi -m test.test_capi.SubinterpreterTest.test_subinterps
Run tests sequentially
0:00:00 load avg: 0.07 [1/1] test_capi
beginning 6 repetitions
123456
......
test_capi leaked [2, 2, 2] references, sum=6
test_capi leaked [1, 2, 1] memory blocks, sum=4
test_capi failed
1 test failed:
test_capi
Total duration: 220 ms
Tests result: FAILURE
test_pickle hits it too. Once I track it down and fix it I'm planning on merging.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect your reference leaks are coming from the iterators.
| PyErr_SetString(PyExc_RuntimeError, "unable to get sys.modules"); |
|---|
| return NULL; |
| } |
| i = 0; |
| while (PyDict_Next(modules_dict, &i, &module_name, &module)) { |
| PyObject *iterator = PyObject_GetIter(modules); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this iterator get cleaned up?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I would have found it eventually but you just saved me a bunch of time!
| } |
|---|
| } |
| else { |
| PyObject *iterator = PyObject_GetIter(modules); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This iterator doesn't appear to get cleaned up either.