Uh oh!
There was an error while loading. Please reload this page.
gh-123504: Fix reference leak in initialization of _tkinter - #123505
Conversation
picnixz
commented
Aug 30, 2024
What is the output |
ZeroIntensity
commented
Aug 30, 2024
|
| 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?
| 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?
ZeroIntensity
commented
Aug 30, 2024
FWIW, it might be worth trying to switch |
picnixz
commented
Aug 30, 2024
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. |
Uh oh!
There was an error while loading. Please reload this page.
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]
Uh oh!
There was an error while loading. Please reload this page.
…J9_BB.rst Co-authored-by: Victor Stinner <vstinner@python.org>
Uh oh!
There was an error while loading. Please reload this page.
| 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 :-(
vstinner
commented
Sep 3, 2024
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