Skip to content

Conversation

mdehoon
Copy link

@mdehoon mdehoon commented Sep 20, 2025

This pull request fixes the bug described in issue #139145 by moving the call to Tcl_DeleteFileHandler from EventHook to MyFileProc.
The reasoning behind this is:

  • after the first call to MyFileProc, stdin_ready is already set to 1, so there is no reason to keep watching stdin and call MyFileProc again, so we may as well call Tcl_DeleteFileHandler` immediately;
  • more importantly, if EventHook started another event loop (e.g. the tight event loop started by wait_variable; see issue tkinter wait_variable can cause the event loop to spin #139145), then calling Tcl_DeleteFileHandler in MyFileProc (instead of waiting for control to return to EventHook) will ensure that the tight event loop won't be spinning its wheels by calling MyFileProc continuously.

@bedevere-app
Copy link

bedevere-app bot commented Sep 20, 2025

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 skip news label instead.

Copy link
Member

@StanFromIreland StanFromIreland left a 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.

@bedevere-app
Copy link

bedevere-app bot commented Sep 20, 2025

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 skip news label instead.

Copy link
Contributor

@chrstphrchvz chrstphrchvz left a 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Tcl_CreateFileHandler(tfile, TCL_READABLE, MyFileProc, &tfile);
Tcl_CreateFileHandler(tfile, TCL_READABLE, MyFileProc, (void *)(Py_intptr_t)tfile);

Comment on lines +3359 to +3361
int *tfile = clientData;
stdin_ready = 1;
Tcl_DeleteFileHandler(*tfile);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
int *tfile = clientData;
stdin_ready = 1;
Tcl_DeleteFileHandler(*tfile);
int tfile = (int)(Py_intptr_t)clientData;
stdin_ready = 1;
Tcl_DeleteFileHandler(tfile);

Copy link
Author

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,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants