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

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

Closed
egoroof opened this issue Feb 23, 2016 · 90 comments

Comments

Projects
None yet
@egoroof
Copy link

commented Feb 23, 2016

  • Version: 5.7.0
  • Platform: windows 10 x64
  • Subsystem: streams?

I make commits using cz-cli (it uses arrow keys for selection). Just updated node to 5.7.0 (from 5.6.0) on windows 10 and now arrow keys (upd also Ctrl+C and maybe something other) not working in command line. Pressing arrow keys does nothing (like keypress event doesn't happen).

Guess it related #2996.

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Feb 23, 2016

@egoroof What terminal are you using?

@egoroof

This comment has been minimized.

Copy link
Author

commented Feb 23, 2016

CMD
upd: PowerShell the same results

@Fishrock123 Fishrock123 changed the title arrow keys stopped working after update to 5.7.0 arrow keys not working in windows cmd.exe Feb 23, 2016

@egoroof egoroof changed the title arrow keys not working in windows cmd.exe keypress events not working in windows cmd.exe & powershell Feb 24, 2016

@rvagg

This comment has been minimized.

Copy link
Member

commented Feb 24, 2016

possibly related report from nodeschool nodeschool/discussions#1641

someone needs to bisect this one, @nodejs/platform-windows can anyone repro?

@egoroof

This comment has been minimized.

Copy link
Author

commented Feb 24, 2016

someone needs to bisect this one

Can I help somehow? Maybe I should run some tests to get more info about the bug?

@rvagg

This comment has been minimized.

Copy link
Member

commented Feb 24, 2016

@egoroof if you can set yourself up with a development environment to compile and test Node on your Windows machine then you could help track down exactly where the bug was introduced. git bisect is a great tool to learn if you haven't before, you need to come up with a good test, manual or automatic, to tell you if a version of Node is broken or not then find a spot in the git history where it's not broken then use git bisect to identify the actual commit where the breakage started and we can go from there.

Unless anyone in here has a better idea of where we can narrow this down to.

@eljefedelrodeodeljefe

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2016

@egoroof if you could reproduce without external dependencies like commitizen that would be great. In the referenced issue, there are already examples. I can reproduce some. Trying to get gdb running on windows and hunt this bug down, too.

@egoroof

This comment has been minimized.

Copy link
Author

commented Feb 24, 2016

Here is the test:

// pressing any key should get the proccess exit
// without the fix you will have to press enter to do it
// it can show you false positives sometimes
// run the test many times to get high probability that it is true passing
process.stdin.setRawMode(true);
process.stdin.on('data', function (char) {
    console.log('Exiting on any key press...');
    process.exit();
});

Here is the results (which I tested with cmd.exe):

works:

  • 5.5.0
  • 5.4.1
  • 5.3.0
  • 5.2.0
  • 5.1.1
  • 5.1.0
  • 4.3.1

doesn't work:

  • 5.7.0
  • 5.6.0
  • 5.0.0

It seems it was originally fixed in 5.1.0 (af46112). Then it returned back in 5.6.0.

@rvagg

This comment has been minimized.

Copy link
Member

commented Feb 24, 2016

if it is to do with streams then it may be in one of the commits that have touched the streams code since v5.5.0:

7684b0f stream: fix no data on partial decode
f706cb0 streams: 5% throughput gain when sending small chunks
ee8d4bb stream: prevent object map change in TransformState
c8b6de2 stream: refactor redeclared variables

Only the last two in that list were in between v5.5.0 and v5.6.0. You could try backing them each out and see if it makes a difference.

Also, /cc @nodejs/streams

@calvinmetcalf

This comment has been minimized.

Copy link
Member

commented Feb 24, 2016

the second to last one is the only one that actually changes anything so I somewhat doubt the problem lies there

@egoroof

This comment has been minimized.

Copy link
Author

commented Feb 26, 2016

Ok. I find this bug is complicated. The test is manual and it can show you false positives sometimes. So I run the test many times to get high probability that it is true passing.
Moreover I find that this test have more chances to be passed if you machine uses lots of resouces. I just run PhpStorm IDE and during the starting of the IDE I run test many times and in most cases the test was passed. Then when the IDE is loaded the test doesn't pass in most cases.

Bisect result is: f1a0827 deps: sync with upstream c-ares/c-ares@2bae2d5.

To be sure that this is the cause:
git checkout nodejs/v5.x
git revert -n f0bd176d6d5523626efb3005cf41f11663bde280
git revert -n b46f3b84d4a35c9bf75a30dcfd68d8a3317713db
git revert -n f1a082741743f51112bbf14d4d300f30e99d54d3
vcbuild nosign

And do testing. Seems to work (I didn't find failures with the test).
See #5090 about these commits.

Can someone create automated test for it? I have no idea how.

@rvagg

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

Thanks for investigating @egoroof, what we first need to do is figure out how on earth c-ares could be implicated on processing stdin on Windows, and if that is indeed the cause of the bug, how do we fix it?

/cc @indutny @saghul (since you merged and reviewed that patch ... just in case you might have a clue here)

@indutny

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

Wow, this is a bit unexpected. I will take a look.

@saghul

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

That bisect result is weird. c-ares is not related in any way to how key presses are handled, which is done in libuv's tty handle.

Unless it's some bizarre define?!

@rvagg

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

From nodeschool/discussions#1641 (comment):

I think I've gotten to the bottom of this. It seems that the problem with the non-responsive menus only occurs when I have an instance of the Microsoft Edge browser running. When I kill all instances of Edge, the tutorial menus do work.

There's a network connection ... doesn't help much but perhaps adds a little bit of credence to the c-ares implication.

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2016

The last issue didn't manifest on Windows 7. I think only 10 and possibly 8 were affected. Can someone verify?

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Feb 29, 2016

Tried to reproduce on Windows 10, Node.js 5.7.0 to no avail. @egoroof is it possible that your bisect is in error? I find it very hard to believe that a update to c-ares is really the cause.

@egoroof

This comment has been minimized.

Copy link
Author

commented Mar 1, 2016

@silverwind maybe. I'll try to do more tests and another bisect. But the fact that reverting those commits makes it working seems to be true.

@egoroof

This comment has been minimized.

Copy link
Author

commented Mar 1, 2016

@silverwind just finished new bisect. Got the same result:

E:\Dropbox\repo\node>git bisect start
E:\Dropbox\repo\node>git bisect bad
E:\Dropbox\repo\node>git bisect good v5.5.0
Bisecting: 152 revisions left to test after this (roughly 7 steps)
[a3a0cf603a2581e3641ddd44015d07d6d367de2c] tools: add arrow function rules to eslint

E:\Dropbox\repo\node>git bisect bad
Bisecting: 75 revisions left to test after this (roughly 6 steps)
[a2881e2187d8b924d09dedf5f091fbacf7323aa7] test: remove test-cluster-* var redeclarations

E:\Dropbox\repo\node>git bisect good
Bisecting: 37 revisions left to test after this (roughly 5 steps)
[dde160378e9f7de3b71014448da3613967502610] doc: fix link in cluster documentation

E:\Dropbox\repo\node>git bisect good
Bisecting: 18 revisions left to test after this (roughly 4 steps)
[ec62789152d4cebdeed64e49cee73a0c131dcd88] crypto: fix memory leak in LoadPKCS12

E:\Dropbox\repo\node>git bisect good
Bisecting: 9 revisions left to test after this (roughly 3 steps)
[8e579ba759b9964adf6b6029d1c37d4468e2f5d3] http: strictly forbid invalid characters from headers

E:\Dropbox\repo\node>git bisect bad
Bisecting: 4 revisions left to test after this (roughly 2 steps)
[c4c8b3bf2ea37c972b07ad820987549f1861ba34] doc: fix dgram doc indentation

E:\Dropbox\repo\node>git bisect bad
Bisecting: 1 revision left to test after this (roughly 1 step)
[b46f3b84d4a35c9bf75a30dcfd68d8a3317713db] src,deps: replace LoadLibrary by LoadLibraryW

E:\Dropbox\repo\node>git bisect bad
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[f1a082741743f51112bbf14d4d300f30e99d54d3] deps: sync with upstream bagder/c-ares@2bae2d5

E:\Dropbox\repo\node>git bisect bad
f1a082741743f51112bbf14d4d300f30e99d54d3 is the first bad commit
commit f1a082741743f51112bbf14d4d300f30e99d54d3
Author: Fedor Indutny <fedor@indutny.com>
Date:   Thu Feb 4 16:34:47 2016 -0500

    deps: sync with upstream bagder/c-ares@2bae2d5

    PR-URL: https://github.com/nodejs/node/pull/5090
    Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

:040000 040000 9cde32ee881592e87cc8f9631ec1ec4aa316ed0a 158a3bae5fb407f6d7212d249542600b5b3ad802 M  deps
:040000 040000 ec2de4335873088bb0c5776400bb40eba289ad9d 3f6e5e0926a51b388849d034fa3faa3b3803ee36 M  src
@silverwind

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2016

Hmm, maybe we can provide some kind of test build with that commit reverted so people who can reproduce can test it?

@Skywalker13

This comment has been minimized.

Copy link

commented Mar 1, 2016

I've done the bisect like @egoroof (Windows 10)

Here my results:

git bisect start
# good: [dd882563e56652f76e25994d046ab49b1bc5f836] 2016-01-20, Version 5.5.0 (Stable)
git bisect good dd882563e56652f76e25994d046ab49b1bc5f836
# bad: [645d4d5d593c3822e567efec47ac375cfd86b83f] 2016-02-09, Version 5.6.0 (Stable)
git bisect bad 645d4d5d593c3822e567efec47ac375cfd86b83f
# good: [612ce66c7816193d43c435336216364a12c3dd53] net: refactor redeclared variables
git bisect good 612ce66c7816193d43c435336216364a12c3dd53
# good: [87b27c913d5c292b36ebde26928b157b82649341] test: fix redeclared test-intl var
git bisect good 87b27c913d5c292b36ebde26928b157b82649341
# good: [95615196de77aedebc921dd52c82f63fd8f9e099] src: clean up usage of __proto__
git bisect good 95615196de77aedebc921dd52c82f63fd8f9e099
# bad: [b46f3b84d4a35c9bf75a30dcfd68d8a3317713db] src,deps: replace LoadLibrary by LoadLibraryW
git bisect bad b46f3b84d4a35c9bf75a30dcfd68d8a3317713db
# good: [9f7aa6f8686ccdcbb3fd012f700a5e551445d30d] doc: clarify dgram socket.send() multi-buffer support
git bisect good 9f7aa6f8686ccdcbb3fd012f700a5e551445d30d
# good: [d9e934c71f1c2b87bb837ac808204391c794c95b] crypto: add `pfx` certs as CA certs too
git bisect good d9e934c71f1c2b87bb837ac808204391c794c95b
# bad: [f1a082741743f51112bbf14d4d300f30e99d54d3] deps: sync with upstream bagder/c-ares@2bae2d5
git bisect bad f1a082741743f51112bbf14d4d300f30e99d54d3
# good: [ec62789152d4cebdeed64e49cee73a0c131dcd88] crypto: fix memory leak in LoadPKCS12
git bisect good ec62789152d4cebdeed64e49cee73a0c131dcd88
# first bad commit: [f1a082741743f51112bbf14d4d300f30e99d54d3] deps: sync with upstream bagder/c-ares@2bae2d5
@Skywalker13

This comment has been minimized.

Copy link

commented Mar 1, 2016

I reverted a small hunk in c-ares and the problem disappears.

Here the changes that I've done on f1a0827

diff --git a/deps/cares/src/ares_init.c b/deps/cares/src/ares_init.c
index 4607944..4c7eaf6 100644
--- a/deps/cares/src/ares_init.c
+++ b/deps/cares/src/ares_init.c
@@ -1068,6 +1068,10 @@ done:
  */
 static int get_DNS_Windows(char **outptr)
 {
+  /* Try using IP helper API GetAdaptersAddresses() */
+  if (get_DNS_AdaptersAddresses(outptr))
+    return 1;
+
   /*
      Use GetNetworkParams First in case of
      multiple adapter is enabled on this machine.
@@ -1078,10 +1082,6 @@ static int get_DNS_Windows(char **outptr)
   if (get_DNS_NetworkParams(outptr))
     return 1;

-  /* Try using IP helper API GetAdaptersAddresses() */
-  if (get_DNS_AdaptersAddresses(outptr))
-    return 1;
-
   /* Fall-back to registry information */
   return get_DNS_Registry(outptr);
 }
@saghul

This comment has been minimized.

Copy link
Member

commented May 25, 2016

@orangemocha (and others) see #6806 Hopefully we will have the patched libuv in Node v4 once we sort out the stdout/err shenanigans.

@jokeyrhyme

This comment has been minimized.

Copy link

commented Jun 2, 2016

@martinheidegger the following SemVer range should hopefully match any

  • known-broken: >=4.0 <4.2 || >=5.0 <5.3 || >=5.6 <6.2
  • known fixed: <4 || >=4.2 <5 || >=5.3 < 6 || >= 6.2

I did consider a runtime feature-detection approach, but some of the earlier reports made it seem non-deterministic.

I'll put together a package to just do a SemVer comparison against the node version unless someone else beats me to it. :)

@jokeyrhyme

This comment has been minimized.

Copy link

commented Jun 2, 2016

@martinheidegger here we go:

Feel free to let me know if it has any false-positives / false-negatives for you. :)

saghul added a commit to saghul/node that referenced this issue Jul 11, 2016

deps: upgrade libuv to 1.9.1
Fixes: nodejs#4002
Fixes: nodejs#5384
Fixes: nodejs#6563
Refs: nodejs#2680 (comment)
PR-URL: nodejs#6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins added a commit that referenced this issue Jul 11, 2016

deps: upgrade libuv to 1.9.1
Fixes: #4002
Fixes: #5384
Fixes: #6563
Refs: #2680 (comment)
PR-URL: #6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins added a commit that referenced this issue Jul 11, 2016

deps: upgrade libuv to 1.9.1
Fixes: #4002
Fixes: #5384
Fixes: #6563
Refs: #2680 (comment)
PR-URL: #6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins added a commit that referenced this issue Jul 12, 2016

deps: upgrade libuv to 1.9.1
Fixes: #4002
Fixes: #5384
Fixes: #6563
Refs: #2680 (comment)
PR-URL: #6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins added a commit that referenced this issue Jul 14, 2016

deps: upgrade libuv to 1.9.1
Fixes: #4002
Fixes: #5384
Fixes: #6563
Refs: #2680 (comment)
PR-URL: #6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins added a commit that referenced this issue Jul 14, 2016

deps: upgrade libuv to 1.9.1
Fixes: #4002
Fixes: #5384
Fixes: #6563
Refs: #2680 (comment)
PR-URL: #6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@amartiuk

This comment has been minimized.

Copy link

commented Mar 29, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.