bpo-28411: Support other mappings in PyInterpreterState.modules. - #3593
Conversation
Sorry, something went wrong.
| if (PyErr_ExceptionMatches(PyExc_KeyError)) { | ||
| PyErr_Clear(); | ||
| } | ||
| return mod; |
There was a problem hiding this comment.
This is missing the Py_XDECREF to return a borrowed reference instead of a new one.
Sorry, something went wrong.
There was a problem hiding this comment.
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!
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm. Looks like that wasn't the refleak I was looking for. :/ Back I go.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Good call. I'll fix that.
Sorry, something went wrong.
| if (PyErr_Occurred()) { | ||
| PyErr_Clear(); | ||
| } | ||
| Py_XDECREF(mod); |
There was a problem hiding this comment.
This could do with a second comment saying "Return a borrowed reference instead of a new one"
Sorry, something went wrong.
There was a problem hiding this comment.
Will do.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
ncoghlan
left a comment
There was a problem hiding this comment.
I suspect your reference leaks are coming from the iterators.
Sorry, something went wrong.
|
|
||
| i = 0; | ||
| while (PyDict_Next(modules_dict, &i, &module_name, &module)) { | ||
| PyObject *iterator = PyObject_GetIter(modules); |
There was a problem hiding this comment.
Where does this iterator get cleaned up?
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you! I would have found it eventually but you just saved me a bunch of time!
Sorry, something went wrong.
| } | ||
| } | ||
| else { | ||
| PyObject *iterator = PyObject_GetIter(modules); |
There was a problem hiding this comment.
This iterator doesn't appear to get cleaned up either.
Sorry, something went wrong.
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