src: restore stdio on program exit#17737
Conversation
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: nodejs#14752
addaleax
left a comment
There was a problem hiding this comment.
LGTM, but I have two questions:
- Would this kind of code maybe have a better home in libuv?
- Does this affect child processes’ inherited stdio? i.e. if the parent of a detached child exits first, does the stdio mode for the child process also get reset?
I should have anticipated that question. :-) Yes, maybe it can, but #14752 is not really a libuv issue. Node.js turns the stdio file descriptors non-blocking with This code could migrate to libuv over time but I don't know if it's useful outside of node.js.
Yes, but so does the existing code. Let me think about whether there's a way we can do better. |
|
Previous CI expired. New CI: https://ci.nodejs.org/job/node-test-pull-request/12567/ |
|
The build is not working on centos6-64 https://ci.nodejs.org/job/node-test-commit-linux/15597/nodes=centos6-64/console |
|
@bnoordhuis any status here? |
|
Ping @bnoordhuis |
|
BTW, I think this might fix one of the issues lurking around our CI these days: |
|
I'll try to revisit this in the next few days. I can't reproduce the centos6 failure locally so I'll probably have to do some printf-style debugging on the CI. |
|
Ping @bnoordhuis |
|
Ping @bnoordhuis again |
|
Closing due to inactivity. Please feel free to reopen when you plan on working on this again @bnoordhuis |
|
I think, that the error from @joyeecheung is caused, when node is run in non-terminal environment
errno is EINVAL. |
| s.flags = GetFileDescriptorFlags(fd); | ||
| CHECK_NE(s.flags, -1); | ||
|
|
||
| do { |
There was a problem hiding this comment.
In my opinions isatty check should be added. Something like this:
if( isatty(fd) ){
do {
err = tcgetattr(fd, &s.termios);
} while (err == -1 && errno == EINTR);
if (err == 0) {
s.isatty = true;
} else {
// tcgetattr() is not supposed to return ENODEV or EOPNOTSUPP
// but it does so anyway on MacOS.
CHECK(errno == ENODEV || errno == ENOTTY || errno == EOPNOTSUPP );
}
}
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
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
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
CI: https://ci.nodejs.org/job/node-test-commit/14917/
Still pondering how to write a good test. The specific issue of #14752 is when fds 1 and 2 both refer to the write end of the same pipe but that's kind of hard to test without resorting to hacks and internals.