src: restore stdio on program exit #20592

ktaborski · 2018-05-08T08:12:18Z

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.

Author of change: @bnoordhuis

Continuation of #17737 as original pull request was closed.
Fixes: #14752

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

gireeshpunathil · 2018-05-08T11:20:36Z

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

gireeshpunathil · 2018-05-08T11:21:03Z

looks really promising, let us see how the CI responds.

gireeshpunathil · 2018-05-08T14:56:49Z

2 failures, one on freebsd:

07:35:12 not ok 2134 sequential/test-net-connect-local-error
07:35:12   ---
07:35:12   duration_ms: 0.234
07:35:12   severity: fail
07:35:12   exitcode: 1
07:35:12   stack: |-
07:35:12     assert.js:77
07:35:12       throw new AssertionError(obj);
07:35:12       ^
07:35:12     
07:35:12     AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
07:35:12     + expected - actual
07:35:12     
07:35:12     - undefined
07:35:12     + 12347
07:35:12         at Socket.onError (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/sequential/test-net-connect-local-error.js:18:10)
07:35:12         at Socket.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/common/index.js:474:15)
07:35:12         at Socket.emit (events.js:182:13)
07:35:12         at emitErrorNT (internal/streams/destroy.js:82:8)
07:35:12         at emitErrorAndCloseNT (internal/streams/destroy.js:50:3)
07:35:12         at process._tickCallback (internal/process/next_tick.js:63:19)
07:35:12   ...

and one on arm:

not ok 309 parallel/test-crypto-dh-leak
  ---
  duration_ms: 1.558
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:247
        throw err;
        ^
    
    AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
    
      assert(after - before < 5 << 20)
    
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64/test/parallel/test-crypto-dh-leak.js:26:1)
        at Module._compile (internal/modules/cjs/loader.js:678:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:689:10)
        at Module.load (internal/modules/cjs/loader.js:589:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:528:12)
        at Function.Module._load (internal/modules/cjs/loader.js:520:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:719:10)
        at startup (internal/bootstrap/node.js:229:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:577:3)

Does anyone know if this could be related to the PR?

Trott · 2018-05-08T16:37:44Z

Is it possible/worthwhile to write at least a minimal test for this behavior?

Trott · 2018-05-08T16:39:29Z

@gireeshpunathil The first error has been seen in CI before: #18658

The second one is new to me.

Let's see what happens if we re-run CI:
https://ci.nodejs.org/job/node-test-pull-request/14719/

gireeshpunathil · 2018-05-09T01:15:37Z

thanks @Trott - those 2 failures are vanished and new ones appear, so safe to assume these are unrelated flaky stuff.

@tabkrz - #18446 and #14752 have minimal test cases to reproduce the issue, please see if you can develop them into fully-blown tests.

A rough sketch would be:

node -> non-node -> node
After the granchild has exited, check for the fidelity of the non-node child
The non-tty scenario that you have identified.

Please refer to test/parallel/test-child-process-spawn* test cases for general structure, spawn logic, validation logic etc.

ktaborski · 2018-05-09T05:43:01Z

I will check.

addaleax

Does anyone know if this could be related to the PR?

My intuition would be “no”, but we should probably not land this PR without a fully green CI.

Next attempt: https://ci.nodejs.org/job/node-test-commit/18334/

gireeshpunathil · 2018-05-10T01:57:01Z

interesting to see every time we run a new CI, the failure count increases, linearly!

btw what is ${STATUS_LABEL} — tests failed ? I don't remember seeing this earlier.

Trott · 2018-05-11T02:39:44Z

Just took some problematic CI nodes offline. Let's try again...

CI: https://ci.nodejs.org/job/node-test-commit/18400/

gireeshpunathil · 2018-05-11T05:01:28Z

barring the unknown ${STATUS_LABEL} failure, the linux one is clearly infra:

13:54:49 make[1]: *** [Makefile:87: node] Terminated
13:54:49 make: *** [Makefile:465: build-ci] Terminated
13:54:49 FATAL: command execution failed
13:54:49 java.nio.channels.ClosedChannelException
13:54:49 	at org.jenkinsci.remoting.protocol.NetworkLayer.onRecvClosed(NetworkLayer.java:154)
13:54:49 	at org.jenkinsci.remoting.protocol.impl.NIONetworkLayer.ready(NIONetworkLayer.java:142)
13:54:49 	at org.jenkinsci.remoting.protocol.IOHub$OnReady.run(IOHub.java:789)
13:54:49 	at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
13:54:49 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
13:54:49 	at

bnoordhuis · 2018-05-11T08:59:15Z

@tabkrz Can you keep my name on the code I wrote? I don't get too hung up on authorship if it's just a few lines but this is #17737 almost verbatim.

ktaborski · 2018-05-11T19:13:02Z

@bnoordhuis, please check if current form of PR is sufficient to you.

bnoordhuis

@tabkrz Yes, it's fine now. Left some comments on the second commit.

bnoordhuis · 2018-05-11T19:30:41Z

src/node.cc

@@ -3313,7 +3313,6 @@ void SetupProcessObject(Environment* env,


 void SignalExit(int signo) {
-


Unrelated whitespace change.

bnoordhuis · 2018-05-11T19:31:39Z

src/node.cc

@@ -4202,17 +4201,19 @@ inline void PlatformInit() {

    s.flags = GetFileDescriptorFlags(fd);
    CHECK_NE(s.flags, -1);
+
+    if (isatty(fd)) {


You could just if (isatty(fd)) continue; - saves a level of indent and makes the diff less noisy.

I think, it should be if ( ! isatty(fd)) continue;

That's what I mean, of course. :-)

bnoordhuis · 2018-05-11T19:35:24Z

src/node.cc

+        // tcgetattr() is not supposed to return ENODEV or EOPNOTSUPP
+        // but it does so anyway on MacOS.
+        CHECK(errno == ENODEV || errno == ENOTTY || errno == EOPNOTSUPP);
+      }


This can probably be simplified to just this now:

s.isatty = true; do { err = tcgetattr(fd, &s.termios); } while (err == -1 && errno == EINTR); CHECK_EQ(err, 0);

gireeshpunathil · 2018-05-13T14:18:39Z

We never got a clean CI, at the same time the failures were inconclusive. So running once again:
CI: https://ci.nodejs.org/job/node-test-pull-request/14837/

gireeshpunathil · 2018-05-15T08:56:33Z

all clean.

BridgeAR · 2018-05-18T13:42:13Z

@bnoordhuis @tabkrz would it be fine for you to land this as a single commit that is done by @bnoordhuis and @tabkrz as a co-author?

ktaborski · 2018-05-19T08:01:03Z

Fine for me.

apapirovski · 2018-05-22T08:34:23Z

@tabkrz Could you update the commits to correctly have your github email and name associated with them? If we land these as is then you won't get proper credit for the work. Thanks!

(You can see our contributing guide for more info but basically it's this https://help.github.com/articles/setting-your-commit-email-address-in-git/)

ktaborski · 2018-05-22T08:52:36Z

@apapirovski, done.

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. Fixes: #14752

termios-flags can be stored only in TTY environment

jasnell · 2018-05-23T14:58:29Z

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

jasnell · 2018-05-23T18:38:08Z

Rerun CI on Windows: https://ci.nodejs.org/job/node-test-commit-windows-fanned/18238/
Arm: https://ci.nodejs.org/job/node-test-commit-arm/16355/
Linux: https://ci.nodejs.org/job/node-test-commit-linux/19004/

BridgeAR · 2018-05-25T15:29:27Z

I am unsure how to land this one. Without squashing the first commit is going to result in errors. Suggestions?

Node leaves stdio file descriptors in non-blocking mode when exiting. This has been reported, fixed, unfixed, ad nauseum. nodejs/node#14752 nodejs/node#17737 nodejs/node#20592 nodejs/node#21257 Should this become a problem in more places, we could add such a workaround elsewhere. But for now I'm limiting the ugliness to the unit-tests container, where we see this cause a lot of failures. Closes cockpit-project#9484

Node leaves stdio file descriptors in non-blocking mode when exiting. This has been reported, fixed, unfixed, ad nauseum. nodejs/node#14752 nodejs/node#17737 nodejs/node#20592 nodejs/node#21257 Should this become a problem in more places, we could add such a workaround elsewhere. But for now I'm limiting the ugliness to the unit-tests container, where we see this cause a lot of failures. Closes #9484

Before PR 20592, closing all handles associated with the main event loop would also mean that `uv_tty_reset_mode()` can’t function properly because the corresponding FDs have already been closed. Add regression tests for this condition. Refs: #21020 Refs: #20592 PR-URL: #21027 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Node leaves stdio file descriptors in non-blocking mode when exiting. This has been reported, fixed, unfixed, ad nauseum. nodejs/node#14752 nodejs/node#17737 nodejs/node#20592 nodejs/node#21257 Stolen from cockpit-project/cockpit@ef50d97cf4

Before PR 20592, closing all handles associated with the main event loop would also mean that `uv_tty_reset_mode()` can’t function properly because the corresponding FDs have already been closed. Add regression tests for this condition. Refs: #21020 Refs: #20592 PR-URL: #21027 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

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>

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>

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

jasnell requested a review from bnoordhuis May 8, 2018 15:36

addaleax approved these changes May 9, 2018

View reviewed changes

bnoordhuis reviewed May 11, 2018

View reviewed changes

jasnell approved these changes May 12, 2018

View reviewed changes

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

gireeshpunathil mentioned this pull request May 19, 2018

nodejs sometimes leaves stdout/stderr in non-blocking mode #14752

Closed

bnoordhuis and others added 2 commits May 22, 2018 11:43

src: restore stdio on program exit

ae154cf

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. Fixes: #14752

src: Fix error in non TTY environment

4c66c9a

termios-flags can be stored only in TTY environment

MylesBorins mentioned this pull request Jun 6, 2018

v10.4.0 proposal #21167

Merged

gireeshpunathil mentioned this pull request Jun 12, 2018

Revert "src: restore stdio on program exit" #21257

Closed

2 tasks

stefwalter mentioned this pull request Jun 23, 2018

unit-tests: Switch stdio to non-blocking after invoking Node.js cockpit-project/cockpit#9484

Merged

bnoordhuis mentioned this pull request Nov 8, 2018

src: restore stdio on program exit #24260

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: restore stdio on program exit #20592

src: restore stdio on program exit #20592

ktaborski commented May 8, 2018 •

edited

Loading

gireeshpunathil commented May 8, 2018

gireeshpunathil commented May 8, 2018

gireeshpunathil commented May 8, 2018

Trott commented May 8, 2018

Trott commented May 8, 2018

gireeshpunathil commented May 9, 2018

ktaborski commented May 9, 2018

addaleax left a comment

gireeshpunathil commented May 10, 2018

Trott commented May 11, 2018

gireeshpunathil commented May 11, 2018

bnoordhuis commented May 11, 2018

ktaborski commented May 11, 2018

bnoordhuis left a comment

bnoordhuis May 11, 2018

bnoordhuis May 11, 2018

ktaborski May 11, 2018

bnoordhuis May 11, 2018

bnoordhuis May 11, 2018

gireeshpunathil commented May 13, 2018

gireeshpunathil commented May 15, 2018

BridgeAR commented May 18, 2018

ktaborski commented May 19, 2018

apapirovski commented May 22, 2018

ktaborski commented May 22, 2018

jasnell commented May 23, 2018

jasnell commented May 23, 2018

BridgeAR commented May 25, 2018

		@@ -3313,7 +3313,6 @@ void SetupProcessObject(Environment* env,


		void SignalExit(int signo) {

src: restore stdio on program exit #20592

src: restore stdio on program exit #20592

Conversation

ktaborski commented May 8, 2018 • edited Loading

Checklist

gireeshpunathil commented May 8, 2018

gireeshpunathil commented May 8, 2018

gireeshpunathil commented May 8, 2018

Trott commented May 8, 2018

Trott commented May 8, 2018

gireeshpunathil commented May 9, 2018

ktaborski commented May 9, 2018

addaleax left a comment

Choose a reason for hiding this comment

gireeshpunathil commented May 10, 2018

Trott commented May 11, 2018

gireeshpunathil commented May 11, 2018

bnoordhuis commented May 11, 2018

ktaborski commented May 11, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis May 11, 2018

Choose a reason for hiding this comment

bnoordhuis May 11, 2018

Choose a reason for hiding this comment

ktaborski May 11, 2018

Choose a reason for hiding this comment

bnoordhuis May 11, 2018

Choose a reason for hiding this comment

bnoordhuis May 11, 2018

Choose a reason for hiding this comment

gireeshpunathil commented May 13, 2018

gireeshpunathil commented May 15, 2018

BridgeAR commented May 18, 2018

ktaborski commented May 19, 2018

apapirovski commented May 22, 2018

ktaborski commented May 22, 2018

jasnell commented May 23, 2018

jasnell commented May 23, 2018

BridgeAR commented May 25, 2018

ktaborski commented May 8, 2018 •

edited

Loading