Clean up module import handling in PyInit__core - #1081
Conversation
Merging this PR will not alter performance
Comparing 
Footnotes
|
| if (temp_obj == NULL) | ||
| return NULL; | ||
| goto error; | ||
| if (!PyType_Check(temp_obj)) { | ||
| Py_DECREF(temp_obj); | ||
| PyErr_SetString(PyExc_TypeError, "enum.EnumMeta should be a type"); | ||
| return NULL; |
There was a problem hiding this comment.
These are still handled manually, as they work a bit different from the rest. There's only 2 cases of them though, so I felt introducing a macro wasn't worth it
There was a problem hiding this comment.
@sobolevn I'm wondering if we should just remove these checks? They could only ever be reached if someone patched these classes in the stdlib for some reason, where it would then fail further down the road if these aren't types. We're also not doing these kinds of assertions anywhere else in this block
There was a problem hiding this comment.
Removing them would make this much cleaner, because then we could use the same pattern as everywhere else
There was a problem hiding this comment.
Actually, @ofek it seems you added those. Was there a particular reason for this? If yes, we should probably document it, otherwise I'd say we remove them
There was a problem hiding this comment.
I don't recall but if CI passes with their removal then feel free to merge!
There was a problem hiding this comment.
Tests at least pass locally. I'll try removing them
49a70c5 to
3b22041
Compare
3b22041 to
d382e12
Compare
Co-authored-by: sobolevn <mail@sobolevn.me>
b44a139 to
f7829ec
Compare
There was a problem hiding this comment.
Did a deep pass on bd95764 (code walk of every failure path + an empirical check). Substance looks good, I'd like to get this in.
Both claimed leaks are real and fixed. On the base, the uuid and decimal blocks are the only two that never Py_DECREF(temp_module) (the reference is overwritten by the next PyImport_ImportModule), and they're exactly what LSan would flag on a plain import. Verified empirically too: with the merge-base and this branch built from the same parent, sys.getrefcount(sys.modules["uuid"]) and ...["decimal"] drop from 4 to 3 while every other module stays identical. Beyond those two, the unified error label also closes the ~20 conditional error paths that used to drop a live temp_module on early return NULL - walked all ten import blocks, the ownership chain converges on every path, no double-DECREF (the SET_TEMP_OBJ/IMPORT_TEMP_MODULE XDECREF-before-replace handles reentry, and the error label's XDECREFs are NULL-safe for the early PyType_Ready gotos). Full unit suite is green on a local build.
On the open items:
- The conflict with main is a single hunk from #962: keep
SET_REF(convert_generic_alias, "convert_generic_alias")in the_utilsblock and drop the now-redundantPy_DECREF(temp_module). Nothing from #1052/#1097/#1109 touchesPyInit__core, so it's a one-line resolution. - The
EnumMeta/ABCMetaPyType_Checkremoval you mentioned trying (and ofek pre-approved with "if CI passes... feel free to merge") never got pushed. Either way works for me - push the removal here, or merge as-is and drop them in a follow-up. I'd just rather not let a green refactor sit for another two weeks.
After the rebase this is an approve from me.
Clean up handling of module imports in
PyInit__core:errorlabel to ensure temporary objects are cleaned up (previously we often didreturn NULLearly, missingPy_XDECREFin a few places)SET_MODULE_REFmacro to uniformly handle the "callPyModule_AddObjectRef, check for errors" patternIMPORT_TEMP_MODULEto uniformly handle the "import module astemp_module, get attribute fromtemp_moduleastemp_obj, and assign it to the module state"Removes 2 leaks found with
LSanon (checked locally, as it's turned off in CI)Based on @sobolevn's suggestion here: #1059 (review)