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

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

Closed
wants to merge 2 commits into from

Conversation

evanlucas
Copy link
Contributor

This reverts commit c2c9c0c.
It seems to be causing hangs when piping output to other processes.

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

This reverts commit c2c9c0c.
It seems to be causing hangs when piping output to other processes.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 11, 2018
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

Should we be adding a regression test for #21213? (Maybe separate PR?)

@addaleax
Copy link
Member

Oh … so … about the test failure.

The original PR fixed another bug, namely #21020. I opened a PR for that, #21027, but because this had already been fixed by the other PR, I stripped it down to the tests only.

Can you include the src/node.cc part of 63806b1cd62cee61d8be121768031d50c389efd8 here? That was my original change, and it should solve the CI issue (so that we keep #21020 fixed).

Otherwise, 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.

Fixes: nodejs#21020
@addaleax
Copy link
Member

@evanlucas Took the liberty of pushing that commit into this PR, hope that’s okay – tests should look better now:

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

@addaleax addaleax added the tty Issues and PRs related to the tty subsystem. label Jun 11, 2018
@addaleax
Copy link
Member

After a quick chat with @evanlucas, we agreed that we’d like to fast-track this in time for the v10.x security release tomorrow. Please 👍 if you agree.

(CI is ✔️✔️✔️)

@addaleax addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 11, 2018
@addaleax
Copy link
Member

Landed in 43fd1d7, 14dc17d

@addaleax addaleax closed this Jun 11, 2018
addaleax added a commit that referenced this pull request Jun 11, 2018
Otherwise, 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.

Fixes: #21020
PR-URL: #21257
Reviewed-By: Richard Lau <[email protected]>
addaleax pushed a commit that referenced this pull request Jun 11, 2018
This reverts commit c2c9c0c.
It seems to be causing hangs when piping output to other processes.

PR-URL: #21257
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@evanlucas evanlucas deleted the revertstdiorestore branch June 11, 2018 22:46
@gireeshpunathil
Copy link
Member

sorry, I am unable to follow. @evanlucas / @addaleax, can you please clarify what is the current state of the code base, after this landing?

@addaleax
Copy link
Member

@gireeshpunathil The revert commit reverts #20592. The other commit fixes #21020, which is one of the problems addressed by #20592. The test changes for that bug came in #21027, which remain on master.

@gireeshpunathil
Copy link
Member

@addaleax - thanks for the clarification, so this means the current master state is:

  • tty echo mode is restored on exit
  • stdio flow mode is NOT restored on exit
    Am I right?

@addaleax
Copy link
Member

@gireeshpunathil Yes, I think so.

@jdx
Copy link
Contributor

jdx commented Jun 13, 2018

I just tested #21020 in 10.4.1 and the bug has not regressed 👍

Thanks for all the hard work!

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
@AdamMajer
Copy link
Contributor

This revert broke the build of NodeJS on OBS (open build service) for openSUSE. Reverting the revert fixes problem.

FWIW, messing around with nonblocking stdio file descriptors without dup() has unintended consequences.

@addaleax
Copy link
Member

@AdamMajer If you are seeing issues with building Node.js master (or some other tip-of-branch), could you open a new issue?

AdamMajer added a commit to AdamMajer/nodejs-packaging that referenced this pull request Sep 10, 2018
Reverting the revert ... otherwise stdout/stderr are left
in O_NONBLOCK state on exit from Node in certain circumstances
which results in failture of %install script as the stdout
pipe of OBS overflows and returns EAGAIN
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++. fast-track PRs that do not need to wait for 48 hours to land. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants