-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-139145: fix spinning event loop in tkinter #139180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a blurb and a test.
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just a past contributor, not a Python member.
I think the issue description and proposed change to _tkinter.c make sense. I have not reviewed the unit test.
However, I notice tfile being passed by address to Tcl_CreateFileHandler(); is there a particular reason for doing so? To avoid any potential stack-use-after-scope errors, such as if the while loop in EventHook() were to end because of errorInCmd rather than stdin_ready, I think it is better to pass tfile by value, even though that requires casting to avoid compiler warnings/errors (unless CPython already has some PTR2INT()/INT2PTR()-style macros I am unaware of).
| #ifndef MS_WINDOWS | ||
| tfile = fileno(stdin); | ||
| Tcl_CreateFileHandler(tfile, TCL_READABLE, MyFileProc, NULL); | ||
| Tcl_CreateFileHandler(tfile, TCL_READABLE, MyFileProc, &tfile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Tcl_CreateFileHandler(tfile, TCL_READABLE, MyFileProc, &tfile); | |
| Tcl_CreateFileHandler(tfile, TCL_READABLE, MyFileProc, (void *)(Py_intptr_t)tfile); |
| int *tfile = clientData; | ||
| stdin_ready = 1; | ||
| Tcl_DeleteFileHandler(*tfile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| int *tfile = clientData; | |
| stdin_ready = 1; | |
| Tcl_DeleteFileHandler(*tfile); | |
| int tfile = (int)(Py_intptr_t)clientData; | |
| stdin_ready = 1; | |
| Tcl_DeleteFileHandler(tfile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this. This is an important point.
The while-loop in EventHook can only be exited if either stdin_ready is 1 or errorInCmd is 1.
stdin_ready is set to 1 only in MyFileProc, which also calls Tcl_DeleteFileHandler. As the file handler is deleted, there won't be a dangling pointer.
But I was missing a call to Tcl_DeleteFileHandler for the case in which errorInCmd is 1, and MyFileProc was never called. Then then file handler will stay alive and will keep a dangling pointer to tfile after EventHook exits.
I fixed this by adding a call to Tcl_DeleteFileHandler to the if (errorInCmd) {... to ensure that the file handler is deleted before EventHook exits, and no pointer to tfile remains,
|
Let's keep this PR open for now. In particular, it would be better to get PR #140147 through first, as it would let us run the tests also on Windows. |
This pull request fixes the bug described in issue #139145 by moving the call to
Tcl_DeleteFileHandlerfromEventHooktoMyFileProc.The reasoning behind this is:
MyFileProc,stdin_readyis already set to 1, so there is no reason to keep watching stdin and callMyFileProcagain, so we may as well call Tcl_DeleteFileHandler` immediately;EventHookstarted another event loop (e.g. the tight event loop started bywait_variable; see issue tkinter wait_variable can cause the event loop to spin #139145), then callingTcl_DeleteFileHandlerinMyFileProc(instead of waiting for control to return toEventHook) will ensure that the tight event loop won't be spinning its wheels by callingMyFileProccontinuously.