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

src: restore stdio on program exit #24260

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
10 participants
@bnoordhuis
Copy link
Member

commented Nov 8, 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.

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

@bnoordhuis bnoordhuis requested a review from evanlucas Nov 8, 2018

@@ -1657,14 +1658,14 @@ void SetupProcessObject(Environment* env,


void SignalExit(int signo) {
uv_tty_reset_mode();

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Nov 8, 2018

Author Member

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.

Show resolved Hide resolved src/node.cc
Show resolved Hide resolved src/node.cc
Show resolved Hide resolved src/node.cc

@refack refack added the cli label Nov 8, 2018

@cjihrig

cjihrig approved these changes Nov 9, 2018

@refack
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Member

commented Nov 11, 2018

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 bnoordhuis force-pushed the bnoordhuis:fix14752-again branch from fe35f69 to 7d3da7d Nov 12, 2018

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2018

@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

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

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

refack approved these changes Nov 16, 2018

Copy link
Member

left a comment

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

@refack

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

@refack @bnoordhuis The issue is at #21213cat <(./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…

@addaleax addaleax removed the author ready label Nov 16, 2018

@refack

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

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

src: restore stdio on program exit
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

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

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

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

This comment has been minimized.

@addaleax
Copy link
Member

left a comment

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

Trott added a commit to Trott/io.js that referenced this pull request Jun 13, 2019

src: restore stdio on program exit
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

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Landed in 5872705

@Trott Trott closed this Jun 13, 2019

BridgeAR added a commit that referenced this pull request Jun 17, 2019

src: restore stdio on program exit
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>

@BridgeAR BridgeAR referenced this pull request Jun 17, 2019

Merged

v12.5.0 proposal #28268

BridgeAR added a commit that referenced this pull request Jun 26, 2019

2019-06-18, Version 12.5.0 (Current)
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

BridgeAR added a commit that referenced this pull request Jun 26, 2019

2019-06-18, Version 12.5.0 (Current)
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

BridgeAR added a commit that referenced this pull request Jun 26, 2019

2019-06-26, Version 12.5.0 (Current)
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

BridgeAR added a commit that referenced this pull request Jun 27, 2019

2019-06-27, Version 12.5.0 (Current)
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

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 27, 2019

2019-06-27, Version 12.5.0 (Current)
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

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2019

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

@ivan ivan referenced this pull request Jun 29, 2019

Merged

nodejs-12_x: 12.4.0 -> 12.5.0 #63883

3 of 10 tasks complete
@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

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

@devsnek

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Future releasers: please do not backport this without #28490

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.