This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author obserience
Recipients epaine, gpolo, obserience, pitrou, serhiy.storchaka
Date 2020-06-17.19:59:30
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1592423971.19.0.303124870982.issue39093@roundup.psfhosted.org>
In-reply-to
Content
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.
History
Date User Action Args
2020-06-17 19:59:31obseriencesetrecipients: + obserience, pitrou, gpolo, serhiy.storchaka, epaine
2020-06-17 19:59:31obseriencesetmessageid: <1592423971.19.0.303124870982.issue39093@roundup.psfhosted.org>
2020-06-17 19:59:31obseriencelinkissue39093 messages
2020-06-17 19:59:30obseriencecreate