gh-123504: Fix reference leak in initialization of _tkinter#123505
gh-123504: Fix reference leak in initialization of _tkinter#123505vstinner merged 9 commits intopython:mainfrom
_tkinter#123505Conversation
|
What is the output |
|
Modules/_tkinter.c
Outdated
|
|
||
| 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
| 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. |
vstinner
left a comment
There was a problem hiding this comment.
LGTM, it works as expected.
Without this PR:
$ ./python -Xshowrefcount -c "import tkinter"
[130 refs, 78 blocks]
With this PR:
$ ./python -Xshowrefcount -c "import tkinter"
[0 refs, 0 blocks]
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>
| 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. |
| 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