Issue 3638: Remove module level functions in _tkinter that depend on TkappObject (original) (raw)
Created on 2008-08-21 22:00 by vstinner, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Messages (26)
Author: STINNER Victor (vstinner) * 
Date: 2008-08-21 22:00
mainloop() is a method of a Tkapp object, but it's also a method of _tkinter module. In TkApp_MainLoop, self is seen as a TkappObject whereas it can be a module object! So instruction like "self->dispatch=1" will replace a random byte in the module object (eg. ob_type attribute). Example to crash: import gc import _tkinter _tkinter.mainloop() gc.collect()
It always crashs in my Python 3.0, but not in Python 2.6.
Solution: just remove tkinter.mainloop() => it should have no side effect since users use tkinter (without the "") which only uses Tkapp.mainloop() (the right way to use Tkapp_MainLoop() function). Solution "implemented" in the patch.
Author: Guilherme Polo (gpolo) *
Date: 2008-09-13 14:19
Looks fine to me. But I can't see the reason to keep this as a module function in python 2.6 so I would remove it there too.
Author: STINNER Victor (vstinner) *
Date: 2008-12-11 23:08
gpolo agree to remove it. If no one is opposed for this change, can someone apply the patch?
Author: Antoine Pitrou (pitrou) *
Date: 2008-12-31 16:24
"tkinter.mainloop" seems used in a bunch of places according to Google Code. Am I missing something?
http://www.google.com/codesearch?hl=fr&lr=&q=%22tkinter.mainloop%22&sbtn=Rechercher
Author: Guilherme Polo (gpolo) *
Date: 2008-12-31 16:27
On Wed, Dec 31, 2008 at 2:24 PM, Antoine Pitrou <report@bugs.python.org> wrote:
Antoine Pitrou <pitrou@free.fr> added the comment:
"tkinter.mainloop" seems used in a bunch of places according to Google Code. Am I missing something?
Yes, those are not _tkinter.mainlooop, they are Tkinter.mainloop.
http://www.google.com/codesearch?hl=fr&lr=&q=%22tkinter.mainloop%22&sbtn=Rechercher
Author: Guilherme Polo (gpolo) *
Date: 2008-12-31 16:30
On Wed, Dec 31, 2008 at 2:27 PM, Guilherme Polo <report@bugs.python.org> wrote:
Guilherme Polo <ggpolo@gmail.com> added the comment:
On Wed, Dec 31, 2008 at 2:24 PM, Antoine Pitrou <report@bugs.python.org> wrote:
Antoine Pitrou <pitrou@free.fr> added the comment:
"tkinter.mainloop" seems used in a bunch of places according to Google Code. Am I missing something?
Yes, those are not _tkinter.mainlooop, they are Tkinter.mainloop.
Or were you referring to my other comment about removing it from Tkinter.py too ?
Author: Antoine Pitrou (pitrou) *
Date: 2008-12-31 16:30
Well, well, sorry for the noise!
Author: Martin v. Löwis (loewis) *
Date: 2009-01-03 07:32
The patch is incorrect. Tkapp_Mainloop is supposed to work both as a module function and a method, and it tests for self to find out which case it is. Now, this test is apparently broken in 3.x, but that could be fixed, instead of ripping the module function out. Or, if that function is removed, the code to support it should also be removed.
The same issue probably also affects other functions that double both as instance methods and module functions.
Author: STINNER Victor (vstinner) *
Date: 2009-01-03 14:00
Tkapp_Mainloop is supposed to work both as a module function and a method, and it tests for self to find out which case it is. Now, this test is apparently broken in 3.x, ...
Ok. In Python 2.x, selfptr is NULL whereas selfptr is a pointer to the module in Python 3.x. New attached patch uses PyModule_Check() to check if selfptr is the module or an object.
if that function is removed, the code to support it should also be removed.
Which code? I don't see which code uses _tkinter.mainloop. I saw code that calls the method mainloop() of a Tkapp object, like tkinter.mainloop() does. Or am I wrong?
Author: Martin v. Löwis (loewis) *
Date: 2009-01-03 14:10
Ok. In Python 2.x, selfptr is NULL whereas selfptr is a pointer to the module in Python 3.x. New attached patch uses PyModule_Check() to check if selfptr is the module or an object.
The patch looks right in principle. Please make sure not to include printf calls in it.
Please also consider all similar cases, such as Tkapp_CreateFileHandler.
if that function is removed, the code to support it should also be removed.
Which code?
The code inside the function: all tests whether self is NULL. If mainloop would stop being a module function, this code also should go.
Author: Guilherme Polo (gpolo) *
Date: 2009-01-03 14:27
Victor, you seem to be confusing "code that supports it" with "functions that use it". There are some conditional code inside Tkapp_MainLoop that depends on self being available, that is what Martin meant by code that supports it, and since it would no longer be a module function that code would no longer be needed.
Now, I'm much more in favour to remove it from moduleMethods than from adjusting it to work in py3k. My reasons for the moment are:
- To me, it makes much more sense to call a mainloop function from a Tcl interpreter object than from a module;
- There is a member named dispatching in TkappObject, so I believe when this tcl/tk bridge was created -- or at least when this member was added -- it had the intention to allow multiple mainloops at some time (or is it really only intended to be used when trying to grab the thread that created the tcl interpreter ?);
- It reduces code :)
Although the reason #2 won't just work after removing mainloop from the module functions, it also doesn't help keeping it there.
Author: Martin v. Löwis (loewis) *
Date: 2009-01-03 15:00
Now, I'm much more in favour to remove it from moduleMethods than from adjusting it to work in py3k.
Would you apply the same reasoning to the other Tkapp methods available on _tkinter?
IIRC, these functions were there first; the Tkapp object was added later. The module functions are kept for compatibility (which we were free to break in 3.0).
I'm not opposed to removing these module functions now (not even for 3.0.1), but...
- To me, it makes much more sense to call a mainloop function from a Tcl interpreter object than from a module;
To me, it makes perfect sense. Windowing events are a global, and not specific to a Tcl interpreter - before receiving it, you cannot know (in general) what interpreter it is for. Indeed, Tcl's Tcl_DoOneEvent doesn't take a Tcl_Interp* parameter.
- There is a member named dispatching in TkappObject, so I believe when this tcl/tk bridge was created -- or at least when this member was added -- it had the intention to allow multiple mainloops at some time (or is it really only intended to be used when trying to grab the thread that created the tcl interpreter ?);
This entire machinery got added when Tcl started supporting threads in a way fundamentally different from Python's support for threads. Tcl is inherently single-threaded, no GIL. To support threads, you create a new interpreter for each new thread, and the different interpreters are completely independent from each other. So when a Tkinter application passes a Tcl command from one thread to another, this would crash or otherwise not work.
In the old days, Python's threading could be used with Tcl just fine; any thread could make Tk calls (on Unix, at least). When Tcl threading was added, I tried to preserve this mode, by making one thread the Tcl thread. Any other Python thread can't make direct Tcl calls (since those would get to a different interpreter), but instead an RPC system based on Tcl's thread synchronization was added. As this RPC depends on the Tcl mainloop, the "dispatching" member tells the caller if the Tcl thread is up.
So, no: the "dispatching" member is there for continued support of a single mainloop, not for multiple mainloops.
- It reduces code :)
That is a good reason. We would still need a complete patch.
Author: STINNER Victor (vstinner) *
Date: 2009-01-03 15:27
Victor, you seem to be confusing "code that supports it" with "functions that use it"
Oh ok, I didn't understood what "code that supports it" mean.
There are some conditional code inside Tkapp_MainLoop that depends on self being available, that is what Martin meant by code that supports it, and since it would no longer be a module function that code would no longer be needed.
Yes, right. I also prefer smaller code base with same functions ;-)
Author: Guilherme Polo (gpolo) *
Date: 2009-01-03 15:36
Martin v. Löwis <martin@v.loewis.de> added the comment:
Now, I'm much more in favour to remove it from moduleMethods than from adjusting it to work in py3k.
Would you apply the same reasoning to the other Tkapp methods available on _tkinter?
As I see all those functions that are calling into Tcl should be moved from the module functions. Some parts of the code inside these functions are conflicting with each other, while one will try to allow multiple threads, the other part will just disallow it. And since to follow the Tcl appartment model we need to access some of the members in TkappObject it just makes me want to remove those functions even more.
IIRC, these functions were there first; the Tkapp object was added later. The module functions are kept for compatibility (which we were free to break in 3.0).
I'm not opposed to removing these module functions now (not even for 3.0.1), but...
- To me, it makes much more sense to call a mainloop function from a Tcl interpreter object than from a module;
To me, it makes perfect sense. Windowing events are a global, and not specific to a Tcl interpreter - before receiving it, you cannot know (in general) what interpreter it is for. Indeed, Tcl's Tcl_DoOneEvent doesn't take a Tcl_Interp* parameter.
That is way to see it. But what I was taking into account was something like the proper update of the dispatching member for different interpreters, this would mean dispatching could be used to stop a running mainloop by setting it to 0.
- There is a member named dispatching in TkappObject, so I believe when this tcl/tk bridge was created -- or at least when this member was added -- it had the intention to allow multiple mainloops at some time (or is it really only intended to be used when trying to grab the thread that created the tcl interpreter ?);
This entire machinery got added when Tcl started supporting threads in a way fundamentally different from Python's support for threads. Tcl is inherently single-threaded, no GIL. To support threads, you create a new interpreter for each new thread, and the different interpreters are completely independent from each other. So when a Tkinter application passes a Tcl command from one thread to another, this would crash or otherwise not work.
That is exactly one of the reasons to move mainloop and others from the moduleMethods. It is hard for me to accept the two distinct behaviours present in _tkinter, which are based from where you call the function.
In the old days, Python's threading could be used with Tcl just fine; any thread could make Tk calls (on Unix, at least). When Tcl threading was added, I tried to preserve this mode, by making one thread the Tcl thread. Any other Python thread can't make direct Tcl calls (since those would get to a different interpreter), but instead an RPC system based on Tcl's thread synchronization was added. As this RPC depends on the Tcl mainloop, the "dispatching" member tells the caller if the Tcl thread is up.
But I don't see a RPC being used there, I just see some polling.
So, no: the "dispatching" member is there for continued support of a single mainloop, not for multiple mainloops.
- It reduces code :)
That is a good reason. We would still need a complete patch.
Author: Martin v. Löwis (loewis) *
Date: 2009-01-03 15:48
But I don't see a RPC being used there, I just see some polling.
Consider Tkapp_Call (e.g.). If this is invoked in the Tk interpreter thread, then there is a direct call to Tcl_EvalObjv/Tkapp_CallResult.
If the call is made from a different thread, then a Tkapp_CallEvent is allocated, filled with the parameters, and Tkapp_ThreadSend is invoked. This puts the event into the thread queue of the receiving thread, and waits for a condition.
In the interpreter thread, Tkapp_CallProc is invoked, which extracts the arguments from the event, invokes Tcl_EvalObj/Tkapp_CallResult, and notifies the condition.
Author: Guilherme Polo (gpolo) *
Date: 2009-01-03 15:57
Martin v. Löwis <martin@v.loewis.de> added the comment:
But I don't see a RPC being used there, I just see some polling.
Consider Tkapp_Call (e.g.). If this is invoked in the Tk interpreter thread, then there is a direct call to Tcl_EvalObjv/Tkapp_CallResult.
If the call is made from a different thread, then a Tkapp_CallEvent is allocated, filled with the parameters, and Tkapp_ThreadSend is invoked. This puts the event into the thread queue of the receiving thread, and waits for a condition.
In the interpreter thread, Tkapp_CallProc is invoked, which extracts the arguments from the event, invokes Tcl_EvalObj/Tkapp_CallResult, and notifies the condition.
This is all true but the dispatching isn't used there actually. dispatching is being used in a polling manner to try to catch the thread running the tcl interpreter which someone tried to call into, the code then proceeds to do what you described.
Author: Martin v. Löwis (loewis) *
Date: 2009-01-03 16:04
This is all true but the dispatching isn't used there actually. dispatching is being used in a polling manner to try to catch the thread running the tcl interpreter which someone tried to call into, the code then proceeds to do what you described.
Right. If the main thread doesn't actually invoke mainloop(), the Tcl events don't get dispatched, and the RPC system breaks down, effectively leading to a deadlock. To prevent application breakage during startup, a grace period is added in case applications create threads before starting the mainloop.
Author: Guilherme Polo (gpolo) *
Date: 2009-01-03 20:19
Here is a patch, two actually. The next one deprecates the functions in trunk
Author: Martin v. Löwis (loewis) *
Date: 2009-01-03 20:57
These look fine. I think further changes are necessary: tkinter/init.py tries to load createfilehandler/deletefilehandler; this becomes redundant. Also, why did you leave DoOneEvent and Quit as-is - don't they fall into the same category?
One minor nit: I personally dislike function pointer casts, because they can easily go wrong. I rather prefer if the C compiler tells me when I mess up with function pointer types. Hence _tkinter uses selfptr/self all the time.
Author: Guilherme Polo (gpolo) *
Date: 2009-01-03 21:24
These look fine. I think further changes are necessary: tkinter/init.py tries to load createfilehandler/deletefilehandler; this becomes redundant.
Indeed, I forgot to look into the Python code.
I also see as redundant the checks for _tkinter._flatten and _tkinter._cnfmerge, the former because if we are still on Tkinter.py (or init.py in py3k) then _tkinter exists and _flatten is there too, the latter because there is no _cnfmerge in _tkinter.
Also, why did you leave DoOneEvent and Quit as-is - don't they fall into the same category?
Ah, it is nice you ask this. I was indeed going to move then, but I noticed strange things with DoOneEvent which made me only remove those functions that were already checking the thread id of the caller and the interp creator. But rethinking about this, it seems to make more sense to do the move now and fix the "strange things" in other patch. The problems I noticed may be a bit off-topic for this specific issue, but I would hate to just say I had problems with a function and don't say which errors I got, and how I got, so I have to write. So, the problem started when I noticed DoOneEvent doesn't check for python errors after Tcl_DoOneEvent returns, making me try to get one of those SystemErrors "NULL result without error" -- I failed to get it, but I got other unexpected results:
I get an empty string with the text below, but I was expecting some nameerror message:
import _tkinter
def mybgerror(errmsg): print errmsg
called = [] def bad(): called.append(True) raise InvalidThing
x = _tkinter.create() x.createcommand("bad", bad) x.createcommand("bgerror", mybgerror) x.call("after", 1, "bad")
while not called: _tkinter.dooneevent() _tkinter.dooneevent()
I ended up finding another problem (or problems, I forgot to annotate them), like with call:
import _tkinter
def bad(): raise InvalidThing
x = _tkinter.create() x.createcommand("bad", bad) x.call("bad")
Results in:
Traceback (most recent call last): File "my_badtests/test1.py", line 7, in x.call("bad") _tkinter.TclError
Meaning call is overwriting a python error by a tcl error, but since there was no error in the tcl interpreter the error message was empty (too many errors in a message).
But these all would deserve another(s) issues, so I will be moving "quit" and "dooneevent" from there too.
One minor nit: I personally dislike function pointer casts, because they can easily go wrong. I rather prefer if the C compiler tells me when I mess up with function pointer types. Hence _tkinter uses selfptr/self all the time.
Fine.
Author: Guilherme Polo (gpolo) *
Date: 2009-01-03 21:26
I get an empty string with the text below, but I was expecting some
s/text/test
Author: Martin v. Löwis (loewis) *
Date: 2009-01-03 21:38
But these all would deserve another(s) issues, so I will be moving "quit" and "dooneevent" from there too.
I haven't tried reproducing these problems, but this all sounds plausible.
So go ahead and check in all the changes for this issue.
Don't forget Misc/NEWS entries, and don't forget to svnmerge block the 2.7 change from being merged into 3.k. You should then also merge the 3k change into the 3.0 branch. I don't think we have to merge the warnings into 2.6 (but could if we wanted to). Leave the various revision numbers in this report when done.
If you rather prefer me to check it in, please let me know.
Author: Guilherme Polo (gpolo) *
Date: 2009-01-03 22:07
Thanks Martin,
trunk: r68231 (blocked on py3k) py3k: r68237 release30-maint: r68239
Author: Guilherme Polo (gpolo) *
Date: 2009-01-03 22:18
Uh, I forgot about the code removal in init in the first commits.
Author: STINNER Victor (vstinner) *
Date: 2009-01-04 17:39
gpolo: Nice patches, good job and thanks ;-)
Author: Martin Panter (martin.panter) *
Date: 2014-08-06 10:21
See Issue 22155 for fixing the createfilehandler FAQ documentation
History
Date
User
Action
Args
2022-04-11 14:56:38
admin
set
github: 47888
2014-08-06 10:21:06
martin.panter
set
nosy: + martin.panter
messages: +
2009-01-04 17:39:40
vstinner
set
messages: +
2009-01-03 22🔞35
gpolo
set
messages: +
2009-01-03 22:07:49
gpolo
set
status: open -> closed
messages: +
versions: + Python 3.1, Python 2.7, - Python 2.6
resolution: fixed
title: tkinter.mainloop() is meaningless and crash: remove it -> Remove module level functions in _tkinter that depend on TkappObject
2009-01-03 21:38:30
loewis
set
messages: +
2009-01-03 21:26:46
gpolo
set
messages: +
2009-01-03 21:24:20
gpolo
set
messages: +
2009-01-03 20:57:10
loewis
set
assignee: gpolo
messages: +
2009-01-03 20:20:12
gpolo
set
files: + deprecated_funcs.diff
2009-01-03 20:19:50
gpolo
set
files: + issue3638.diff
messages: +
2009-01-03 16:04:30
loewis
set
messages: +
2009-01-03 15:57:51
gpolo
set
messages: +
2009-01-03 15:48:11
loewis
set
messages: +
2009-01-03 15:36:13
gpolo
set
messages: +
2009-01-03 15:27:38
vstinner
set
messages: +
2009-01-03 15:00:26
loewis
set
messages: +
2009-01-03 14:27:09
gpolo
set
messages: +
2009-01-03 14:10:01
loewis
set
messages: +
2009-01-03 14:00:14
vstinner
set
files: + _tkinter_mainloop.patch
messages: +
2009-01-03 13:57:22
vstinner
set
files: - tkinter_remove_mainloop.patch
2009-01-03 07:32:40
loewis
set
nosy: + loewis
messages: +
2008-12-31 16:30:56
pitrou
set
messages: +
2008-12-31 16:30:11
gpolo
set
messages: +
2008-12-31 16:27:31
gpolo
set
messages: +
2008-12-31 16:24:18
pitrou
set
nosy: + pitrou
messages: +
2008-12-11 23:08:52
vstinner
set
messages: +
2008-09-13 14:22:24
gpolo
set
title: tkinter.mainloop() is meanling less and crash: remove it -> tkinter.mainloop() is meaningless and crash: remove it
2008-09-13 14:19:58
gpolo
set
nosy: + gpolo
messages: +
2008-08-26 00:52:57
ajaksu2
set
nosy: + ajaksu2
2008-08-21 22:00:13
vstinner
create