Problem with newer less

TL;DR: node -h | less results in some unexpected behavior of "less".

The issue also happens with js-beautify -h | less and with node -e 'console.log(123)' | less.

It was analyzed that when node exits, it resets the terminal state to the state which it read on init, but that reset happens while less is already/still running, and changes the terminal state under its feet (less may change the terminal state on init, and restore it on exit).

This apparently has always been an issue/race, but a recent version of less changed the timing of the terminal init, and now it results in an issue.

The preferred solution in these cases is that if node only prints to stdout, and stdout is not a tty, then it should not touch the terminal state.

It's unclear what should happen if it prints to other streams beyond stdout (mainly stderr).

Isn't that an issue with less, and not Node.js?

Isn't that an issue with less, and not Node.js?

That's a rather dismissive question, is it not?

If you believe or think that it's not a problem of node, then you should explain why you think that, and preferably show at least one other non-interactive application (or which can be interactive but currently runs in non-interactive mode) besides node that changes the terminal state when it only prints things to stdout while stdout is not a tty.

Hint: you would have to try very hard to find such application, but when you do, please file a bug with them, similar to this issue with node.

Non-interactive applications should not touch the terminal state (unless that's what they're designed to do - like tput). Not when they start, not when they exit, not anywhere in between, and especially not when their stdout is not a tty.

Otherwise, for instance if they're part of a pipe but not the final pipe-element, then they have no mandate and no business to touch the terminal state, because they don't print to the terminal - stdout is not a tty.

That's a rather dismissive question, is it not?

Perhaps it was, unintentionally, but I didn't mean any disrespect. I was merely asking since a less upgrade caused the issue, not a Node.js one. I assure you again, I'm not trying to dismiss the concern.

No offense taken, but again, if you think it's not a node problem that node -h | whatever or node -h > help.node touches the terminal state, then you should explain why.

The change in less was the timing of the terminal init. This race always existed, but was not noticed or didn't manifest in the past. Now it has, and the reason was identified as node resetting the terminal state when it exits, even when it's non interactive and its stdout is not a tty.

In general, only interactive applications have mandate to change the terminal state.

Applications which only read stdin and/or print to stdout/stderr typically have no need to touch the terminal state. But even if they do because $reasons, these reasons typically don't exist when they don't even print to the terminal - like when they're part of a pipe but not the final pipe element.

Exceptions do exist, like sudo command | interactive-app where sudo may want to disable terminal echo while it asks for a password, and this is indeed a problem, because in this case sudo is also interactive, and such a pipeline is indeed a recipe for trouble.

But this is not the case with purely non-interactive applications, like js-beautify file.js | less or node -h > file.

But this is not the case with purely non-interactive applications, like js-beautify file.js | less or node -h > file.

A question is whether node knows in advance whether the script is interactive or not, or whether it can avoid the issue for non-interactive scripts without breaking interactive scripts (even partially).

A question is whether node knows in advance whether the script is interactive or not

I don't think that's the question, because I don't think "in advance" is relevant. No one can know in advance (it could take code as user input, for instance).

Without being familiar with the internal architecture and initializations, and excluding cases where the application itself changes the terminal settings, the I'd think node itself should only touch the terminal state on first interaction with the terminal (an fd which is a tty) which requires it, and preferably not at all if it's not required.

Typically stdout/stderr don't need touching the terminal state at all, even when the stream is a tty (and even on Windows, if done right - by using WriteConsoleW - note W - and without changing the console codepage to UTF-8).

When reading stdin, if it's a tty, then this typically makes it an interactive application, and if node requires touching the terminal state in such case, then so be it.

The point is to touch the state only if necessary, and only at the first time it's actually necessary. Not blindly.

I disagree, because the user can start typing before the first read by the script. So, for scripts that need raw mode, the terminal should be put in raw mode as soon as possible, i.e. at the beginning.

IMHO, the safest way to avoid the issue would be a declaration at the beginning of the scripts to tell whether the terminal should be put to raw mode. The default could be the current behavior or something smarter.

So node should restore the status of the file descriptions it has changed

Are we arguing something? Because it's the 3rd time I'm saying it can restore it if it sees fit, and it seems that you agree with that. I fail to see how does this comment contribute to this discussion...

You said "The O_NONBLOCK is a state between node and libc. Node should be free to change/reset it whenever it wants, and it should have no visible impact for other applications with the same controlling terminal." This is incorrect.

This is incorrect.

Maybe, and I said I didn't follow exacly your links, but regardless, it's unrelated to this issue. If it wants to restore it, it can, and on that we agree, even if for different reasons and/or lack of knowledge on my part. The bottom line is the same.

It just doesn't have anything to do with resetting the terminal state - which is the subject of this issue.

So unless someone thinks that touching O_NONBLOCK is related to this issue in some way (which it may be, if we missed something), can we leave this aside and focus on the terminal state only?

If it wants to restore it, it can,

Not "it can". It must restore it. Please do not suggest to introduce a new issue.

@avih

@avih

Do you know who sets the terminal mode to raw, and when?

the built-in terminal repl does, through readline. it needs to handle editing and completion keys before enter. applications can also request raw mode through readline or process.stdin.setRawMode(true):

node/lib/internal/readline/interface.js

Lines 331 to 338 in 019e869

    
           emitKeypressEvents ??= require('internal/readline/emitKeypressEvents'); 
        
           emitKeypressEvents(input, this); 
        
           // `input` usually refers to stdin 
        
           input.on('keypress', onkeypress); 
        
           input.on('end', ontermend); 
        
           this[kSetRawMode](true);

for node -e 'console.log(123)' | cat, it doesn't enable raw mode. i checked the source and traced tcsetattr() calls on my local macos build. node -h | cat behaved the same way. it was obvious though.

startup saves the terminal settings, and exit applies them if the fd was a tty at startup and still refers to the same device. there's no check for whether node changed its terminal mode:

node/src/node.cc

Lines 688 to 722 in 019e869

    
           bool is_same_file = 
        
               (s.stat.st_dev == tmp.st_dev && s.stat.st_ino == tmp.st_ino); 
        
           if (!is_same_file) continue;  // Program reopened file descriptor. 
        
           int flags; 
        
           do 
        
             flags = fcntl(fd, F_GETFL); 
        
           while (flags == -1 && errno == EINTR);  // NOLINT 
        
           CHECK_NE(flags, -1); 
        
           // Restore the O_NONBLOCK flag if it changed. 
        
           if (O_NONBLOCK & (flags ^ s.flags)) { 
        
             flags &= ~O_NONBLOCK; 
        
             flags |= s.flags & O_NONBLOCK; 
        
             int err; 
        
             do 
        
               err = fcntl(fd, F_SETFL, flags); 
        
             while (err == -1 && errno == EINTR);  // NOLINT 
        
             CHECK_NE(err, -1); 
        
           } 
        
           if (s.isatty) { 
        
             sigset_t sa; 
        
             int err; 
        
             // We might be a background job that doesn't own the TTY so block SIGTTOU 
        
             // before making the tcsetattr() call, otherwise that signal suspends us. 
        
             sigemptyset(&sa); 
        
             sigaddset(&sa, SIGTTOU); 
        
             CHECK_EQ(0, pthread_sigmask(SIG_BLOCK, &sa, nullptr)); 
        
             do 
        
               err = tcsetattr(fd, TCSANOW, &s.termios); 
        
             while (err == -1 && errno == EINTR);  // NOLINT

in those runs, stdin and stderr were still ttys, so cleanup applied the saved settings through them. with the opt-out, neither example made any tcsetattr() calls.

Again, as far as I know these are two different things.

yes, they're separate. i mentioned both because the original restoration change covered both. the pr preserves O_NONBLOCK restoration; my question was about the additional terminal settings restoration beyond uv_tty_reset_mode().

the built-in terminal repl does, through readline. it needs to handle editing and completion keys before enter. applications can also request raw mode through readline or process.stdin.setRawMode(true):

Right. So applications which do process.stdin.setRawMode(true) are presumably also responsible to restore the state on exit - on their own and without node doing it for them. The docs don't guarantee that node restores the terminal state on exit (except signals etc), so it's the application responsibility, yes?

I'm guessing that it's similar with the REPL and readline - they set raw mode on their own at some stage, and restore the original mode on exit? Or at least they should be doing that? (restoring on their own)

So correct me if I'm wrong, but in both these cases there's actually no need for node to restore the terminal state (raw mode) on exit, because it's the responsibility of the application, right?

for node -e 'console.log(123)' | cat, it doesn't enable raw mode. i checked the source and traced tcsetattr() calls on my local macos build. node -h | cat behaved the same way. it was obvious though.

I can't say that it was obvious to me, but it's definitely good to get a confirmation that the original test case of js-beautify FILE | less was probably analyzed correctly, and indeed in such cases, on init node reads the console state - but doesn't modify it.

Definitely a good start.

startup saves the terminal settings, and exit applies them if the fd was a tty at startup and still refers to the same device. there's no check for whether node changed its terminal mode:

Right.

I'm looking at the top of this function, but I cant say I understand what this loop iterates on, and what's the value of fd in each iteration:

  for (auto& s : stdio) {
    const int fd = &s - stdio;

I'd appreciate your help in parsing/understanding the fd value in each iteration.

in those runs, stdin and stderr were still ttys, so cleanup applied the saved settings through them.

So the loop iterations are on fd 0/1/2 (stdin/stdout/stderr) (can it go over other fd's too?), and for stdin/stderr it was detected as tty (initially and on exit), and therefore restore their original attributes via tcsetattr on exit?

We already know what's there to restore for stdin - raw mode (although we don't yet know whether it's actually node's responsibility to do that, but hopefully we can clarify this through this discussion).

Do we know what's there to restore for stdout/stderr? I don't think you mentioned a reason that they might need to be restored.

with the opt-out, neither example made any tcsetattr() calls.

What's this opt-out? PR #66540 ?

If yes, it's good to know that it addresses this issue, and if we can't automate it, then it looks like we have a good way to control it manually.

If my understanding above is correct, and assuming we didn't identify a reason to restore stdout/stderr, then I think, ideally, we could reach these conclusions:

We didn't yet identify cases where node sets raw mode on its own.
Node is not responsible to restore stdin raw mode for applications which enable raw mode on their own - the application is responsible to restore it (that includes the REPL).
There's no good reason to restore stdout/stderr on exit.
Therefore, node should not restore the terminal state on exit for any of stdin/stdout/stderr (maybe except signals and other special cases - but not generally).

How does that sound?

(regardless, I really appreciate your help with references to other issues and to existing code. I definitely couldn't contribute otherwise due to my general lack of involvement with node, so thank you).

thanks, i'm new to node's internals too, so i'm taking some time to check the source and reproduce these cases before replying.

on the loop, stdio is a fixed array declared as stdio[1 + STDERR_FILENO]. STDERR_FILENO is 2, so it contains three records: the saved state for stdin, stdout and stderr.

auto& s makes s a reference to the current array element. &s is therefore that element's address, and stdio in the subtraction becomes the address of the first element. subtracting those pointers gives the element's index: 0, 1, then 2.

the loop's opening is equivalent to:

for (int i = 0; i < 3; ++i) {
  auto& s = stdio[i];
  const int fd = i;
  // ...
}

it cannot iterate over other fds. the separate uv_tty_reset_mode() call before it can use a different fd saved internally by libuv.

array declaration / cleanup loop

one small clarification about the tty detection: s.isatty is recorded at startup. exit doesn't perform a fresh tty check; it checks that fstat() still matches the saved device/inode, then uses the recorded s.isatty. neither check tells it whether node changed the terminal settings.

Do we know what's there to restore for stdout/stderr?

the settings belong to the terminal device, so stdin and stderr don't have separate raw-mode settings when they refer to the same tty. restoring through fd 2 can restore the same terminal used through fd 0.

i haven't found a termios change required just for ordinary stdout/stderr output. their inclusion in this loop is part of the broad startup-state restore, rather than evidence that writing output changed their terminal mode. that explains what gets restored through them, but doesn't establish that the unconditional restore is necessary.

What's this opt-out?

yes, --no-restore-terminal-state in #66540.

it skips only node's restoration of the startup termios snapshot. libuv's raw-mode cleanup and restoration of O_NONBLOCK remain enabled. that's why there were no tcsetattr() calls in those console/help examples with the flag: they never enabled raw mode, so there was no libuv raw-mode state to restore either.

on readline/repl, a terminal readline interface enables raw mode, and close() calls setRawMode(false). that covers its normal close path. process.exit() can bypass that path, though, and node already has a test expecting terminal cleanup after setRawMode(true); process.exit(0) without an explicit reset by the application.

readline close / existing process.exit test

i didn't find a general normal-exit guarantee in the setRawMode() api docs, but the restoration behavior was announced in the v12.5.0 release notes. so i'm cautious about concluding that cleanup is entirely the application's responsibility. that doesn't prove the full startup snapshot is needed for these cases: in my single-tty macos probes, libuv still restored the settings with the opt-out enabled.

i also found a concrete historical reason for the broader fallback: ben described a yubikey openssl engine leaving the terminal in a bad state when interrupted during its passphrase prompt:
#41143 (comment)

i checked that mechanism locally with a small native addon that disables echo directly through tcsetattr(), bypassing libuv, and registers its own atexit() cleanup. its cleanup worked on normal exit. with an externally sent SIGINT, the default node restoration restored echo, while the opt-out left echo disabled. this wasn't a reproduction of the actual yubikey engine, and it doesn't also contradict your proposal.

one further detail from the probes: after setRawMode(false) or readline close, libuv can still retain its saved reset state. a later change by another process was overwritten on exit even with the opt-out. the flag therefore addresses the console/help case, but doesn't resolve every terminal-state ownership case.
libuv's saved reset state

How does that sound?

i agree that the console/help cases shouldn't overwrite another application's terminal settings. i haven't established that all normal-exit startup restoration can safely be removed, though. the remaining compatibility question seems to be whether node should continue providing a fallback for native changes on ordinary exit, or explicitly leave those to applications while retaining the signal cleanup.

i didn't find a general normal-exit guarantee in the setRawMode() api docs, but the restoration behavior was announced in the v12.5.0 release notes. so i'm cautious about concluding that cleanup is entirely the application's responsibility.

This announce mentions PR #24260, which claims 2 fixes:

nodejs sometimes leaves stdout/stderr in non-blocking mode #14752 about the non-blocking mode, with an example similar to what I gave above (with the "Resource temporarily unavailable" error). I agree that this should have been fixed, but the O_NONBLOCK is a property of the file description, not of the file descriptor. So, if there is a way for the application to do a file redirection, the restoration from Node (instead of the application) may be wrong because it could restore the wrong file description. I don't know whether a file redirection is currently possible, but it might be explicitly the case in the future (Missing fs(/promises) APIs: dup, dup2, fdopen #41733 if reconsidered).
node 10.2.0+ turning off stty echo when using process.stdin.setRawMode() #21020 about the terminal raw mode. But this should not be done unconditionally, as seen above.

i also found a concrete historical reason for the broader fallback: ben described a yubikey openssl engine leaving the terminal in a bad state when interrupted during its passphrase prompt: #41143 (comment)

I don't understand this comment. There isn't an example. In particular, setting the terminal in raw mode disables interrupt, quit, and suspend special characters (see the stty(1) man page and the effect of stty -isig), so Ctrl-C on a passphrase prompt should not trigger a SIGINT; if the yubikey openssl engine just disables echo but not the interrupt, quit, and suspend special characters, this may be a bug there.

And an application should be aware if the terminal can be used. It could be a bug in the yubikey openssl engine (possibly a bad API design), which Node should not try to work around if it has side effects in other contexts (as seen in this bug report).

Related to this bug, see #35536.

@fatihguzeldev

@fatihguzeldev thanks again for the research and detailed reply.

I asked the people who reported/confirmed the raw mode as a bug, and the author which "fixed" it, to join this discussion and hopefully help us with some answers.

Do we know what's there to restore for stdout/stderr?

the settings belong to the terminal device, so stdin and stderr don't have separate raw-mode settings when they refer to the same tty.

Sure.

restoring through fd 2 can restore the same terminal used through fd 0.

Sounds reasonable, but that's orthogonal to the question.

Because if we conclude that there's no need to restore stdout/stderr, then we only need to deal with stdin and raw mode - where we do know that there may be something to restore (raw mode), but not sure who should be responsible to do that.

The release notes of v12.5 also mention only the reasons of raw mode (which we know of, but currently I don't think it was a node bug) and O_NONBLOCK (which is orthogonal to this issue and doesn't affect it), but not stdout/stderr.

So let's hope that @bnoordhuis can help with these questions and possible solutions, as the author who added this reset in #24260.

@vinc17fr

@vinc17fr thanks for the link. i read #35536, including ben's reply about compatibility. that's why i'm hesitant to conclude that the fallback can simply go away. the release notes alone don't justify the unconditional restore.

on O_NONBLOCK, agreed. node checks device/inode, but that doesn't tell us whether it's the original open file description. reopening the same file/tty could pass that check. is that one of the cases you mean?

node/src/node.cc

Lines 682 to 707 in 019e869

    
           struct stat tmp; 
        
           if (-1 == fstat(fd, &tmp)) { 
        
             CHECK_EQ(errno, EBADF);  // Program closed file descriptor. 
        
             continue; 
        
           } 
        
           bool is_same_file = 
        
               (s.stat.st_dev == tmp.st_dev && s.stat.st_ino == tmp.st_ino); 
        
           if (!is_same_file) continue;  // Program reopened file descriptor. 
        
           int flags; 
        
           do 
        
             flags = fcntl(fd, F_GETFL); 
        
           while (flags == -1 && errno == EINTR);  // NOLINT 
        
           CHECK_NE(flags, -1); 
        
           // Restore the O_NONBLOCK flag if it changed. 
        
           if (O_NONBLOCK & (flags ^ s.flags)) { 
        
             flags &= ~O_NONBLOCK; 
        
             flags |= s.flags & O_NONBLOCK; 
        
             int err; 
        
             do 
        
               err = fcntl(fd, F_SETFL, flags); 
        
             while (err == -1 && errno == EINTR);  // NOLINT 
        
             CHECK_NE(err, -1);

i checked #41733 too. those APIs haven't landed, though native code can already replace fds.

There isn't an example.

i don't have a yubikey reproducer. that reference was historical context; my addon test was separate. it only disabled ECHO, and its atexit cleanup worked on normal exit. with an externally sent SIGINT, node restored echo by default, but left it disabled with the opt-out.

so it wasn't full raw mode. ISIG stayed enabled, which lets a password prompt hide input while still allowing Ctrl-C. the current OpenSSL prompt code does this too:

node/deps/openssl/openssl/crypto/ui/ui_openssl.c

Lines 483 to 492 in 019e869

    
           static int noecho_console(UI *ui) 
        
           { 
        
           #ifdef TTY_FLAGS 
        
               memcpy(&(tty_new), &(tty_orig), sizeof(tty_orig)); 
        
               tty_new.TTY_FLAGS &= ~ECHO; 
        
           #endif 
        
           #if defined(TTY_set) && !defined(OPENSSL_SYS_VMS) 
        
               if (is_a_tty && (TTY_set(fileno(tty_in), &tty_new) == -1)) 
        
                   return 0;

are you saying those settings could be wrong for the prompt, or that failing to restore them on interruption is the bug? we don't know what the old yubikey engine actually did.

i agree that none of this establishes a need to restore every tty stdio fd unconditionally. i'm still unsure who should handle changes made by a native library inside the node process: the library itself, or node?

would your answer differ between normal exit and node's default SIGINT/SIGTERM exit? the docs say those handlers reset terminal mode, without spelling out which changes that covers:

https://nodejs.org/api/process.html#signal-events

a library may have a cleanup bug, but an application relying on node's fallback could start leaving the terminal in a bad state after a node update. that's the compatibility concern ben raised here:

#35536 (comment)

i agree that the current behavior causes harm too. we need to know which cleanup should remain before deciding how to remove the unwanted resets.

i'm still unsure who should handle changes made by a native library inside the node process: the library itself, or node?

Ultimately this is under node's control, which is delegated by the application to do what the application needs.

If node used the library to change the state, and asssuming node needs it restored later, then it's node's job to restore it later.

If it's a bug at the library (or an application - liky ubikey) that it doesn't restore the state, then IMO node should not cover it up by restoring it anyway, because this also has other consequences, like breaking our stty.js and node -h | less.

Instead, it should be reported to the broken library/module/application.

In general, node is a framework to build applications. The control of the behavior is ultimately at the application, including cleanups in various cases which the application is responsible to. node is a middle man which facilitates the application and let it use some underlaying system interfaces.

I think node should be as least opinionated as possible when it comes to "fixing" buggy applications, because this would necessarily have consequences - like preventing applications doing intentionally what some buggy application did accidentally, like with the case of stty.js.

on O_NONBLOCK, agreed. node checks device/inode, but that doesn't tell us whether it's the original open file description. reopening the same file/tty could pass that check. is that one of the cases you mean?

Yes (the fact that node checks device/inode limits the issue, but not when the same file is opened again).

node/deps/openssl/openssl/crypto/ui/ui_openssl.c

Lines 483 to 492 in 019e869

static int noecho_console(UI *ui)

{

#ifdef TTY_FLAGS

memcpy(&(tty_new), &(tty_orig), sizeof(tty_orig));

tty_new.TTY_FLAGS &= ~ECHO;

#endif

#if defined(TTY_set) && !defined(OPENSSL_SYS_VMS)

if (is_a_tty && (TTY_set(fileno(tty_in), &tty_new) == -1))

return 0;

are you saying those settings could be wrong for the prompt, or that failing to restore them on interruption is the bug?

Yes, if a library call (e.g. from libcrypto) modifies the terminal state but the process can be interrupted by Ctrl-C, this is an issue, at least if this is not documented (so that the caller is not aware of it).

i agree that none of this establishes a need to restore every tty stdio fd unconditionally. i'm still unsure who should handle changes made by a native library inside the node process: the library itself, or node?

I suppose that trapping the main signals (SIGINT, SIGQUIT, SIGHUP, SIGTERM) is needed. Is this only under node's control or controlled by the application? If only under node's control, then there should be a flag that would be set/reset by the application to tell node whether it should restore the terminal state.

would your answer differ between normal exit and node's default SIGINT/SIGTERM exit?

I would say that the application should be responsible for restoring the terminal state, possibly except if a flag is used to tell node to do that (see above).

the docs say those handlers reset terminal mode, without spelling out which changes that covers:

https://nodejs.org/api/process.html#signal-events

The terminal state should not be "restored" unconditionally, because there are cases where node is not in control of the terminal (which is this bug about less). Hence the idea of a flag set by the application. Note: the application would also be responsible for resetting this flag when it no longer needs the terminal (e.g. if the terminal can now be used by another process, such as less).

a library may have a cleanup bug, but an application relying on node's fallback could start leaving the terminal in a bad state after a node update. that's the compatibility concern ben raised here:

#35536 (comment)

I think that the main issue was about O_NONBLOCK (set by console.log, probably used by many users). I'm just suggesting to improve the behavior concerning the terminal state for now.

@avih

@avih @vinc17fr i'd propose changing the ordinary-exit default on POSIX: stop restoring the startup termios snapshot, while preserving the existing cleanup for raw mode enabled through node's APIs.

more specifically:

ordinary exit means natural completion and process.exit();
those paths would no longer replay the startup terminal settings;
raw-mode exit cleanup would remain;
O_NONBLOCK restoration would remain;
the default SIGINT/SIGTERM handlers and other forced-exit cleanup would stay unchanged initially.

keeping raw-mode exit cleanup is a deliberate choice here. there are already tests expecting setRawMode(true) to be cleaned up on natural exit and process.exit(). i'd make that behavior explicit in the contract rather than withdraw it together with the broader fallback.

native changes outside those APIs would be the application's responsibility on ordinary exit. the intended results would be:

case	ordinary-exit behavior
the console/help pipeline cases, without raw-mode use	leave the pager's terminal settings alone
a `stty.js` tool deliberately changing settings through native code or a child process	leave those changes in place
an application using node's raw-mode APIs	retain its raw-mode exit cleanup
a native library without its own cleanup	no automatic startup-snapshot fallback

@vinc17fr applications can install signal listeners on POSIX through process.on(), including for SIGINT, SIGTERM, SIGHUP and SIGQUIT. listeners for SIGINT/SIGTERM replace the documented default handlers. JS signal callbacks are asynchronous, though, so installing one doesn't by itself guarantee cleanup while a synchronous native call is blocking the event loop. native libraries can also install their own native handlers; that part depends on the library.

https://nodejs.org/api/process.html#signal-events

i'm reading your flag proposal as an application-controlled restoration request that can be set and cleared during execution, rather than a fixed CLI option. that gives us something more specific than checking whether a tty was touched.

for node's raw-mode APIs, enabling raw mode could establish that request, and disabling it could end it. a native caller that wants node to handle cleanup could explicitly request it before changing the terminal. i'd tie the request and saved settings to a particular tty, capturing the settings before that operation rather than using the process-startup snapshot.

i haven't worked out the API or shared-handle bookkeeping yet. but that would let a caller ask for cleanup without node having to guess whether an arbitrary native change was accidental or intentional.

on the password prompt: the caller needs to know which settings the library changes and what cleanup is expected if it is interrupted. i wasn't treating ECHO-off/ISIG-on alone as proof of a bug. we still don't have enough information about the old yubikey case to make that claim.

i'd also keep the O_NONBLOCK concern separate. the same-file reopen case you confirmed is still worth addressing, but this proposal wouldn't change that restoration.

i read ben's compatibility concern as covering terminal settings too, not only O_NONBLOCK. he specifically discusses making tcsetattr() conditional on prior raw-mode use, and says that not all tty changes are visible to node:

#35536 (comment)

so preserving O_NONBLOCK doesn't establish that withdrawing the terminal fallback is harmless. an application relying on it could start leaving the terminal in a bad state after an update. i'd treat that as a major-version behavior change, with a way to explicitly request the old restoration behavior during a transition.

there is also work needed to preserve raw-mode cleanup. simply removing the startup restore and keeping uv_tty_reset_mode() isn't sufficient: libuv has one global reset snapshot, and that snapshot can survive setRawMode(false). we need cleanup for separate stdio ttys without replaying stale settings after raw mode has already been disabled.

node/deps/uv/src/unix/tty.c

Lines 296 to 312 in 019e869

    
           if (tty->mode == UV_TTY_MODE_NORMAL && mode != UV_TTY_MODE_NORMAL) { 
        
             do 
        
               rc = tcgetattr(fd, &tty->orig_termios); 
        
             while (rc == -1 && errno == EINTR); 
        
             if (rc == -1) 
        
               return UV__ERR(errno); 
        
             /* This is used for uv_tty_reset_mode() */ 
        
             do 
        
               expected = 0; 
        
             while (!atomic_compare_exchange_strong(&termios_spinlock, &expected, 1)); 
        
             if (orig_termios_fd == -1) { 
        
               orig_termios = tty->orig_termios; 
        
               orig_termios_fd = fd; 
        
             }

retaining the current default signal cleanup initially would be a compatibility decision. the objection to unconditional restoration still applies there when another process is using the terminal. the docs promise terminal reset for SIGINT/SIGTERM; they don't specify that it must always replay the full startup snapshot.

would you consider using node's raw-mode API sufficient to request exit cleanup, or should that require a separate application-controlled setting too?

a stty.js tool deliberately changing settings through native code or a child process leave those changes in place
an application using node's raw-mode APIs retain its raw-mode exit cleanup

How can you distinguish between these cases? and in general whether some behavior X should be considered a bug (of the application) or not?

Afterall, "bug" does not describe a behavior. Identical behavior X can be considered a bug in some cases, and correct behavior in others.

What makes X a bug is whether or not it behaves as documented and as intended, and preferably those are logical and reasonable.

rm.js deleting some file is highly likely documented and intended behavior, and not a bug.

But node -h deleting some file is highly likely a bug.

Similarly stty.js intentionally and as documented, changes the terminal state according to the user's instructions, so it's not a bug.

Ubikey does not intend to leave the terminal in raw mode, so it is a bug.

How can node know when to choose to restore it, and when not to restore it?

Navigation Menu

Repository navigation

Problem with newer `less` #66440

14 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	emitKeypressEvents ??= require('internal/readline/emitKeypressEvents');
	emitKeypressEvents(input, this);

	// `input` usually refers to stdin
	input.on('keypress', onkeypress);
	input.on('end', ontermend);

	this[kSetRawMode](true);

	bool is_same_file =
	(s.stat.st_dev == tmp.st_dev && s.stat.st_ino == tmp.st_ino);
	if (!is_same_file) continue; // Program reopened file descriptor.

	int flags;
	do
	flags = fcntl(fd, F_GETFL);
	while (flags == -1 && errno == EINTR); // NOLINT
	CHECK_NE(flags, -1);

	// Restore the O_NONBLOCK flag if it changed.
	if (O_NONBLOCK & (flags ^ s.flags)) {
	flags &= ~O_NONBLOCK;
	flags \|= s.flags & O_NONBLOCK;

	int err;
	do
	err = fcntl(fd, F_SETFL, flags);
	while (err == -1 && errno == EINTR); // NOLINT
	CHECK_NE(err, -1);
	}

	if (s.isatty) {
	sigset_t sa;
	int err;

	// We might be a background job that doesn't own the TTY so block SIGTTOU
	// before making the tcsetattr() call, otherwise that signal suspends us.
	sigemptyset(&sa);
	sigaddset(&sa, SIGTTOU);

	CHECK_EQ(0, pthread_sigmask(SIG_BLOCK, &sa, nullptr));
	do
	err = tcsetattr(fd, TCSANOW, &s.termios);
	while (err == -1 && errno == EINTR); // NOLINT

	struct stat tmp;
	if (-1 == fstat(fd, &tmp)) {
	CHECK_EQ(errno, EBADF); // Program closed file descriptor.
	continue;
	}

	bool is_same_file =
	(s.stat.st_dev == tmp.st_dev && s.stat.st_ino == tmp.st_ino);
	if (!is_same_file) continue; // Program reopened file descriptor.

	int flags;
	do
	flags = fcntl(fd, F_GETFL);
	while (flags == -1 && errno == EINTR); // NOLINT
	CHECK_NE(flags, -1);

	// Restore the O_NONBLOCK flag if it changed.
	if (O_NONBLOCK & (flags ^ s.flags)) {
	flags &= ~O_NONBLOCK;
	flags \|= s.flags & O_NONBLOCK;

	int err;
	do
	err = fcntl(fd, F_SETFL, flags);
	while (err == -1 && errno == EINTR); // NOLINT
	CHECK_NE(err, -1);

	static int noecho_console(UI *ui)
	{
	#ifdef TTY_FLAGS
	memcpy(&(tty_new), &(tty_orig), sizeof(tty_orig));
	tty_new.TTY_FLAGS &= ~ECHO;
	#endif

	#if defined(TTY_set) && !defined(OPENSSL_SYS_VMS)
	if (is_a_tty && (TTY_set(fileno(tty_in), &tty_new) == -1))
	return 0;

	if (tty->mode == UV_TTY_MODE_NORMAL && mode != UV_TTY_MODE_NORMAL) {
	do
	rc = tcgetattr(fd, &tty->orig_termios);
	while (rc == -1 && errno == EINTR);

	if (rc == -1)
	return UV__ERR(errno);

	/* This is used for uv_tty_reset_mode() */
	do
	expected = 0;
	while (!atomic_compare_exchange_strong(&termios_spinlock, &expected, 1));

	if (orig_termios_fd == -1) {
	orig_termios = tty->orig_termios;
	orig_termios_fd = fd;
	}

Problem with newer less #66440

Description

Activity

MikeMcC399 commented on Oct 1, 2026

avih commented on Oct 1, 2026

avivkeller commented on Oct 2, 2026

avih commented on Oct 2, 2026

avivkeller commented on Oct 2, 2026

avih commented on Oct 2, 2026

vinc17fr commented on Oct 5, 2026

avih commented on Oct 5, 2026

vinc17fr commented on Oct 5, 2026

14 remaining items

avih commented on Oct 6, 2026

vinc17fr commented on Oct 6, 2026

avih commented on Oct 6, 2026

vinc17fr commented on Oct 6, 2026

fatihguzeldev commented on Oct 7, 2026

avih commented on Oct 7, 2026

fatihguzeldev commented on Oct 8, 2026

vinc17fr commented on Oct 8, 2026

vinc17fr commented on Oct 8, 2026

avih commented on Oct 8, 2026

fatihguzeldev commented on Oct 8, 2026

avih commented on Oct 8, 2026

vinc17fr commented on Oct 8, 2026

fatihguzeldev commented on Oct 8, 2026

avih commented on Oct 8, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Problem with newer `less` #66440