gh-123504: Fix reference leak in initialization of _tkinter
#123505
Conversation
|
What is the output |
|
Modules/_tkinter.c
Outdated
| @@ -3419,8 +3419,9 @@ PyInit__tkinter(void) | |||
| #endif | |||
|
|
|||
| Tkinter_TclError = PyErr_NewException("_tkinter.TclError", NULL, NULL); | |||
| if (PyModule_AddObjectRef(m, "TclError", Tkinter_TclError)) { | |||
| if (PyModule_AddObject(m, "TclError", Tkinter_TclError)) { | |||
There was a problem hiding this comment.
Tkinter_TclError should be a strong reference to _tkinter.TclError. Even if you remove "TclError" from the the module dict, the use of Tkinter_TclError should not crash.
BTW, do not use PyModule_AddObject(), it is broken beyond repair. Use PyModule_AddObjectRef() or PyModule_Add(), what is more convenient.
There was a problem hiding this comment.
Ok, where should the strong reference to Tkinter_TclError be decref'd? Should we add a module clear function?
Modules/_tkinter.c
Outdated
| @@ -3474,18 +3475,21 @@ PyInit__tkinter(void) | |||
| Py_DECREF(m); | |||
| return NULL; | |||
| } | |||
| Py_DECREF(Tkapp_Type); | |||
There was a problem hiding this comment.
It is the same for all other global references (otherwise why would they be global references?).
Yes, if you want to release these references, you need a module clearing function. This is not a big deal, because usually when you unload the module, you quit the program, so who cares about few dangling references?
|
FWIW, it might be worth trying to switch |
I had those same concerns when dealing with curses in #123291 but since the module is a bit of a mess, I just focused on getting the correct DECREF when needed. |
Misc/NEWS.d/next/Library/2024-08-30-09-01-35.gh-issue-123504.lJ9_BB.rst
Outdated
Show resolved
Hide resolved
…J9_BB.rst Co-authored-by: Victor Stinner <vstinner@python.org>
|
|
||
| static struct PyModuleDef _tkintermodule = { | ||
| PyModuleDef_HEAD_INIT, | ||
| "_tkinter", | ||
| NULL, | ||
| -1, |
There was a problem hiding this comment.
You removed .m_size = -1, but it's ok, m_size=0 also means "no module state".
There was a problem hiding this comment.
There is a difference between 0 and -1 related to subinterpreters.
There was a problem hiding this comment.
From the documentation:
Setting ``m_size`` to ``-1`` means that the module does not support
sub-interpreters, because it has global state.There was a problem hiding this comment.
Oh I see, I forgot about this difference :-(
|
I enabled auto-merge. Thanks for the PR updates. I don't think that such "leak" at Python exit is important enough to justify a backport. |
|
|
||
| static struct PyModuleDef _tkintermodule = { | ||
| PyModuleDef_HEAD_INIT, | ||
| "_tkinter", | ||
| NULL, | ||
| -1, |
There was a problem hiding this comment.
There is a difference between 0 and -1 related to subinterpreters.
| NULL | ||
| .m_name = "_tkinter", | ||
| .m_methods = moduleMethods, | ||
| .m_clear = module_clear, |
There was a problem hiding this comment.
If you define "clear", you should always define "travel".
There was a problem hiding this comment.
Would it make sense to emit a warning or even raise an error at runtime?
For example in Objects/typeobject.c, I added an assertion in _PyType_CheckConsistency():
if (type->tp_flags & Py_TPFLAGS_HAVE_GC) {
// bpo-44263: tp_traverse is required if Py_TPFLAGS_HAVE_GC is set.
// Note: tp_clear is optional.
CHECK(type->tp_traverse != NULL);
}There was a problem hiding this comment.
Looks like a good idea. For both types and modules.
_tkinterleaks type references on initialization #123504