win,tty: fix read stop in line mode #866

orangemocha · 2016-05-11T22:20:50Z

Closing the handle does not make ReadConsoleW exit reliably on Windows 7 and above. Thus, after switching from line to raw mode, keypresses were held until enter was pressed. This has caused a number of issues in node: nodejs/node#2504 , nodejs/node#2996, nodejs/node#5384 , nodejs/node#5821 , nodejs/node#5940 .

This fix makes ReadConsoleW exit by writing a return keypress to its input buffer, similar to what was already done for raw mode. The second commit removes the duplicate handle that was used to close the console before. The third commit restores the cursor position to where it was before writing the enter, because ReadConsoleW echoes the newline in the screen.

This PR includes a test that switches the console to raw mode and uses WriteConsoleInputW to test reading in raw mode. I do not know of a unix equivalent to WriteConsoleInputW, so the test is for Windows only. In this test, switching to raw mode is done after a 100 ms sleep, to allow uv_tty_line_read_thread to reach ReadConsoleW.

The added User32 library is necessary for MapVirtualKeyW.

Fixes: #852

cc @saghul @piscisaureus

eljefedelrodeodeljefe · 2016-05-11T23:06:57Z

Who would have thought...

saghul · 2016-05-12T06:57:37Z

include/uv-win.h

@@ -483,7 +483,6 @@ RB_HEAD(uv_timer_tree_s, uv_timer_s);
  union {                                                                     \
    struct {                                                                  \
      /* Used for readable TTY handles */                                     \
-      HANDLE read_line_handle;                                                \


I think this will break the ABI because the size of struct rd is changed. Add a comment of it not being used anymore and we'll remove it for v2.

Good call! Will fix.

saghul · 2016-05-12T07:18:35Z

Thanks a lot for working on this Alexis! I left some comments. There is one little important thing which definitely needs fixing: the ABI breakeage. Other than that, I don't have enough experience with low level Windows console handling to make a final judgement call, so I'll trust you and @piscisaureus.

orangemocha · 2016-05-12T13:52:23Z

Thanks for the feedback, @saghul ! I have added 3 commits to address your comments. I can fixup the original commits once the PR has passed review.

saghul · 2016-05-12T17:06:53Z

LGTM if the CI is green: https://ci.nodejs.org/job/libuv-test-commit-windows/48/ But I'd like @piscisaureus to take a look too, if possible.

MylesBorins · 2016-05-12T17:11:44Z

it looks like the libuv CI is down... this is likely related to the updated that came in yesterday. /cc @jbergstroem

saghul · 2016-05-12T17:21:38Z

I just noticed, thanks for taking a look Myles!

saghul · 2016-05-12T17:22:17Z

To save anyone a trip to the console:

Started by upstream project "libuv-test-commit-windows" build number 50
originally caused by:
 Started by user Saúl Ibarra Corretgé
[EnvInject] - Loading node environment variables.
Building remotely on test-azure_msft-win2008r2-x64-2 (win2008r2-1p win2008r2 win2008r2-vs2015) in workspace c:\workspace\libuv-test-commit-windows\nodes\win2008r2
Wiping out workspace first.
Cloning the remote Git repository
Cloning repository https://github.com/$user/$project.git
 > git init c:\workspace\libuv-test-commit-windows\nodes\win2008r2 # timeout=10
Fetching upstream changes from https://github.com/$user/$project.git
 > git --version # timeout=10
 > git -c core.askpass=true fetch --tags --progress https://github.com/$user/$project.git +refs/heads/*:refs/remotes/origin/*
ERROR: Error cloning remote repo 'origin'
hudson.plugins.git.GitException: Command "git -c core.askpass=true fetch --tags --progress https://github.com/$user/$project.git +refs/heads/*:refs/remotes/origin/*" returned status code 128:
stdout: 
stderr: fatal: unable to access 'https://github.com/$user/$project.git/': The requested URL returned error: 400

    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1719)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandWithCredentials(CliGitAPIImpl.java:1463)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.access$300(CliGitAPIImpl.java:63)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$1.execute(CliGitAPIImpl.java:314)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$2.execute(CliGitAPIImpl.java:506)
    at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$1.call(RemoteGitImpl.java:152)
    at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$1.call(RemoteGitImpl.java:145)
    at hudson.remoting.UserRequest.perform(UserRequest.java:120)
    at hudson.remoting.UserRequest.perform(UserRequest.java:48)
    at hudson.remoting.Request$2.run(Request.java:332)
    at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68)
    at java.util.concurrent.FutureTask.run(Unknown Source)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
    at hudson.remoting.Engine$1$1.run(Engine.java:85)
    at java.lang.Thread.run(Unknown Source)
    at ......remote call to test-azure_msft-win2008r2-x64-2(Native Method)
    at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1416)
    at hudson.remoting.UserResponse.retrieve(UserRequest.java:220)
    at hudson.remoting.Channel.call(Channel.java:781)
    at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler.execute(RemoteGitImpl.java:145)
    at sun.reflect.GeneratedMethodAccessor529.invoke(Unknown Source)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler.invoke(RemoteGitImpl.java:131)
    at com.sun.proxy.$Proxy101.execute(Unknown Source)
    at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1057)
    at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1097)
    at hudson.scm.SCM.checkout(SCM.java:485)
    at hudson.model.AbstractProject.checkout(AbstractProject.java:1269)
    at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:607)
    at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
    at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:529)
    at hudson.model.Run.execute(Run.java:1738)
    at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
    at hudson.model.ResourceController.execute(ResourceController.java:98)
    at hudson.model.Executor.run(Executor.java:410)
ERROR: null
Notifying upstream projects of job completion
Finished: FAILURE

MylesBorins · 2016-05-12T22:06:30Z

fingers crossed CI should be working now

ci: https://ci.nodejs.org/view/libuv/job/libuv-test-commit-windows/51/

I will keep this comment updated with the status of getting this working to avoid spamming the thread

saghul · 2016-05-12T22:10:59Z

Thanks Myles! Build is 💚

Closing the handle does not make ReadConsoleW exit reliably on Windows 7 and above. Thus, after switching from line to raw mode, keypresses were held until enter was pressed. This makes ReadConsoleW exit by writing a return keypress to its input buffer, similar to what was already done for raw mode. Fixes: libuv#852

orangemocha · 2016-05-12T23:10:30Z

Great, thanks! I applied the fixups, should be ready for landing.

saghul · 2016-05-13T06:17:11Z

Can you please reword the second commit? Now we are not removing anything :-)

I can work on a release tonight or on monday, but I guess nobody wants to release on a friday...

MylesBorins · 2016-05-13T06:24:45Z

@saghul we usually do the node releases on Tuesdays... so as long as it is out the door Monday I see no reason it could make it into next weeks v6 release

Since we cancel ReadConsole by sending a newline, the duplicate handle is no longer necessary.

saghul · 2016-05-13T13:00:23Z

Thanks Alexis!

@libuv/collaborators can I get another pair of eyes on this?
On May 13, 2016 11:25, "Alexis Campailla" notifications@github.com wrote:

Second commit reworded.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#866 (comment)

piscisaureus · 2016-05-13T21:42:22Z

src/win/tty.c



 /* Null uv_buf_t */
 static const uv_buf_t uv_null_buf_ = { 0, NULL };

+static CRITICAL_SECTION uv__read_console_trap_lock;
+static volatile BOOL uv__read_console_trap = FALSE;


There is no need to mark these volatile.

You are probably right, but in my (long, possibly dated) experience with VC++ it's safer to rely on explicit volatile declarations. It should be noted that the volatile semantics are MS-specific: https://msdn.microsoft.com/en-us/library/12a04hfd.aspx.
It's possible that the compiler has gotten smarter and knows not to reorder accesses around critical sections, but is there really much harm in making it explicit?

orangemocha · 2016-05-15T16:03:17Z

@piscisaureus, I updated the last commit to address most of your feedback. PTAL.
You can view the incremental fixup here, should that make it easier for you to review.

I removed the wait from the main thread, and made the ReadConsole thread restore the console state. This should be safe and as you noted, it’s best not to have any waits in the main thread.

I also added some logic to handle the simple scroll case. If the cursor was at the bottom of the screen buffer, then we can assume that the ENTER we sent caused the screen buffer to scroll up of one line, so we adjust accordingly. Note that I am not handling the case in which the screen buffer scrolled up of more than one line since we called GetConsoleScreenBufferInfo, which would be hard to detect and is hopefully very unlikely in practice. Even if we hit that case, we would end up with the cursor one line below where it should be, which should essentially have the same effect of the ENTER being visible.

I also simplified the synchronization logic by using one status variable and interlocked operations.

saghul · 2016-05-16T07:35:07Z

@piscisaureus I plan to release libuv with this fix (one you approve it) tonight, if you can get it reviewed by then it would be great.

[]

piscisaureus · 2016-05-16T19:33:43Z

src/win/tty.c

+  DWORD written;
+  DWORD err = 0;
+  LONG status;
+


assert(!(handle->flags & UV_HANDLE_CANCELLATION_PENDING))

I can address this now

piscisaureus · 2016-05-16T19:45:21Z

@piscisaureus, I updated the last commit to address most of your feedback. PTAL.
You can view the incremental fixup here, should that make it easier for you to review.

I removed the wait from the main thread, and made the ReadConsole thread restore the console state. This should be safe and as you noted, it’s best not to have any waits in the main thread.

Great!

I also added some logic to handle the simple scroll case. If the cursor was at the bottom of the screen buffer, then we can assume that the ENTER we sent caused the screen buffer to scroll up of one line, so we adjust accordingly. Note that I am not handling the case in which the screen buffer scrolled up of more than one line since we called GetConsoleScreenBufferInfo, which would be hard to detect and is hopefully very unlikely in practice.

Even if we hit that case, we would end up with the cursor one line below where it should be, which should essentially have the same effect of the ENTER being visible.

Sounds good to me.

I also simplified the synchronization logic by using one status variable and interlocked operations.

First of all, good job, it's much cleaner this way.
I think the amount of redundant state could be reduced even further, and the fact that the restore position needs another synchronized variable is a bit suspect to me.

But it's a the fine line between diligent and nitpicky, and many users are clamoring for a fix.
So LGTM, go forth and land.

When we send VK_RETURN to make ReadConsole return, a spurious new line is echoed to the screen. This is pretty visible in Node.js, since it calls uv_tty_read_start() and uv_tty_read_stop() in rapid succession during startup. With this change, we save the screen state just before sending VK_RETURN, and restore the cursor position as soon as ReadConsole returns.

orangemocha · 2016-05-16T20:36:20Z

Added the assert.

orangemocha · 2016-05-16T20:39:51Z

One more CI run: https://ci.nodejs.org/job/libuv-test-commit-windows/57/

saghul · 2016-05-16T21:03:21Z

CI is 💚 and Bert gave us his blessing, so I'm landing this.

Closing the handle does not make ReadConsoleW exit reliably on Windows 7 and above. Thus, after switching from line to raw mode, keypresses were held until enter was pressed. This makes ReadConsoleW exit by writing a return keypress to its input buffer, similar to what was already done for raw mode. Fixes: #852 PR-URL: #866 Reviewed-by: Bert Belder <bertbelder@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

Since we cancel ReadConsole by sending a newline, the duplicate handle is no longer necessary. PR-URL: #866 Reviewed-by: Bert Belder <bertbelder@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

When we send VK_RETURN to make ReadConsole return, a spurious new line is echoed to the screen. This is pretty visible in Node.js, since it calls uv_tty_read_start() and uv_tty_read_stop() in rapid succession during startup. With this change, we save the screen state just before sending VK_RETURN, and restore the cursor position as soon as ReadConsole returns. PR-URL: #866 Reviewed-by: Bert Belder <bertbelder@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

saghul · 2016-05-16T21:07:12Z

Landed in e51442b, 349aa6c and 9eb1311. Thanks Alexis and Joao for the patches, and Bert for the review!

orangemocha · 2016-05-16T21:25:30Z

Awesome! Thanks a lot for the reviews and all the help, @saghul and @piscisaureus !

orangemocha mentioned this pull request May 11, 2016

keypress events not working in windows cmd.exe & powershell nodejs/node#5384

Closed

saghul reviewed May 12, 2016
View reviewed changes

saghul mentioned this pull request May 12, 2016

Prepare 1.9.1 #867

Closed

bnoordhuis mentioned this pull request May 12, 2016

test: test for issue #5574 nodejs/node#5841

Closed

4 tasks

orangemocha force-pushed the joaocgreis-G54-raw-tty-fix branch from 0cf4b49 to ce8faac Compare May 12, 2016 13:50

orangemocha force-pushed the joaocgreis-G54-raw-tty-fix branch from ce8faac to c1cf6dc Compare May 12, 2016 23:09

win,tty: don't duplicate handle for line reads

3ea51e7

Since we cancel ReadConsole by sending a newline, the duplicate handle is no longer necessary.

piscisaureus reviewed May 13, 2016
View reviewed changes

ChiriVulpes mentioned this pull request May 15, 2016

Raw keyboard capture with process.stdin doesn't work as expected. nodejs/help#50

Closed

orangemocha force-pushed the joaocgreis-G54-raw-tty-fix branch from 98548a0 to edfb368 Compare May 15, 2016 15:56

piscisaureus reviewed May 16, 2016
View reviewed changes

orangemocha force-pushed the joaocgreis-G54-raw-tty-fix branch from edfb368 to 5ad6688 Compare May 16, 2016 20:34

saghul closed this May 16, 2016

joaocgreis mentioned this pull request Jul 29, 2016

REPL sometimes prints no output on Windows nodejs/node#7837

Closed

bzoz mentioned this pull request May 26, 2017

CTRL_LOGOFF_EVENT and CTRL_SHUTDOWN_EVENT should be mapped to SIGHUP on Windows #1174

Closed

njsmith mentioned this pull request Jun 13, 2019

Add support for talking to our stdin/stdout/stderr as streams python-trio/trio#174

Open

ihnorton mentioned this pull request Aug 22, 2019

REPL on Windows doesn't display console cursor after moving nodejs/node#20348

Closed

littledivy mentioned this pull request Mar 29, 2024

create-next-app prompts don't work on Windows denoland/deno#21111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

win,tty: fix read stop in line mode #866

win,tty: fix read stop in line mode #866

orangemocha commented May 11, 2016

eljefedelrodeodeljefe commented May 11, 2016

saghul May 12, 2016

orangemocha May 12, 2016

saghul commented May 12, 2016

orangemocha commented May 12, 2016

saghul commented May 12, 2016

MylesBorins commented May 12, 2016 •

edited

Loading

saghul commented May 12, 2016

saghul commented May 12, 2016

MylesBorins commented May 12, 2016

saghul commented May 12, 2016

orangemocha commented May 12, 2016

saghul commented May 13, 2016

MylesBorins commented May 13, 2016

saghul commented May 13, 2016

piscisaureus May 13, 2016

orangemocha May 13, 2016

orangemocha commented May 15, 2016

saghul commented May 16, 2016

piscisaureus May 16, 2016

orangemocha May 16, 2016

piscisaureus commented May 16, 2016

orangemocha commented May 16, 2016

orangemocha commented May 16, 2016

saghul commented May 16, 2016

saghul commented May 16, 2016

orangemocha commented May 16, 2016

win,tty: fix read stop in line mode #866

win,tty: fix read stop in line mode #866

Conversation

orangemocha commented May 11, 2016

eljefedelrodeodeljefe commented May 11, 2016

saghul May 12, 2016

Choose a reason for hiding this comment

orangemocha May 12, 2016

Choose a reason for hiding this comment

saghul commented May 12, 2016

orangemocha commented May 12, 2016

saghul commented May 12, 2016

MylesBorins commented May 12, 2016 • edited Loading

saghul commented May 12, 2016

saghul commented May 12, 2016

MylesBorins commented May 12, 2016

saghul commented May 12, 2016

orangemocha commented May 12, 2016

saghul commented May 13, 2016

MylesBorins commented May 13, 2016

saghul commented May 13, 2016

piscisaureus May 13, 2016

Choose a reason for hiding this comment

orangemocha May 13, 2016

Choose a reason for hiding this comment

orangemocha commented May 15, 2016

saghul commented May 16, 2016

piscisaureus May 16, 2016

Choose a reason for hiding this comment

orangemocha May 16, 2016

Choose a reason for hiding this comment

piscisaureus commented May 16, 2016

orangemocha commented May 16, 2016

orangemocha commented May 16, 2016

saghul commented May 16, 2016

saghul commented May 16, 2016

orangemocha commented May 16, 2016

MylesBorins commented May 12, 2016 •

edited

Loading