src: restore stdio on program exit by bnoordhuis · Pull Request #24260 · nodejs/node

bnoordhuis · 2018-11-08T20:52:14Z

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

nodejs-github-bot · 2018-11-08T20:52:17Z

@bnoordhuis build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1547/pipeline

bnoordhuis · 2018-11-08T20:55:08Z

src/node.cc

It was arguably a bug to call uv_tty_reset_mode() first because the freebsd workaround right below it needs to happen as quickly as possible.

src/node.cc

refack

Missing the state capture logic.
This has been reverted in the past so this needs regression tests.
The code does not follow the CPP style guide.

refack · 2018-11-11T00:55:52Z

I suggest that to solve some of the CPP guidelines issue, the state capture code should be is the structs constructor, which could take an int as input. That way we could have:

static const StdioState stdio[] = {0,1,2};

bnoordhuis · 2018-11-12T11:00:11Z

@refack Good catch. Don't know what went wrong but the commit was missing a chunk. Added now in a squash commit for easier reviewing.

the state capture code should be is the structs constructor

Not a good idea. Constructors run at an ill-defined time. This code should run at a specific time.

@addaleax @evanlucas Do you remember how to trigger the regression? I've actually been unable to find any change in behavior that wasn't a bug; i.e., that wasn't about leaving stdio in non-blocking mode.

refack · 2018-11-12T14:13:47Z

the state capture code should be is the structs constructor

Not a good idea. Constructors run at an ill-defined time. This code should run at a specific time.

Ack.

refack

If no one remembers what to test, let's land this and see 🤷‍♂️

refack · 2018-11-16T11:17:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18671/

addaleax · 2018-11-16T11:22:10Z

@refack @bnoordhuis The issue is at #21213 – cat <(./node -v) would start crashing again for me with this patch applied.

I remember having a very hard time coming up with a regression test for this…

refack · 2018-11-16T14:05:51Z

Can you run cat <(./node -v) in a detached shell process and get the same effects? (I guess you'll have to poll it by PID to get it's status...)

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

bnoordhuis · 2019-05-24T14:38:38Z

Rebased, PTAL. I can't reproduce cat <(./node -v) crashes on either Mac or Linux, by the way. I'm assuming enough changed in the code base that whatever was causing it has been excised.

nodejs-github-bot · 2019-05-24T14:39:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23305/

addaleax

Feels like a lot of the code here could end up in libuv in one way or another? :)

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>

Trott · 2019-06-13T03:47:38Z

Landed in 5872705

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>

Notable changes: This release contains `semver-major` commits. These are in fact not `semver-major` due to follow-up commits that remove all breaking changes. * build: * The startup time is reduced by enabling V8 snapshots by default #28181 * deps: * Updated `V8` to 7.5.288.22 #27375 * The numeric separator (v8.dev/features/numeric-separators) feature is now enabled by default * Updated `OpenSSL` to 1.1.1c #28211 * inspector: * The `--inspect-publish-uid` flag was added to specify ways of the inspector web socket url exposure #27741 * n-api: * Accessors on napi_define_* are now ECMAScript-compliant #27851 * report: * The cpu info got added to the report output #28188 * src: * Restore the original state of the stdio file descriptors on exit to prevent leaving stdio in raw or non-blocking mode #24260 * tools,gyp: * Introduce MSVS 2019 #27375 * util: * inspect: * Array grouping became more compact and uses more columns than before #28059 #28070 * Long strings will not be split at 80 characters anymore. Instead they will be split on new lines #28055 * worker: * `worker.terminate()` now returns a promise and using the callback is deprecated #28021 PR-URL: #28268

Notable changes: * build: * The startup time is reduced by enabling V8 snapshots by default #28181 * deps: * Updated `V8` to 7.5.288.22 #27375 * The numeric separator (v8.dev/features/numeric-separators) feature is now enabled by default * Updated `OpenSSL` to 1.1.1c #28211 * inspector: * The `--inspect-publish-uid` flag was added to specify ways of the inspector web socket url exposure #27741 * n-api: * Accessors on napi_define_* are now ECMAScript-compliant #27851 * report: * The cpu info got added to the report output #28188 * src: * Restore the original state of the stdio file descriptors on exit to prevent leaving stdio in raw or non-blocking mode #24260 * tools,gyp: * Introduce MSVS 2019 #27375 * util: * inspect: * Array grouping became more compact and uses more columns than before #28059 #28070 * Long strings will not be split at 80 characters anymore. Instead they will be split on new lines #28055 * worker: * `worker.terminate()` now returns a promise and using the callback is deprecated #28021 PR-URL: #28268

Notable changes: * build: * The startup time is reduced by enabling V8 snapshots by default nodejs#28181 * deps: * Updated `V8` to 7.5.288.22 nodejs#27375 * The numeric separator (v8.dev/features/numeric-separators) feature is now enabled by default * Updated `OpenSSL` to 1.1.1c nodejs#28211 * inspector: * The `--inspect-publish-uid` flag was added to specify ways of the inspector web socket url exposure nodejs#27741 * n-api: * Accessors on napi_define_* are now ECMAScript-compliant nodejs#27851 * report: * The cpu info got added to the report output nodejs#28188 * src: * Restore the original state of the stdio file descriptors on exit to prevent leaving stdio in raw or non-blocking mode nodejs#24260 * tools,gyp: * Introduce MSVS 2019 nodejs#27375 * util: * inspect: * Array grouping became more compact and uses more columns than before nodejs#28059 nodejs#28070 * Long strings will not be split at 80 characters anymore. Instead they will be split on new lines nodejs#28055 * worker: * `worker.terminate()` now returns a promise and using the callback is deprecated nodejs#28021 PR-URL: nodejs#28268

ivan · 2019-06-29T03:25:00Z

This particular commit is causing my program to reliably hang on exit in some cases. I confirmed that it works fine with 12.4.0, and 12.5.0 with this commit reverted. I haven't narrowed this down to a repro case (sorry), but what I see when I strace it is:

# strace -f -p 26209
strace: Process 26209 attached with 11 threads
[pid 26237] --- stopped by SIGTTOU ---
[pid 26236] --- stopped by SIGTTOU ---
[pid 26235] --- stopped by SIGTTOU ---
[pid 26234] --- stopped by SIGTTOU ---
[pid 26215] --- stopped by SIGTTOU ---
[pid 26214] --- stopped by SIGTTOU ---
[pid 26213] --- stopped by SIGTTOU ---
[pid 26212] --- stopped by SIGTTOU ---
[pid 26211] --- stopped by SIGTTOU ---
[pid 26210] --- stopped by SIGTTOU ---
[pid 26209] --- stopped by SIGTTOU ---

Said node program is being launched by a Python 3.7 process with

subprocess.check_call(["timeout", "12h", "ts", "add-shoo", "--rm", "-c", "-d"] + files, stderr=subprocess.STDOUT)

The SIGTTOU hang on exit occurs when it is run in a tmux, but not when run over ssh without a tty.

update: 12.6.0 is still broken

BridgeAR · 2019-06-29T15:40:23Z

@ivan would you be so kind and open a new issue with your report? Thanks!

devsnek · 2019-07-02T00:52:04Z

Future releasers: please do not backport this without #28490

arekm · 2020-01-25T23:53:56Z

10.x (tested with 10.18.1) didn't get this backport and thus is buggy

bnoordhuis requested a review from evanlucas November 8, 2018 20:52

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 8, 2018

bnoordhuis commented Nov 8, 2018

View reviewed changes

refack reviewed Nov 8, 2018

View reviewed changes

src/node.cc Outdated Show resolved Hide resolved

src/node.cc Outdated Show resolved Hide resolved

src/node.cc Outdated Show resolved Hide resolved

refack added the cli Issues and PRs related to the Node.js command line interface. label Nov 8, 2018

cjihrig approved these changes Nov 9, 2018

View reviewed changes

jasnell approved these changes Nov 10, 2018

View reviewed changes

refack suggested changes Nov 11, 2018

View reviewed changes

bnoordhuis force-pushed the fix14752-again branch from fe35f69 to 7d3da7d Compare November 12, 2018 10:54

refack approved these changes Nov 16, 2018

View reviewed changes

refack added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. baking-for-lts PRs that need to wait before landing in a LTS release. labels Nov 16, 2018

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 16, 2018

gireeshpunathil mentioned this pull request Nov 29, 2018

cat: write error: Resource temporarily unavailable #19838

Closed

bnoordhuis force-pushed the fix14752-again branch from 7d3da7d to 256614a Compare May 24, 2019 14:36

bnoordhuis added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 24, 2019

addaleax approved these changes May 26, 2019

View reviewed changes

Trott closed this Jun 13, 2019

BridgeAR mentioned this pull request Jun 17, 2019

v12.5.0 proposal #28268

Merged

ivan mentioned this pull request Jun 29, 2019

nodejs-12_x: 12.4.0 -> 12.5.0 NixOS/nixpkgs#63883

Merged

10 tasks

addaleax mentioned this pull request Jul 1, 2019

CHECK failure in ResetStdio on >=12.5.0 #28479

Closed

bnoordhuis mentioned this pull request Jul 4, 2019

cat <(node -v) never exits after >= 12.5.0 #28530

Closed

bnoordhuis mentioned this pull request Dec 12, 2019

Using node with process.stdin inside spawned shell #18446

Closed

andersk mentioned this pull request Oct 7, 2020

Background node process corrupts terminal state with tcsetattr() on exit #35536

Open

Enselic mentioned this pull request Dec 7, 2020

Keyboard arrow keys not working for scrolling from STDIN, rather shows keycode of the key pressed sharkdp/bat#1006

Open

targos removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Sep 16, 2022

Uh oh!

Conversation

bnoordhuis commented Nov 8, 2018

Uh oh!

nodejs-github-bot commented Nov 8, 2018

Uh oh!

bnoordhuis Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

refack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Nov 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Nov 12, 2018

Uh oh!

refack commented Nov 12, 2018

Uh oh!

refack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Nov 16, 2018

Uh oh!

addaleax commented Nov 16, 2018

Uh oh!

refack commented Nov 16, 2018

Uh oh!

bnoordhuis commented May 24, 2019

Uh oh!

nodejs-github-bot commented May 24, 2019

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Jun 13, 2019

Uh oh!

ivan commented Jun 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Jun 29, 2019

Uh oh!

devsnek commented Jul 2, 2019

Uh oh!

arekm commented Jan 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

refack left a comment •

edited

Loading

refack commented Nov 11, 2018 •

edited

Loading

refack left a comment •

edited

Loading

ivan commented Jun 29, 2019 •

edited

Loading