-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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 doesn't reset tty on early/aborted exit #41143
Comments
Based on a bit of strace investigation, I think the issue isn't that node doesn't reset the tty state, it's that it does (and shouldn't). Here's what I think is happening:
node should probably not touch the tty state at all in this sort of situation where it's just being used to print to stdout. |
Seems like isatty() can/should be called (if its not already) to determine if stdout is a terminal or a file/pipe... I can confirm that if I redirect stdin and stderr to/from /dev/null, the problem goes away. If I redirect either one (but not both) the problem persists. It looks like node is checking to see if any one of the three (stdin, stdout, stderr) is a terminal. If any one of the three is, it stores/restores the tty. I'm not sure what the correct logic is, but probably not that. Here's an article with a bit of guidance about 20% in (search for "pipeline"): Sounds like in a pipeline, there is a foreground process (less in this case) that is "in charge" of dealing with the terminal. I wonder if there is some more meaningful measure than isatty()? Or may it is as simple as testing stdout and ignoring what stdin and stderr are doing? |
I think the right thing is to just not touch the tty state at all if the program doesn't request it. That's what most programs do by default and it behaves correctly. From a quick search of the code, my guess is that it's happening here: https://github.com/nodejs/node/blob/master/lib/repl.js#L547-L591 |
For anyone who wants to work on this, the relevant logic is in src/node.cc:
Node has to work this way because native add-ons can also change the tty's state. |
I like shachaf's opinion of leaving the tty alone unless node is explicitly launched in interactive mode (and therefore not buried in a pipeline somewhere). If you're not running some kind of command-line input, there's probably no reason to diddle the tty settings. If an add-on wants to change tty settings, it should probably register its own exit hook (assuming that can be done) to restore settings. But it doesn't seem to me like node should always assume the tty needs to be restored just because some add-on might have done something to it. |
Anything that requires the ecosystem to move is pretty much DOA. The yubikey openssl engine (provides TLS keys to node) is a good example of why that isn't workable. It leaves the tty in a bad state if you press ^C on the passphrase prompt and an exit hook won't fix that because node and the yubikey are unaware of each other's existence. (And let's not get into the perils of cleaning up from signal handlers!) Basically, if you want to see this fixed, work within the existing framework. Happy to review pull requests, just cc me. :) |
Compatibility with existing programs seems important. I think an opt-in fix
(e.g. setting a flag to tell ResetStdio to leave the terminal as-is, which
you could set if your program isn't supposed to affect terminal state)
would be fine -- right now the bug is just unavoidable.
…On Tue, Mar 15, 2022, 10:13 Ben Noordhuis ***@***.***> wrote:
Anything that requires the ecosystem to move is pretty much DOA.
The yubikey openssl engine (provides TLS keys to node) is a good example
of why that isn't workable. It leaves the tty in a bad state if you press
^C on the passphrase prompt and an exit hook won't fix that because node
and the yubikey are unaware of each other's existence.
(And let's not get into the perils of cleaning up from signal handlers!)
Basically, if you want to see this fixed, work within the existing
framework. Happy to review pull requests, just cc me. :)
—
Reply to this email directly, view it on GitHub
<#41143 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB3UKRF5A3DPQP5NU2VLYLVADAMDANCNFSM5J3JE5MA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you commented.Message ID:
***@***.***>
|
It could be a coincidence but updating to Node 18 fixed this for me. |
I just want to confirm that it's still happening on node 20.12.1, running |
Turns out I was too hasty. It still occurs in 18. Haven't checked 20. |
I just checked with 20.13.0 and the bug is still present. |
Version
v16.13.1, v17.2.0 tested
Platform
Linux lux0 5.11.22-100.fc32.x86_64 #1 SMP Wed May 19 18:58:25 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Subsystem
No response
What steps will reproduce the bug?
Create this script file in test.js:
#!/usr/bin/env node
for (let i = 0; i < 100000; i++) {
console.log("i:", i)
}
In a shell window, run:
test.js |less
Hit "q" to quit less (after only one page of output), which also kills test.js while it still has undelivered data in the stdout buffer.
Your tty will be left in raw mode (no echo, other command characters disabled). Must do a "stty sane" to restore usability of the console.
How often does it reproduce? Is there a required condition?
Happens every time. Have tested multiple versions of node on Linux and MacOS.
What is the expected behavior?
Should restore my tty settings no matter how ugly the exit conditions.
What do you see instead?
Tty is left in raw mode with no echo. Can't see my typed commands.
Additional information
Is there a standard way of dealing with this in case a script is killed before it can write its output? Or is this a bug in node?
The text was updated successfully, but these errors were encountered: