bpo-31546: fix input hook integration on windows

[tacaswell]

This change was suggested by Steve Dower. I do not have a windows development set up to test this.

If this could make it in for the 3.7 and the next 3.6 patch release that would be great! This bug breaks interactive matplotlib windows for windows users in the standard python prompt.

attn @zooba

https://bugs.python.org/issue31546

[zooba]

My only question is whether it's okay to call PyOS_InputHook multiple times if we get a partial read below? Or if we should only call it once per input() call (in which case, put it outside the while loop)

[tacaswell]

I think it needs to be run in the case of a partial read (which I am taking to mean the user has not hit 'return' yet) as while we wait for the user to finish typing PyOS_InputHook should be called to let the GUI event loops spin and make the windows seem responsive.

On linux these hooks tend to run forever but exit them selves as soon as there is something to be read on stdin. I have a vague memory that the hooks work differently on windows where they run for a fixed about of time and expect to be called repeatedly. If that is the case I am worried this may not actually fix the bug as it looks like ReadConsoleW blocks until it has something to read.

[tacaswell]

On a bit more digging, I think my concerns are miss-placed (see the tk EventHook below), but I am pretty sure that this needs to run every time through this loop.

cpython/Modules/_tkinter.c

Lines 3301 to 3348 in 4925727

	static int
	EventHook(void)
	{
	#ifndef MS_WINDOWS
	int tfile;
	#endif
	PyEval_RestoreThread(event_tstate);
	stdin_ready = 0;
	errorInCmd = 0;
	#ifndef MS_WINDOWS
	tfile = fileno(stdin);
	Tcl_CreateFileHandler(tfile, TCL_READABLE, MyFileProc, NULL);
	#endif
	while (!errorInCmd && !stdin_ready) {
	int result;
	#ifdef MS_WINDOWS
	if (_kbhit()) {
	stdin_ready = 1;
	break;
	}
	#endif
	Py_BEGIN_ALLOW_THREADS
	if(tcl_lock)PyThread_acquire_lock(tcl_lock, 1);
	tcl_tstate = event_tstate;
	result = Tcl_DoOneEvent(TCL_DONT_WAIT);
	tcl_tstate = NULL;
	if(tcl_lock)PyThread_release_lock(tcl_lock);
	if (result == 0)
	Sleep(Tkinter_busywaitinterval);
	Py_END_ALLOW_THREADS
	if (result < 0)
	break;
	}
	#ifndef MS_WINDOWS
	Tcl_DeleteFileHandler(tfile);
	#endif
	if (errorInCmd) {
	errorInCmd = 0;
	PyErr_Restore(excInCmd, valInCmd, trbInCmd);
	excInCmd = valInCmd = trbInCmd = NULL;
	PyErr_Print();
	}
	PyEval_SaveThread();
	return 0;
	}

[zooba]

It certainly fixes the referenced issues, so I approve.

[eryksun]

@zooba, this change reproduces the legacy implementation, which loops in PyOS_StdioReadline until LF is read and calls PyOS_InputHook at the start of my_fgets.

Note that with the default console mode and NULL pInputControl value, ReadConsoleW blocks until a CR is typed or written to the input buffer. In most cases this function reads the whole line in one pass since the initial wbuf_local is 16K characters. The chance of a single line being longer is negligible.

For a more responsive interaction, we would need a low-level implementation that calls ReadConsoleInput, like what PowerShell uses. This entails developing our own command-line editing interface, i.e. we lose everything that's provided by the console's built-in cooked read (e.g. editing keys such as left/right arrows, backspace, home, end, and escape; input history via up/down arrows and F7; and input aliases). This is actually already implemented by pyreadline using ctypes. An ambitious dev could port this to a C extension that provides built-in readline support in Windows.

[eryksun]

A less ambitious addition could implement just command completion using the pInputControl parameter of ReadConsoleW. This is how CMD's tab completion works. It's still a cooked read, so it doesn't do much for responsiveness in terms of PyOS_InputHook. At best the hook can be called in each pass through the tab-completion loop. However, the alternative with ReadConsoleInput is a major undertaking -- and probably too much to take on. It's not simply providing an interface for a readline library; it's developing the entire library from scratch.

Anyway, here's an overview of how the CMD shell implements tab completion for console input.

Before the input loop, it saves the initial cursor position from GetConsoleScreenBufferInfo in order to support rewriting the line while cycling through possible completions. The dwCtrlWakeupMask field of the CONSOLE_READCONSOLE_CONTROL record is set to return when tab is pressed (i.e. 0x200). After the call returns, the dwControlKeyState field is checked for the shift key (0x10), in which case it cycles in reverse.

In the input loop it calls ReadConsoleW and then looks for CR and tab in the buffer. CR terminates the read. Otherwise if tab is found, it completes the command in place in the buffer, which for CMD is simply filename completion. Next it updates the screen with the command. For this, it resets the cursor position via SetConsoleCursorPosition, overwrites the previous command via FillConsoleOutputCharacterW, and writes the new completed command via WriteConsoleW. Finally, for the next iteration it makes the console skip what's already read/completed in the buffer by setting the nInitialChars field, which I think also integrates with the console history buffer, so it doesn't end up with partial lines.

[eryksun]

To quantify the difference here, with the Unix readline module, the input hook is called every 0.1 seconds, which is implemented using a timeout in readline_until_enter_or_signal. The docs for PyOS_InputHook are short on details, so I suppose this is allowed.

As noted above, Tkinter doesn't depend on the input hook being called repeatedly in order to pump events. In Windows the input hook loops so long as _kbhit either returns false or fails because stdin isn't console input. We can make this misbehave. Run bash under mintty and force Python to run interactive via python -i. Then start Tk via root = tkinter.Tk(). The REPL will hang in tkinter's input hook because stdin is a pipe in this scenario.

[zooba]

Pending NEWS item (click "Details" on the check below for instructions), and PEP 7 requires braces on the if block.

[tacaswell]

Is that NEWS text acceptable?

[methane]

Please use 4-space indent.

[tacaswell]

@eryksun has reasonable concerns about this that are hidden in a folded comment above.

From the discussion I think with this patch the behavior will be:

1. user gets new prompt GUI are responsive
2. user hits any key on keyboard (that goes to console ?), the input hook exits and we block in ReadConsoleW until CR
3. while we are waiting for CR the GUI is non-responsive
4. user hits CR, code runs, and we go back to the top

If for 2 it is any key press that stops the interaction this may still be a problem for Matplotlib (as we have a way for users to set up callbacks on keystrokes in the plot window).

@eryksun suggests using pInputControl, but that seems to only provide a hook to return ReadConsoleW on a few more special keys rather than just CR.

Despite the short comings of this patch I think it improves the situation from current one (no interactivity at all) and the alternative seems to be re-writing readline.

Can I request this get flagged as a priority for 3.7.1 and 3.6.7 ?

Unfortunately I do not have easy access to a windows machine set up for dev work to test any of this on.

[zooba]

Reproducing the legacy behavior seems like a sensible first step, rather than rewriting everything at this point. We still have open issues about replacing the whole readline mess with something that works at the Python level, and if we're going to do surgery here I'd rather do that.

Actually calling the hook allows hook authors to do what they need to do to make their scenario work (including sending CR directly to stdin themselves so that the ReadConsole call completes immediately). So I'm merging this (and backporting to 3.6.7 and 3.7.1). The discussion about making it possible to completely override stdin in both interactive and non-interactive scenarios should go back to the issue tracker.

[miss-islington]

Thanks @tacaswell for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

[bedevere-bot]

GH-7990 is a backport of this pull request to the 3.7 branch.

[bedevere-bot]

GH-7991 is a backport of this pull request to the 3.6 branch.