Message371773
I looked into this in greater depth a while back and never followed up.
In "Modules/_tkinter.c" the Tkapp_Dealloc() function has a commented out check that would ensure it isn't called from the wrong thread.
Tkapp_Dealloc(PyObject *self)
{
PyObject *tp = (PyObject *) Py_TYPE(self);
/*CHECK_TCL_APPARTMENT;*/
ENTER_TCL
Tcl_DeleteInterp(Tkapp_Interp(self));
LEAVE_TCL
PyObject_Del(self);
Py_DECREF(tp);
DisableEventHook();
}
If this is uncommented, garbage collection from a non-main thread will fail and the object will be leaked.
Adding a warning would be a good idea so the user knows that a memory leak has occurred.
**Doing this makes the code safe.** No more crashes. It should be done a bugfix regardless of anything else.
####################
>> I propose that the Tkapp object can only be created in the main thread
Restricting interpreter object creation/destruction to the main thread is very much an API change that would break any code that puts the interpreter in a non-main thread.
Since we're keeping the objects around till the interpreter exits we still "leak" the memory and calling the destructor before the process exits has no benefits. Directly leaking the memory isn't any worse in terms of memory consumption and doesn't break currently working code.
####################
My opinion is we should leak the memory, warn the user and forget about it. TCL interpreters are tiny and anyone who was doing this before was crashing their application anyway.
A nice to have would be changing the python tkinter code to cleanup the references when the associated GUI exits (as it arguably should)
--root.quit() doesn't do what the docs say and is regularly abused(ignore it)
--root.destroy() is more clear cut and a better candidate for adding cleanup behavior
implementations:
1)explicitly manage multiple references to the interpreter removing when appropriate (messy)
--remove widget.tk refs on widget.destroy()
--remove variable._tk when root.destroy() called (requires tracking variables with weakrefs)
2)replace root.tk with a weakproxy at initialisation and everything references the weakproxy (no other code changes)
3)replace widget.tk and variable._tk with a property that fetches root.tk
--no weakrefs or explicit cleanup needed
--**I favor this options**
For all implementations we need code to handle post-cleanup tk retrieval failure:
--Calls to root.quit() and other NOP equivalent methods should be ignored
--Variable sets should be ignored but trigger a warning
--Variable reads should RuntimeError since the TCL interpreter is dead
The user can shoot themselves in the foot by grabbing an explicit reference to the TCL interpreter but I've never seen that done and now it only causes a memory leak (which we can warn about).
Again, the python changes are nice to have but the "Modules/_tkinter.c" "CHECK_TCL_APPARTMENT;" macro needs to be uncommented.
I'd be happy to do the python side-changes if that's likely to be accepted. |
|
Date |
User |
Action |
Args |
2020-06-17 19:59:31 | obserience | set | recipients:
+ obserience, pitrou, gpolo, serhiy.storchaka, epaine |
2020-06-17 19:59:31 | obserience | set | messageid: <1592423971.19.0.303124870982.issue39093@roundup.psfhosted.org> |
2020-06-17 19:59:31 | obserience | link | issue39093 messages |
2020-06-17 19:59:30 | obserience | create | |
|