★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/20592Nouvelle recherche | Portail wanayoo
Skip to content

/node

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 #20592

Closed
wants to merge 2 commits into from

Conversation

Reviewers

@bnoordhuis bnoordhuis

@addaleax addaleax

@jasnell jasnell

Assignees

@addaleax addaleax

Labels
C++ author ready
Projects
None yet
Milestone
No milestone
@tabkrz
Copy link
Contributor

tabkrz commented May 8, 2018 •
edited

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

@nodejs-github-bot nodejs-github-bot added the C++ label May 8, 2018

@gireeshpunathil

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented May 8, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented May 8, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented May 8, 2018

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?

@jasnell jasnell requested a review from bnoordhuis May 8, 2018

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented May 8, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented May 8, 2018

@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

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented May 9, 2018

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:

  1. node -> non-node -> node
    After the granchild has exited, check for the fidelity of the non-node child
  2. 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.
@tabkrz

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

tabkrz commented May 9, 2018

I will check.
@addaleax

addaleax approved these changes May 9, 2018
View changes

Copy link
Member

addaleax left a comment

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

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented May 10, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented May 11, 2018

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

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

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented May 11, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

bnoordhuis commented May 11, 2018

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

@tabkrz tabkrz force-pushed the tabkrz:fix_restore_stdio branch 3 times, most recently from 76874f2 to ffe84ae May 11, 2018

@tabkrz

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

tabkrz commented May 11, 2018

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

@tabkrz tabkrz force-pushed the tabkrz:fix_restore_stdio branch from ffe84ae to 01bea03 May 11, 2018

@bnoordhuis

bnoordhuis reviewed May 11, 2018
View changes

Copy link
Member

bnoordhuis left a comment

@tabkrz Yes, it's fine now. Left some comments on the second commit.
src/node.cc Outdated
@@ -3313,7 +3313,6 @@ void SetupProcessObject(Environment* env,


void SignalExit(int signo) {

This comment has been minimized.

Sign in to view
Copy link
@bnoordhuis

bnoordhuis May 11, 2018

Member

Unrelated whitespace change.
src/node.cc Outdated
@@ -4202,17 +4201,19 @@ inline void PlatformInit() {

s.flags = GetFileDescriptorFlags(fd);
CHECK_NE(s.flags, -1);

if (isatty(fd)) {

This comment has been minimized.

Sign in to view
Copy link
@bnoordhuis

bnoordhuis May 11, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@tabkrz

tabkrz May 11, 2018

Author Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@bnoordhuis

bnoordhuis May 11, 2018

Member

That's what I mean, of course. :-)
src/node.cc Outdated
// tcgetattr() is not supposed to return ENODEV or EOPNOTSUPP
// but it does so anyway on MacOS.
CHECK(errno == ENODEV || errno == ENOTTY || errno == EOPNOTSUPP);
}

This comment has been minimized.

Sign in to view
Copy link
@bnoordhuis

bnoordhuis May 11, 2018

Member

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

@tabkrz tabkrz force-pushed the tabkrz:fix_restore_stdio branch from 01bea03 to 113dce5 May 11, 2018

@jasnell

jasnell approved these changes May 12, 2018
View changes

@gireeshpunathil

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented May 13, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented May 15, 2018

all clean.

@gireeshpunathil gireeshpunathil added the author ready label May 15, 2018

@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

BridgeAR commented May 18, 2018

@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?
@tabkrz

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

tabkrz commented May 19, 2018

Fine for me.

@gireeshpunathil gireeshpunathil referenced this pull request May 19, 2018

Open

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

@apapirovski

This comment has been minimized.

Sign in to view
Copy link
Member

apapirovski commented May 22, 2018

@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/)

@tabkrz tabkrz force-pushed the tabkrz:fix_restore_stdio branch from 113dce5 to dbd3b74 May 22, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 5, 2018

@addaleax @shisama
test: check TTY mode reset on exit 
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: nodejs#21020
Refs: nodejs#20592

PR-URL: nodejs#21027
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
38f2811

@MylesBorins MylesBorins referenced this pull request Jun 6, 2018

Merged

v10.4.0 proposal #21167

shisama added a commit to shisama/node that referenced this pull request Jun 9, 2018

@bnoordhuis @tabkrz @shisama
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.

Co-authored-by: Krzysztof Taborski <taborskikrzysztof@gmail.com>
PR-URL: nodejs#20592
Fixes: nodejs#14752
Fixes: nodejs#21020
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addd0c5

shisama added a commit to shisama/node that referenced this pull request Jun 9, 2018

@addaleax @shisama
test: check TTY mode reset on exit 
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: nodejs#21020
Refs: nodejs#20592

PR-URL: nodejs#21027
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
fdb274c

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

@bnoordhuis @tabkrz @shisama
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.

Co-authored-by: Krzysztof Taborski <taborskikrzysztof@gmail.com>
PR-URL: nodejs#20592
Fixes: nodejs#14752
Fixes: nodejs#21020
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
6171823

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

@addaleax @shisama
test: check TTY mode reset on exit 
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: nodejs#21020
Refs: nodejs#20592

PR-URL: nodejs#21027
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
6f42a98

@gireeshpunathil gireeshpunathil referenced this pull request Jun 12, 2018

Closed

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

2 of 2 tasks complete

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

@bnoordhuis @tabkrz @shisama
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.

Co-authored-by: Krzysztof Taborski <taborskikrzysztof@gmail.com>
PR-URL: nodejs#20592
Fixes: nodejs#14752
Fixes: nodejs#21020
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
c947e38

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

@addaleax @shisama
test: check TTY mode reset on exit 
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: nodejs#21020
Refs: nodejs#20592

PR-URL: nodejs#21027
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
4acec06

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

@bnoordhuis @tabkrz @shisama
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.

Co-authored-by: Krzysztof Taborski <taborskikrzysztof@gmail.com>
PR-URL: nodejs#20592
Fixes: nodejs#14752
Fixes: nodejs#21020
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
e57c5fd

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

@addaleax @shisama
test: check TTY mode reset on exit 
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: nodejs#21020
Refs: nodejs#20592

PR-URL: nodejs#21027
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
e75a4a0

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Jun 23, 2018

@stefwalter
unit-tests: Switch stdio to non-blocking after invoking Node.js 
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
d045c5d

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Jun 23, 2018

@stefwalter
unit-tests: Switch stdio to non-blocking after invoking Node.js 
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.
a882c88

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Jun 23, 2018

@stefwalter
unit-tests: Switch stdio to non-blocking after invoking Node.js 
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.
e290d79

@stefwalter stefwalter referenced this pull request Jun 23, 2018

Merged

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

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Jun 23, 2018

@stefwalter
unit-tests: Switch stdio to non-blocking after invoking Node.js 
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.
1625a3e

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Jun 23, 2018

@stefwalter
unit-tests: Switch stdio to non-blocking after invoking Node.js 
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
d920c1c

martinpitt added a commit to cockpit-project/cockpit that referenced this pull request Jun 24, 2018

@stefwalter @martinpitt
unit-tests: Switch stdio to non-blocking after invoking Node.js 
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
Loading status checks…
ef50d97

martinpitt added a commit to martinpitt/cockpituous that referenced this pull request Jun 27, 2018

@martinpitt
release: Switch stdio to non-blocking after invoking Node.js 
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@ef50d97
29eb243

martinpitt added a commit to martinpitt/cockpituous that referenced this pull request Jun 28, 2018

@martinpitt
release: Don't let node mess up stdio 
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@ef50d97
14e6668

KKoukiou added a commit to KKoukiou/cockpit that referenced this pull request Jun 29, 2018

@stefwalter @KKoukiou
unit-tests: Switch stdio to non-blocking after invoking Node.js 
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
58a18cb

addaleax added a commit that referenced this pull request Jun 29, 2018

@addaleax
test: check TTY mode reset on exit 
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>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
bcbae88

martinpitt added a commit to martinpitt/cockpituous that referenced this pull request Jul 11, 2018

@martinpitt
release: Don't let node mess up stdio 
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@ef50d97
deb71a3

martinpitt added a commit to martinpitt/cockpituous that referenced this pull request Jul 11, 2018

@martinpitt
release: Don't let node mess up stdio 
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@ef50d97
f11e89d

martinpitt added a commit to martinpitt/cockpituous that referenced this pull request Jul 11, 2018

@martinpitt
release: Don't let node mess up stdio 
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@ef50d97
9acd8f9

martinpitt added a commit to cockpit-project/cockpituous that referenced this pull request Jul 12, 2018

@martinpitt
release: Don't let node mess up stdio 
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@ef50d97
Loading status checks…
159a916

rvagg added a commit that referenced this pull request Aug 16, 2018

@addaleax @rvagg
test: check TTY mode reset on exit 
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>
f0f44f6

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Nov 8, 2018

@bnoordhuis
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
fe35f69

@bnoordhuis bnoordhuis referenced this pull request Nov 8, 2018

Open

src: restore stdio on program exit #24260

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Nov 12, 2018

@bnoordhuis
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
f4fc6e7
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.