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 · Fixed by #6796
Closed

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

egoroof opened this issue Feb 23, 2016 · 90 comments · Fixed by #6796
Assignees
Labels
confirmed-bug Issues with confirmed bugs. tty Issues and PRs related to the tty subsystem. windows Issues and PRs related to the Windows platform.

Comments

@egoroof
Copy link

egoroof 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.

@mscdex mscdex added readline Issues and PRs related to the built-in readline module. windows Issues and PRs related to the Windows platform. labels Feb 23, 2016
@Fishrock123
Copy link
Contributor

@egoroof What terminal are you using?

@egoroof

This comment was marked as abuse.

@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
Copy link
Member

rvagg 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 was marked as abuse.

@rvagg
Copy link
Member

rvagg 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
Copy link
Contributor

@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 was marked as abuse.

@rvagg
Copy link
Member

rvagg 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
Copy link
Contributor

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

@egoroof

This comment was marked as abuse.

@rvagg
Copy link
Member

rvagg 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
Copy link
Member

indutny commented Feb 26, 2016

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

@saghul
Copy link
Member

saghul 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
Copy link
Member

rvagg 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
Copy link
Contributor

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

@silverwind
Copy link
Contributor

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 was marked as abuse.

@egoroof

This comment was marked as abuse.

@silverwind
Copy link
Contributor

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

@Skywalker13
Copy link

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
Copy link

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);
 }

Skywalker13 referenced this issue Mar 1, 2016
PR-URL: #5090
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
@saghul
Copy link
Member

saghul 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
Copy link

@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
Copy link

@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
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 pushed a commit that referenced this issue Jul 11, 2016
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 pushed a commit that referenced this issue Jul 11, 2016
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 pushed a commit that referenced this issue Jul 12, 2016
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 pushed a commit that referenced this issue Jul 14, 2016
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 pushed a commit that referenced this issue Jul 14, 2016
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>
@ghost
Copy link

ghost 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
Labels
confirmed-bug Issues with confirmed bugs. tty Issues and PRs related to the tty subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet