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

node 10.2.0+ turning off stty echo when using process.stdin.setRawMode() #21020

Closed
jdx opened this issue May 29, 2018 · 8 comments
Closed

node 10.2.0+ turning off stty echo when using process.stdin.setRawMode() #21020

jdx opened this issue May 29, 2018 · 8 comments
Labels
confirmed-bug Issues with confirmed bugs. tty Issues and PRs related to the tty subsystem.

Comments

@jdx
Copy link
Contributor

jdx commented May 29, 2018

  • Version: 10.2.0
  • Platform: MacOS
  • Subsystem: Unsure right now

Starting with node 10.2.0 it is behaving as if I'm running stty -echo and hides the echo output when using heroku run and typing characters in. Node 10.3.0 and 10.3.1 also have the issue, but 10.1.0 and lower does not.

Right now I'm working on providing a simpler example that does not involve the Heroku CLI. I'm assuming this is something related to using .setRawMode() but will update when I have more information.

Update: Here is a simpler example: node -e "process.stdin.setRawMode(true)" in bash. It sets stty -echo.

If you run this in node 10.1.0, nothing happens. If you run it in 10.2.0 in bash, it will turn echo off.

@addaleax addaleax added tty Issues and PRs related to the tty subsystem. v10.x labels May 29, 2018
@addaleax
Copy link
Member

Thanks for reporting this! Do you have any reproduction that doesn’t involve extra setup, or alternatively a standalone reproduction that you could share somewhere?

@jdx
Copy link
Contributor Author

jdx commented May 29, 2018

just run node -e "process.stdin.setRawMode(true)" in bash

@jdx
Copy link
Contributor Author

jdx commented May 29, 2018

I thought it happened when I ran process.stdin.unref(), but it seems that all you need is process.stdin.setRawMode(true)

@jdx
Copy link
Contributor Author

jdx commented May 29, 2018

Here this shows the full behavior:

bash-4.4$ node -v; node -e "process.stdin.setRawMode(true)"; stty
v10.1.0
speed 9600 baud;
lflags: -icanon -isig -iexten echoe echoke echoctl
iflags: -icrnl -ixon iutf8 -brkint
oflags: -oxtabs
cflags: cs8 -parenb
bash-4.4$ n 10.2.0
bash-4.4$ node -v; node -e "process.stdin.setRawMode(true)"; stty
v10.2.0
speed 9600 baud;
lflags: -icanon -isig -iexten -echo echoe echoke echoctl
iflags: -icrnl -ixon iutf8 -brkint
oflags: -oxtabs
cflags: cs8 -parenb

@jdx jdx changed the title node 10.2.0+ turning off stty echo node 10.2.0+ turning off stty echo when using process.stdin.setRawMode() May 29, 2018
@addaleax addaleax added the confirmed-bug Issues with confirmed bugs. label May 29, 2018
@addaleax
Copy link
Member

This was caused by my 17e289e (#19377). My initial guess would be that closing the libuv handle for stdin closes the corresponding file descriptor and leaves libuv unable to reset the state for that file descriptor in uv_tty_reset_mode().

@jdx
Copy link
Contributor Author

jdx commented May 29, 2018

what a shame! seems you worked hard on that PR!

@addaleax
Copy link
Member

I did. 😄 I don’t think we need a full revert here, but addressing this properly is not exactly trivial either. The quick-and-probably-good-enough solution to this problem would be this:

--- a/src/node.cc
+++ b/src/node.cc
@@ -4289,6 +4289,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
   WaitForInspectorDisconnect(&env);
 
   env.set_can_call_into_js(false);
+  uv_tty_reset_mode();
   env.RunCleanup();
   RunAtExit(&env);
 

It does address the problem locally for me, and I think I’ll open a PR with it in a bit.

addaleax added a commit to addaleax/node that referenced this issue May 31, 2018
Before PR 20592, closing all handles associated with the main
event loop would also mean that `uv_tty_reset_mode()`
can’t function properly because the corresponding FDs have
already been closed.

Add regression tests for this condition.

Refs: nodejs#21020
Refs: nodejs#20592
addaleax pushed a commit that referenced this issue May 31, 2018
Record the state of the stdio file descriptors on start-up and restore
them to that state on exit.  This should prevent issues where node.js
sometimes leaves stdio in raw or non-blocking mode.

Co-authored-by: Krzysztof Taborski <taborskikrzysztof@gmail.com>
PR-URL: #20592
Fixes: #14752
Fixes: #21020
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jdx
Copy link
Contributor Author

jdx commented May 31, 2018

I really appreciate it!

addaleax added a commit that referenced this issue Jun 1, 2018
Before PR 20592, closing all handles associated with the main
event loop would also mean that `uv_tty_reset_mode()`
can’t function properly because the corresponding FDs have
already been closed.

Add regression tests for this condition.

Refs: #21020
Refs: #20592

PR-URL: #21027
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this issue Jun 1, 2018
Before PR 20592, closing all handles associated with the main
event loop would also mean that `uv_tty_reset_mode()`
can’t function properly because the corresponding FDs have
already been closed.

Add regression tests for this condition.

Refs: #21020
Refs: #20592

PR-URL: #21027
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit to evanlucas/node that referenced this issue Jun 11, 2018
Otherwise, closing all handles associated with the main
event loop would also mean that `uv_tty_reset_mode()`
can’t function properly because the corresponding FDs have
already been closed.

Fixes: nodejs#21020
addaleax added a commit that referenced this issue Jun 11, 2018
Otherwise, closing all handles associated with the main
event loop would also mean that `uv_tty_reset_mode()`
can’t function properly because the corresponding FDs have
already been closed.

Fixes: #21020
PR-URL: #21257
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
addaleax added a commit that referenced this issue Jun 29, 2018
Before PR 20592, closing all handles associated with the main
event loop would also mean that `uv_tty_reset_mode()`
can’t function properly because the corresponding FDs have
already been closed.

Add regression tests for this condition.

Refs: #21020
Refs: #20592

PR-URL: #21027
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this issue Aug 16, 2018
Before PR 20592, closing all handles associated with the main
event loop would also mean that `uv_tty_reset_mode()`
can’t function properly because the corresponding FDs have
already been closed.

Add regression tests for this condition.

Refs: #21020
Refs: #20592

PR-URL: #21027
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue May 24, 2019
Record the state of the stdio file descriptors on start-up and restore
them to that state on exit.  This should prevent issues where node.js
sometimes leaves stdio in raw or non-blocking mode.

This is a reworked version of commit c2c9c0c from May 2018 that was
reverted in commit 14dc17d from June 2018. The revert was a little
light on details but I infer that the problem was caused by a missing
call to `uv_tty_reset_mode()`.

Apropos the NOLINT comments: cpplint doesn't understand do/while
statements, it thinks they're while statements without a body.

Fixes: nodejs#14752
Fixes: nodejs#21020
Original-PR-URL: nodejs#20592
Trott pushed a commit to Trott/io.js that referenced this issue Jun 13, 2019
Record the state of the stdio file descriptors on start-up and restore
them to that state on exit.  This should prevent issues where node.js
sometimes leaves stdio in raw or non-blocking mode.

This is a reworked version of commit c2c9c0c from May 2018 that was
reverted in commit 14dc17d from June 2018. The revert was a little
light on details but I infer that the problem was caused by a missing
call to `uv_tty_reset_mode()`.

Apropos the NOLINT comments: cpplint doesn't understand do/while
statements, it thinks they're while statements without a body.

Fixes: nodejs#14752
Fixes: nodejs#21020
Original-PR-URL: nodejs#20592

PR-URL: nodejs#24260
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR pushed a commit that referenced this issue Jun 17, 2019
Record the state of the stdio file descriptors on start-up and restore
them to that state on exit.  This should prevent issues where node.js
sometimes leaves stdio in raw or non-blocking mode.

This is a reworked version of commit c2c9c0c from May 2018 that was
reverted in commit 14dc17d from June 2018. The revert was a little
light on details but I infer that the problem was caused by a missing
call to `uv_tty_reset_mode()`.

Apropos the NOLINT comments: cpplint doesn't understand do/while
statements, it thinks they're while statements without a body.

Fixes: #14752
Fixes: #21020
Original-PR-URL: #20592

PR-URL: #24260
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants