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

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Dec 18, 2017

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.

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
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 18, 2017
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@bnoordhuis
Copy link
Member Author

Would this kind of code maybe have a better home in libuv?

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 net.Socket({ fd: fd }) but doesn't clean up afterwards.

This code could migrate to libuv over time but I don't know if it's useful outside of node.js.

Does this affect child processes’ inherited stdio?

Yes, but so does the existing code. Let me think about whether there's a way we can do better.

@BridgeAR
Copy link
Member

BridgeAR commented Jan 5, 2018

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 5, 2018
@joyeecheung
Copy link
Member

Previous CI expired. New CI: https://ci.nodejs.org/job/node-test-pull-request/12567/

@joyeecheung joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2018
@joyeecheung
Copy link
Member

The build is not working on centos6-64

https://ci.nodejs.org/job/node-test-commit-linux/15597/nodes=centos6-64/console

[ -x ./node ] && ./node tools/doc/addon-verify.js || node tools/doc/addon-verify.js
[28174]: ../src/node.cc:4150:void node::PlatformInit(): Assertion `errno == ENODEV || errno == ENOTTY || errno == EOPNOTSUPP' failed.
 1: node::Abort() [./node]
 2: 0x121b84b [./node]
 3: node::PlatformInit() [./node]
 4: node::Start(int, char**) [./node]
 5: __libc_start_main [/lib64/libc.so.6]
 6: 0x8a8341 [./node]
/bin/sh: line 1: 28174 Aborted                 (core dumped) ./node tools/doc/addon-verify.js

@Fishrock123
Copy link
Contributor

@bnoordhuis any status here?

@BridgeAR
Copy link
Member

BridgeAR commented Feb 7, 2018

Ping @bnoordhuis

@joyeecheung
Copy link
Member

BTW, I think this might fix one of the issues lurking around our CI these days:

https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_openssl110_x64/2062/console

05:39:29 make[2]: write error: stdout
05:39:29 gyp infoMakefile:619: recipe for target 'doc-only' failed

@bnoordhuis
Copy link
Member Author

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.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 2, 2018

Ping @bnoordhuis

@BridgeAR
Copy link
Member

Ping @bnoordhuis again

@BridgeAR
Copy link
Member

Closing due to inactivity. Please feel free to reopen when you plan on working on this again @bnoordhuis

@BridgeAR BridgeAR closed this Apr 16, 2018
@ktaborski
Copy link
Contributor

ktaborski commented May 4, 2018

I think, that the error from @joyeecheung is caused, when node is run in non-terminal environment
like Jenkins.

The tcgetattr() function shall fail if:

EBADF
The fildes argument is not a valid file descriptor.
ENOTTY
The file associated with fildes is not a terminal.

errno is EINVAL.

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

do {
Copy link
Contributor

@ktaborski ktaborski May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Jun 23, 2018
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
martinpitt pushed a commit to cockpit-project/cockpit that referenced this pull request Jun 24, 2018
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
martinpitt added a commit to cockpit-project/cockpituous that referenced this pull request Jul 12, 2018
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants