readline: move pause to fix #5927 #5930
Conversation
|
Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion. The following commiters were not found in the CLA:
You can fix all these things without opening another issue. Please see CONTRIBUTING.md for more information |
|
I've signed the CLA now, will the Jenkins bot automatically realise this? |
|
I'm not sure why those 3 tests are failing as they seem very unrelated, perhaps someone can shed some light on it. |
|
Don't worry about them. |
|
Looks okay to me but is it possible to add a regression test for this? Also, can you reword the commit log a little? The first line should explain what changed and why. Don't reference the GH issue in it, do that by wrapping up the commit log with a |
|
@bnoordhuis I have fixed the commit and added a regression test. |
| origSetRawMode.apply(this, arguments); | ||
| } | ||
|
|
||
| rl.close() |
|
@danielchatfield Thanks. The test is not quite what I had in mind but I'm willing to believe it's hard to test for when the test doesn't run in a real terminal (when you run I'll have to ask you to redo the commit log again. It's important that it explains both what changed and why, i.e. what bug does it fix. @piscisaureus Can you take a look at this? |
|
Fixed the commit again to add the missing semi colon. |
Move pause() before _setRawMode() in close() Includes regression test Fixes #5927 pause should be called with RawMode set to true, therefore in close() the pause() should be before the setRawMode(false). This fixes a bug where under windows 8 new instances of readline will not be able to receive input until enter is pressed.
|
@bnoordhuis I tried faking terminal input but couldn't get the test to work without actually using the terminal. I've updated the commit message, I hope it is alright this time. |
|
It's close enough, I'll amend it when I land it. I was looking for something like "readline: fix windows tty stall, pause before setting raw mode" (though that probably doesn't fit in 50 characters.) I'd still like for @piscisaureus to take a look. |
|
@piscisaureus Have you had a chance to take a look yet? |
|
I'm sorry, this issue somehow slipped my attention. This is the forgotten twin fix for f34f1e3. The underlying issue is caused by a minor behavior change from windows 7 to windows 8. Prior to windows 8 libuv could "cancel" a line-buffered read by closing the console handle that the ReadConsole operation was called with. This trick no longer works in windows 8; the ReadConsole operation doesn't return until the user hits enter. @jepickett is working on a libuv patch (joyent/libuv#877) that makes libuv cancel a line-buffered read by simulating an enter keypress instead. However this patch to node is still good since now libuv doesn't have to start and immediately cancel a ReadConsome operation anymore. Testing-wise, this is as good as it gets. Node doesn't offer an API for simulating key presses which is what would be needed to test this more thoroughly. LGTM. |
On windows, libuv will immediately make a `ReadConsole` call (in the thread pool) when a 'flowing' `uv_tty_t` handle is switched to line-buffered mode. That causes an immediate issue for some users, since libuv can't cancel the `ReadConsole` operation on Windows 8 / Server 2012 and up if the program switches back to raw mode later. But even if this will be fixed in libuv at some point, it's better to avoid the overhead of starting work in the thread pool and immediately cancelling it afther that. See also f34f1e3, where the same change is made for the opposite flow, e.g. move `resume()` after `_setRawMode(true)`. Fixes #5927 This is a backport of dfb0461 (see #5930) to the v0.10 branch.
On windows, libuv will immediately make a `ReadConsole` call (in the thread pool) when a 'flowing' `uv_tty_t` handle is switched to line-buffered mode. That causes an immediate issue for some users, since libuv can't cancel the `ReadConsole` operation on Windows 8 / Server 2012 and up if the program switches back to raw mode later. But even if this will be fixed in libuv at some point, it's better to avoid the overhead of starting work in the thread pool and immediately cancelling it afther that. See also f34f1e3, where the same change is made for the opposite flow, e.g. move `resume()` after `_setRawMode(true)`. Fixes #5927 This is a backport of dfb0461 (see #5930) to the v0.8 branch.
Pause should be called before _setRawMode to prevent things going wonky
in windows (see #5927).
(sorry for messing up first pull request)