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

Double keypress in readline / process.stdin #25875

Closed
Ezekiel-DA opened this issue Feb 1, 2019 · 5 comments · Fixed by #26037
Closed

Double keypress in readline / process.stdin #25875

Ezekiel-DA opened this issue Feb 1, 2019 · 5 comments · Fixed by #26037
Labels
doc Issues and PRs related to the documentations. libuv Issues and PRs related to the libuv dependency or the uv binding. readline Issues and PRs related to the built-in readline module.

Comments

@Ezekiel-DA
Copy link

  • Version: 11.8.0, 11.9.0
  • Platform: Windows 10 (x64)
  • Subsystem: console

This code produces repeated output:

const readline = require('readline')
const process = require('process')

let rl = readline.createInterface({terminal: true, input: process.stdin, output: process.stdout})
rl.resume()
rl.input.on('data', (chunk) => {
  console.log(`Received ${chunk.length} bytes of data - content was: ${chunk}`)
})

Run this with Node 11.8.0 or above and press the down arrow; two messages are produced:

Received 3 bytes of data - content was:

Received 3 bytes of data - content was:

Revert to Node 11.7.0, a single message is produced.

Received 3 bytes of data - content was:

This breaks, among many things, scrolling through menus in Inquirer.js: SBoudrias/Inquirer.js#778

This appears to be Windows specific (as far as I could tell, I was only able to test on a Linux VM via Docker right now).

Git bisect tells me this issue first appeared with c0859d7, which upgraded libuv to 1.25.0.

I'll keep digging into libuv (I already have an idea of where the problem was introduced in 1.25.0) but since this impacts Node on Windows somehwat significantly I figured it would be useful to have a way to track this!

@devsnek
Copy link
Member

devsnek commented Feb 1, 2019

@nodejs/libuv @nodejs/repl

@devsnek devsnek added readline Issues and PRs related to the built-in readline module. libuv Issues and PRs related to the libuv dependency or the uv binding. labels Feb 1, 2019
@Fishrock123 Fishrock123 added doc Issues and PRs related to the documentations. and removed libuv Issues and PRs related to the libuv dependency or the uv binding. labels Feb 1, 2019
@Fishrock123
Copy link
Contributor

Fishrock123 commented Feb 1, 2019

@Ezekiel-DA Can you try changing that logging line to use chunk.toString('hex') and comment with the output?

I get:

Received 3 bytes of data - content was: 1b5b42

The libuv change * tty,win: fix Alt+key under WSL (Bartosz Sosnowski) seems suspect to have changed that on your system.


However, I am quite sure this is not a bug.

The 'data' event you are listening to is from process.stdin itself and it's intention is to emit data events as the stdio connection passes them. This is not what you should be using, but I can't blame you - the documentation for what you should be using is actually missing completely.

Node.js internally uses this functionality to condense stdio connection events into what we would expect -- keypresses.

That isn't directly exposed but is used in readline.emitKeypressEvents(), which is automatically used on readline when attached to a Pipe or a Raw-mode TTY (terminal). The events are emitted off of the stream (rl.input), though, for some reason.

The resulting output from the 'keypress' and the down arrow key looks like this:

[Arguments] {
  '0': undefined,
  '1':
   { sequence: '\u001b[B',
     name: 'down',
     ctrl: false,
     meta: false,
     shift: false,
     code: '[B' } 
}

... I don't really know why this event is emitted where it is and why it is not documented, but this is what you should use, I think.

@Ezekiel-DA
Copy link
Author

@Fishrock123 Thanks for looking into this! The only reason I listen to 'data' is to try and find a minimal reproduction for a change in behavior that impacts Inquirer (see SBoudrias/Inquirer.js#778) and appeared when libuv was upgraded to 1.25.0.

I'm pretty sure I found the source of the problem in libuv, and I opened libuv/libuv#2168. As you can see there, the specific scenario of Windows, special keys (e.g. down arrow here) hits a regression that produces an even for both key down and key up, which I'm pretty sure is not the desired behavior!

@Fishrock123 Fishrock123 added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Feb 2, 2019
@RReverser
Copy link
Member

As a data point, I can also reproduce this issue and has been happening since last Node.js upgrade (Windows 10 x64 1809).

@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2019

Word on the street is that libuv/libuv#2160 will fix the issue.

cjihrig pushed a commit to libuv/libuv that referenced this issue Feb 10, 2019
Refs: #2114
Refs: nodejs/node#25875
Refs: nodejs/node#26013
Fixes: #2168
PR-URL: #2160
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
cjihrig added a commit to cjihrig/node that referenced this issue Feb 13, 2019
Notable changes:
- A bug that could result in 100% CPU utilization in Node
  has been fixed (libuv/libuv#2162)
- Node's report module will now include the entire Windows
  product name (libuv/libuv#2170)

PR-URL: nodejs#26037
Fixes: nodejs#26013
Fixes: nodejs#25875
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this issue Feb 13, 2019
Notable changes:
- A bug that could result in 100% CPU utilization in Node
  has been fixed (libuv/libuv#2162)
- Node's report module will now include the entire Windows
  product name (libuv/libuv#2170)

PR-URL: #26037
Fixes: #26013
Fixes: #25875
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
njlr pushed a commit to buckaroo-pm/libuv that referenced this issue Apr 5, 2019
Refs: libuv#2114
Refs: nodejs/node#25875
Refs: nodejs/node#26013
Fixes: libuv#2168
PR-URL: libuv#2160
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue May 16, 2019
Notable changes:
- A bug that could result in 100% CPU utilization in Node
  has been fixed (libuv/libuv#2162)
- Node's report module will now include the entire Windows
  product name (libuv/libuv#2170)

PR-URL: nodejs#26037
Fixes: nodejs#26013
Fixes: nodejs#25875
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this issue May 16, 2019
Notable changes:
- A bug that could result in 100% CPU utilization in Node
  has been fixed (libuv/libuv#2162)
- Node's report module will now include the entire Windows
  product name (libuv/libuv#2170)

Backport-PR-URL: #27728
PR-URL: #26037
Fixes: #26013
Fixes: #25875
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
musm pushed a commit to musm/libuv that referenced this issue Jul 9, 2020
Refs: libuv#2114
Refs: nodejs/node#25875
Refs: nodejs/node#26013
Fixes: libuv#2168
PR-URL: libuv#2160
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
(cherry picked from commit 7ed1ece)
musm pushed a commit to musm/libuv that referenced this issue Jul 9, 2020
Refs: libuv#2114
Refs: nodejs/node#25875
Refs: nodejs/node#26013
Fixes: libuv#2168
PR-URL: libuv#2160
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
(cherry picked from commit 7ed1ece)
musm pushed a commit to musm/libuv that referenced this issue Jul 9, 2020
Refs: libuv#2114
Refs: nodejs/node#25875
Refs: nodejs/node#26013
Fixes: libuv#2168
PR-URL: libuv#2160
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
(cherry picked from commit 7ed1ece)
musm pushed a commit to musm/libuv that referenced this issue Jul 14, 2020
Refs: libuv#2114
Refs: nodejs/node#25875
Refs: nodejs/node#26013
Fixes: libuv#2168
PR-URL: libuv#2160
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
(cherry picked from commit 7ed1ece)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. libuv Issues and PRs related to the libuv dependency or the uv binding. readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants