classification
Title: Interrupts are lost during readline PyOS_InputHook processing (reopening)
Type: behavior Stage: patch review
Components: Extension Modules Versions: Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Michiel.de.Hoon, martin.panter, mdehoon, r.david.murray, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2015-01-14 02:13 by Michiel.de.Hoon, last changed 2015-10-27 12:17 by martin.panter.

Files
File name Uploaded Description Edit
issue23237.patch mdehoon, 2015-01-24 04:42 Patch for issue 23237 review
issue23237.version2.patch mdehoon, 2015-06-03 14:17 Patch for issue 23237, version 2 review
_tkinter.c.patch mdehoon, 2015-06-04 12:32 Patch for _tkinter.c review
hook-interrupt.3.6.patch martin.panter, 2015-06-05 07:28 For Python 3 review
Messages (14)
msg233997 - (view) Author: Michiel de Hoon (Michiel.de.Hoon) Date: 2015-01-14 02:13
This bug was previously reported in
http://bugs.python.org/issue3180
but was closed after seven years for being out of date.
Still, the bug remains: Interrupts are lost during readline PyOS_InputHook processing.

To reproduce the bug, try

>>> from Tkinter import *
>>> Tk()

You will notice that Ctrl-C now does not generate a KeybordInterrupt.

This bug appears in all Python versions I tested (Python 2.7, 3.3, 3.4) and, judging from the source code, is still present in the current Python source code in Mercurial. The bug appears both on Mac and on Linux; I do not have access to Windows.

The suggested patch at http://bugs.python.org/issue3180 makes multiple changes to the behavior of PyOS_InputHook. In particular, it allows for waiting for a file descriptor other than stdin. I think that this is unrelated to the current bug, so it should not be included in the patch, in particular since that would change the API.
msg234000 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-01-14 02:54
Updating versions to reflect where it might get fixed (which is what we use the versions field for).
msg234592 - (view) Author: Michiel de Hoon (mdehoon) * Date: 2015-01-24 04:42
I am attaching a patch for this bug for Python 2.7.
msg244021 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-05-25 09:04
I left a couple comments on Reitveld. The main question is: is there any reason why we can’t poll PyErr_CheckSignals() directly the tkinter EventHook loop, rather than juggling a new SIGINT handler then reraising it? That way we might trigger other Python signal handlers properly as well.
msg244449 - (view) Author: Michiel de Hoon (mdehoon) * Date: 2015-05-30 06:35
Vadmium, thank you for carefully looking at this patch.

Polling PyErr_CheckSignals() directly in the tkinter EventHook loop works in the #if defined(WITH_THREAD) || defined(MS_WINDOWS) block, as there Tcl_DoOneEvent(TCL_DONT_WAIT) exits immediately.

However in the #else block Tcl_DoOneEvent(0) is called. Here, Tcl_DoOneEvent does not return until there is some event. So if a user presses Ctrl-C, then trip_signal in Modules/signalmodule.c does get called, but Tcl_DoOneEvent won't know about it and will continue to wait for an event. The KeyboardInterrupt will then happen only after the user presses enter or moves the mouse over the Tk window.

That said, now I realize that my patch doesn't solve that problem either. So I need to go back and think of a proper way to exit Tcl_DoOneEvent in case of an interrupt. I think that that is a sufficiently complex problem to warrant its own patch. For the current patch, I suggest we consider the changes to Modules/readline.c and Parser/myreadline.c only, and leave out any changes to Modules/_tkinter.c.

In response to your comments on Modules/readline.c:

>Modules/readline.c:998:
> if(PyOS_InputHook) has_input = PyOS_InputHook();
> This contradicts the documentation, which says
> PyIO_InputHook()’s return value is ignored

Yes you are correct; the documentation will have to be updated if we start using the return value of PyOS_InputHook().

My proposal to use return value == -1 and errno == EINTR to indicate interrupts is based on the convention for select(). In extension modules such as Tkinter, PyOS_InputHook points to a function that returns if input is available on stdin, so effectively it's like a simplified version of select() that only looks at stdin.

In Tkinter, pygtk, and PyQT, PyOS_InputHook returns 0; in matplotlib's MacOSX backend it returns +1. So I think it is safe to start using -1 for interrupts.
But yes, the documentation will have to be updated.

>Modules/readline.c:1065:
> old_timeout = rl_set_keyboard_input_timeout(0);
> Won’t this poll PyOS_InputHook over and over, wasting CPU time?
No it won't. In well-designed code (including Tkinter, pygtk, PyQT, and matplotlib's MacOSX backend) PyOS_InputHook does not return until there is some activity on stdin, and therefore PyOS_InputHook does not get called repetitively anyway. So the timeout is only relevant as the delay until readline() calls PyOS_InputHook(). Since there is no point to this delay, it's better to set it to zero.
msg244751 - (view) Author: Michiel de Hoon (mdehoon) * Date: 2015-06-03 14:17
I am uploading an updated version of the patch.
I'd be happy to submit a patch to the documentation also, but wasn't able to find it on Mercurial. Can somebody please point me to the right repository for the documentation?
msg244806 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-06-04 07:17
Hi Michiel, if you are looking for the source of <https://docs.python.org/dev/c-api/veryhigh.html#c.PyOS_InputHook>, that corresponds to Doc/c-api/veryhigh.rst in the repository.

This bug would be fairly easy to solve for “tkinter” if we could drop the Tcl_DoOneEvent(0) half of the code, and always use the TCL_DONT_WAIT polling instead. Then the event hook could return if there were no immediate events, and it would be up to readline() or whatever to check for interrupts or input, or loop back to PyOS_InputHook() after a small delay.

Otherwise, I think we need to come up with a way to inject an event into a TCL event queue when there is a signal that needs handling. If this were possible, it would also fix the SIGINT responsiveness from Tk.mainloop() etc. Or it would be nice if there was a Tcl_DoOneEvent(WAIT_UNTIL_INTERRUPTED_BY_A_SIGNAL) option.

A possible test case for this bug would reopen stdin to something harmless, set a SIGALRM handler, and call input(). The signal should eventually cause input() to raise an exception.
msg244810 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-06-04 11:01
I take back my claim about it being easy to fix tkinter with the existing API. My ideas break down because PyOS_InputHook() is used differently in Parser/myreadline.c to Modules/readline.c. So think if we want proper compatibility, a separate hook API might be needed.
msg244815 - (view) Author: Michiel de Hoon (mdehoon) * Date: 2015-06-04 12:32
Patch for Modules/_tkinter.c.
To be used together with issue23237.version2.patch.
msg244816 - (view) Author: Michiel de Hoon (mdehoon) * Date: 2015-06-04 12:33
I am not sure if I follow. With the patch to _tkinter.c, interrupts seem to work correctly with all three variations of PyOS_InputHook.
msg244857 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-06-05 07:28
My previous comments were about trying to maintain backwards compatibility. E.g. with the current patches, an old PyOS_InputHook() that happened to always return a negative value would mean the input never gets checked by the new Python code. I dunno what the policy on changing the C API though. Perhaps people are happy changing this in 3.6.

Also, the signal handling isn’t perfect in tkinter (e.g. it doesn’t account for non-SIGINT handlers), but perhaps practicality and simplicity beat purity here and we can say it is good enough :)

I am posting hook-interrupt.3.6.patch, which combines the readline and tkinter patches and fixes them up for Python 3. Also added some notes to the PyOS_InputHook documentation.
msg244870 - (view) Author: Michiel de Hoon (mdehoon) * Date: 2015-06-05 15:30
I tried the patch hook-interrupt.3.6.patch ; as far as I can tell it is working fine.
msg253424 - (view) Author: Michiel de Hoon (mdehoon) * Date: 2015-10-25 07:52
Anything I can do to help this patch move forward?
msg253531 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-10-27 12:16
From memory my biggest worry was changing the API (meaning of return value). I don’t know what the policy is, but it might need a new separate API. You might find more input from experts on the python-dev list or something.
History
Date User Action Args
2015-10-27 12:17:00martin.pantersetmessages: + msg253531
2015-10-25 07:52:19mdehoonsetmessages: + msg253424
2015-06-05 15:30:57mdehoonsetmessages: + msg244870
2015-06-05 07:28:50martin.pantersetfiles: + hook-interrupt.3.6.patch

messages: + msg244857
versions: + Python 3.6, - Python 2.7, Python 3.4, Python 3.5
2015-06-04 12:33:14mdehoonsetmessages: + msg244816
2015-06-04 12:32:26mdehoonsetfiles: + _tkinter.c.patch

messages: + msg244815
2015-06-04 11:01:54martin.pantersetmessages: + msg244810
2015-06-04 07:17:28martin.pantersetmessages: + msg244806
2015-06-03 14:17:17mdehoonsetfiles: + issue23237.version2.patch

messages: + msg244751
2015-05-30 06:35:07mdehoonsetmessages: + msg244449
2015-05-25 09:04:39martin.pantersetmessages: + msg244021
2015-05-23 11:45:09martin.pantersetnosy: + martin.panter
2015-02-17 08:29:22serhiy.storchakasetnosy: + serhiy.storchaka

stage: patch review
2015-02-17 04:26:02mdehoonsetnosy: + vstinner
2015-01-24 04:42:30mdehoonsetfiles: + issue23237.patch

nosy: + mdehoon
messages: + msg234592

keywords: + patch
2015-01-14 02:54:51r.david.murraysetnosy: + r.david.murray

messages: + msg234000
versions: - Python 3.2, Python 3.3, Python 3.6
2015-01-14 02:13:03Michiel.de.Hooncreate