Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Looks like readline don't always triggers keypress events on Windows #2996

Closed
SBoudrias opened this issue Sep 22, 2015 · 40 comments
Closed

Looks like readline don't always triggers keypress events on Windows #2996

SBoudrias opened this issue Sep 22, 2015 · 40 comments
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem. windows Issues and PRs related to the Windows platform.

Comments

@SBoudrias
Copy link

Hi there, I'm the author on Inquirer.

I would've love to have more details to report on this bug. But I don't have much currently, I'm hoping someone would be able to help in getting more precise information. Also, due to this blindness, I can't exclude totally that this is an issue with Inquirer; all I know is these bug reports started to appear on my repo recently.

Basically, it looks like on Windows (since mostly Node 4.0), readlines don't always triggers keypress events the first time we initialize one. This have reported on Inquirer repository: SBoudrias/Inquirer.js#266

Some people the issue have said they've been able to reproduce using only a readline directly, but it's not super clear what they've done.

On my side, I've been unable to reproduce this bug on any Windows VM. So I really have a hard time to come up with reproducible step for you.

@maxdow
Copy link

maxdow commented Sep 22, 2015

Here an example to reproduce the problem :

var readline = require("readline");

var rl = readline.createInterface(process.stdin, process.stdout);

rl.on("line",function(){
  console.log("line event");
});

process.stdin.on("keypress",function(){
  console.log("keypress");
});

On a win8 computer, i don't have the keypress event triggered at first. I have to press enter to "trig" the keypress event. On win xp there is no problem, i got keypress and line events separatly.

I suppose it works the same as win xp on linux .

Inquirer.js list run fine on win xp that's why it's probably related to node

@peterblazejewicz
Copy link

same: Win: 8.1 Node: 4.1.0

@brendanashworth brendanashworth added readline Issues and PRs related to the built-in readline module. windows Issues and PRs related to the Windows platform. labels Sep 22, 2015
@sayedihashimi
Copy link

+1 ran into this as well

@cveld
Copy link

cveld commented Sep 25, 2015

On Windows 10 Node 4.1.0 I don't have the issue in a node session kicked off from cmd.exe. Although the Node interactive window within Visual Studio 2015 does not show any event handler activity. But my guess is that this is due the design of the Node interactive window.

@tienloc1
Copy link

On Windows 8.1 (node-v0.12.6), the keypress events work for me in cmd.exe but it still not work in git bash. (git-v2.5.3).

@dthree
Copy link

dthree commented Sep 29, 2015

Also ran into this.

@darrellenns
Copy link

I'm also having this issue since upgrading to windows 10. Using maxdow's sample code, I get no keypress events until I press enter. Once I press enter, all the previous keypress events trigger at once. After that, keypress events trigger as normal.

I have tried this in both powershell and cmd, and I've also tried changing them to "legacy console" (in cmd/powershell preferences). All had the same behavior.

@darrellenns
Copy link

Also just tried opening node.js in a new window using the command "start node test.js". Still exhibits the same issue.

@silverwind
Copy link
Contributor

I suspect it could be related to aed6bce. Could one of you try if the issue persists with the last build before this commit, 2.0.1: https://iojs.org/download/release/v2.0.1/win-x64/ ?

@peterblazejewicz
Copy link

@silverwind
You could be on the right track. My setup: Win 8.1, Node 4.1.1. I've symlinked to 2.0.1:

node --version
v2.0.1

and the problem goes away.
We are tracking this problem on project that uses readline under the hood:
OmniSharp/generator-aspnet#351
Thanks!

@edi9999
Copy link

edi9999 commented Oct 6, 2015

Same problem here, as reported here : workshopper/workshopper-adventure#11

@silverwind
Copy link
Contributor

cc: @rlidwka

@peterblazejewicz could you by change confirm that https://iojs.org/dist/v2.0.2/win-x64/ breaks it?

@peterblazejewicz
Copy link

@silverwind

@peterblazejewicz could you by change confirm that https://iojs.org/dist/v2.0.2/win-x64/ breaks it?

Well, I've used and tested v.2.0.1 - which appears to fix problem. I'll test v2.0.2 tomorrow (CEST) and post back.

@silverwind
Copy link
Contributor

FYI, I was able to narrow down a regression range on #3194 (comment), which may be related to this.

@peterblazejewicz
Copy link

@peterblazejewicz could you by change confirm that https://iojs.org/dist/v2.0.2/win-x64/ breaks it?

@silverwind No, it does not. It still works with 2.0.2. I've double checked this. Works fine.

@silverwind
Copy link
Contributor

Regression range determined by the test case in #2996 (comment): v2.3.1...v2.3.2.

@yugangw-msft
Copy link

@silverwind, by reverting the 8cee8f5, I am able to get both "iojs-2.3.2" and "node 4.x" work.

I debugged a little, node has one thread hanging on ReadConsoleW till you press "enter" key. After reversion, node still executes the wait code, but quickly continues with error of "invalid handle" . Based on the property naming used by the original code, the error probably is legit.

So, could we back out this change, which appears a simple code improvement with less impact. We can always come back to it later.

@scottdallamura
Copy link

I have a readline-only repro. At the first press of enter, all of the pending keypress events fire, resulting in an extra echo of the first input line. This repros on win8.1 and win10, but not on a mac.

var readline = require('readline');

var rl = readline.createInterface({input: process.stdin, output: process.stdout});

rl.question("input 1: ", function (answer1) {
    console.log("you entered " + answer1);
    rl.question("input 2: ", function (answer2) {
        console.log("you entered " + answer2);
        rl.question("input 3: ", function (answer3) {
            console.log("you entered " + answer3);
            rl.question("input 4: ", function (answer4) {
                console.log("you entered " + answer4);
                rl.close();
            });
        });
    });
});
C:\node>node readlinetest.js
input 1: 111
111
you entered 111
input 2: 222
you entered 222
input 3: 333
you entered 333
input 4: 444
you entered 444

@silverwind
Copy link
Contributor

cc: @chrisdickinson @indutny

silverwind added a commit that referenced this issue Dec 4, 2015
This reverts 8cee8f5 which was causing stdin to behave strangely on
Windows 8 and 10. The suspected explanation for the issue is that there
might be a race condition occuring when stdin._readableState.reading is
set indirectly through `push('')`.

PR-URL: #3490
Fixes: #2996
Fixes: #2504
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
silverwind added a commit that referenced this issue Dec 17, 2015
This reverts 8cee8f5 which was causing stdin to behave strangely on
Windows 8 and 10. The suspected explanation for the issue is that there
might be a race condition occuring when stdin._readableState.reading is
set indirectly through `push('')`.

PR-URL: #3490
Fixes: #2996
Fixes: #2504
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@nzakas
Copy link

nzakas commented Dec 21, 2015

Any chance this fix will make it into LTS? ESLint is continuing to get reports of this problem from LTS users as of 4.2.3.

@MylesBorins
Copy link
Contributor

@nzakas it is in staging and planned to land in 4.2.4 :D

@nzakas
Copy link

nzakas commented Dec 21, 2015

Awesome, thanks @thealphanerd

silverwind added a commit that referenced this issue Dec 23, 2015
This reverts 8cee8f5 which was causing stdin to behave strangely on
Windows 8 and 10. The suspected explanation for the issue is that there
might be a race condition occuring when stdin._readableState.reading is
set indirectly through `push('')`.

PR-URL: #3490
Fixes: #2996
Fixes: #2504
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@peterblazejewicz
Copy link

4.2.4 (LTS) on Windows 10 works terrific in all terminals I'm using:

  • CMD
  • PowerShell
  • Git Bash

Thanks!

@ThaJay
Copy link

ThaJay commented Feb 18, 2016

looks like this issue is back:
I just had this bug in Windows 10 with Git Bash or PowerShell, Node 5.6.0 and eslint 2.1.0.
As all topics I have found say this is fixed, I installed the latest Git for Windows (version 2.7.1).
The arrow keys still did not work.
eslint/eslint#3395 (comment)

@hgcummings
Copy link

As noted in the eslint issue thread linked above, I've seen this working with Node 5.4.1 and failing in 5.6.0, on the same Windows 10 box. Consistent across multiple terminals (cmd, PowerShell, bash).

@ghost
Copy link

ghost commented Feb 29, 2016

I'm having this issue still on Node 5.7.0, Windows 10.

@silverwind
Copy link
Contributor

See #5384 for the current issue.

@ghost
Copy link

ghost commented Mar 29, 2018

Keypress doesn't work in Git Bash for me also. But it works fine with ConEmu64 (https://conemu.github.io/) on Windows 10.

@sschmidTU
Copy link

Keypresses are actually executed invisibly in Git Bash. At least for me.
It's a Git bash/Non-TTY terminal issue, FoRVaiS pretty much researched all there is to know about it here:
terkelg/prompts#36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.