Node doesn't reset tty on early/aborted exit

[gotchoices]

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?

[shachaf]

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:

• less starts, saves tty state, sets -echo
• node starts, saves tty state (for some reason?)
• less exits, restores +echo
• node gets SIGPIPE, exits, restores "original" tty state of -echo

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.

[gotchoices]

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"): https://www.linusakesson.net/programming/tty/

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?

[shachaf]

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 If the REPL evaluation code is being used to run scripts, then it's saving and restoring the terminal mode as if it was being used interactively. But there's no reason for it to do that for non-interactive use.

[bnoordhuis]

For anyone who wants to work on this, the relevant logic is in src/node.cc:

• node restores the tty's state most of the time, see ResetStdio (called on normal program exit and SIGINT or SIGTERM but not other signals)

• the state is captured at startup in PlatformInit

the right thing is to just not touch the tty state at all if the program doesn't request it

Node has to work this way because native add-ons can also change the tty's state.

[gotchoices]

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.

[bnoordhuis]

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. :)

[shachaf]

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 https://github.com/nodejs/node/issues/41143#issuecomment-1068241940, 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: @.***>

[brendon]

It could be a coincidence but updating to Node 18 fixed this for me.

[JosephHalter]

I just want to confirm that it's still happening on node 20.12.1, running yarn run serve and Ctrl+C messes up my terminal every time.

[brendon]

It could be a coincidence but updating to Node 18 fixed this for me.

Turns out I was too hasty. It still occurs in 18. Haven't checked 20.

[gotchoices]

I just checked with 20.13.0 and the bug is still present.