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

process: provide dummy stdio for non-console Windows apps #20640

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented May 9, 2018

The only known condition where we could not provide appropriate
stdio streams so far were non-console Windows applications.
Since this issue has come up a few times in our issue tracker now,
switch to providing dummy streams for these cases instead.

If there are other valid cases in which uv_guess_handle fails,
and where there is a more sensible way to provide stdio,
we’ll probably still find out because the streams don’t work
properly either way.

Refs: nodejs/help#1251

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. labels May 9, 2018
@addaleax addaleax added windows Issues and PRs related to the Windows platform. and removed errors Issues and PRs related to JavaScript errors originated in Node.js core. labels May 9, 2018
@mscdex
Copy link
Contributor

mscdex commented May 9, 2018

s/knonw/known/ in commit message

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

otherwise lgtm

@gireeshpunathil
Copy link
Member

@addaleax - you took the approach of covering all the uv_guess_handle failure cases under windows console scenario right? I guess there were real erroneous scenarios (i.e., running under windows powershell) that led to uv_guess_handle failure through race conditions. We never bothered to debug them as we haven't made a support statement around powershell. I am unable to find references, @bnoordhuis or @refack may know better on this.

Given the possibility of behavioral change (standard stream data absorbed / unavailable) will this be a semver major?

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Code changes LGTM. On the fence as to whether it's a good idea to unconditionally suppress all errors.

doc/api/errors.md Show resolved Hide resolved
@addaleax
Copy link
Member Author

I guess there were real erroneous scenarios (i.e., running under windows powershell) that led to uv_guess_handle failure through race conditions. We never bothered to debug them as we haven't made a support statement around powershell.

Sorry, I didn’t know of that. What I tried to express in the commit message is that even in those cases, things don’t work like they should and we’d probably still get a bug report. The flipside here is that I don’t know how else to address this if we do want non-throwing behaviour for stdio in Windows apps (which I think we want).

Given the possibility of behavioral change (standard stream data absorbed / unavailable) will this be a semver major?

I don’t really see this as a “something that worked before doesn’t work anymore” kind of thing, but I can also understand if people want to be careful. For now, I’ll just add dont-land labels for LTS.

const { Readable } = require('stream');
stdin = new Readable({ read() {} });
stdin.push(null);
break;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can we remove this break and the one below?

Copy link
Member Author

Choose a reason for hiding this comment

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

@lpinca done!

@addaleax
Copy link
Member Author

@gireeshpunathil Are you okay with this?

Also, yes, I would like to go with semver-patch for this one, unless anybody has strong feelings about it.

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

@gireeshpunathil
Copy link
Member

Unfortunately I am unable to make up myself with the mapping of uv_guess_handle failure with a win-32 non-console embedding scenario. At the same time I don't have a suggestion to offer too!

So I am neither approving nor blocking.

@addaleax
Copy link
Member Author

@jasnell
Copy link
Member

jasnell commented May 23, 2018

Fresh CI run since the previous run results are no longer available: https://ci.nodejs.org/job/node-test-pull-request/15063/

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels May 29, 2018
@BridgeAR
Copy link
Member

Seems like there are related test failures.

@jasnell
Copy link
Member

jasnell commented Sep 10, 2018

Ping @addaleax

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018
@addaleax addaleax force-pushed the dummy-stdio branch 2 times, most recently from fc2a431 to 13904dc Compare October 13, 2018 20:58
@addaleax
Copy link
Member Author

@Trott
Copy link
Member

Trott commented Dec 4, 2018

Landed in ab6c09b

@Trott Trott closed this Dec 4, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 4, 2018
The only known condition where we could not provide appropriate
stdio streams so far were non-console Windows applications.
Since this issue has come up a few times in our issue tracker now,
switch to providing dummy streams for these cases instead.

If there are other valid cases in which `uv_guess_handle` fails,
and where there is a more sensible way to provide stdio,
we’ll probably still find out because the streams don’t work
properly either way.

Refs: nodejs/help#1251

PR-URL: nodejs#20640
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
The only known condition where we could not provide appropriate
stdio streams so far were non-console Windows applications.
Since this issue has come up a few times in our issue tracker now,
switch to providing dummy streams for these cases instead.

If there are other valid cases in which `uv_guess_handle` fails,
and where there is a more sensible way to provide stdio,
we’ll probably still find out because the streams don’t work
properly either way.

Refs: nodejs/help#1251

PR-URL: #20640
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
@BridgeAR
Copy link
Member

BridgeAR commented Dec 7, 2018

This test is failing in the 11.4.0 release proposal in the same way as it did here before.

@addaleax I am uncertain what is best to pull out: this test, the libuv update or both. Do you have a suggestion?

@BridgeAR
Copy link
Member

BridgeAR commented Dec 7, 2018

@targos and me decided to pull this one out instead of the libuv update. I added the backport-requested label accordingly.

@BridgeAR BridgeAR mentioned this pull request Dec 7, 2018
2 tasks
@addaleax
Copy link
Member Author

addaleax commented Dec 7, 2018

@BridgeAR Do you know why the libuv update + this PR in combination don’t fail on master?

Also, I am not optimistic about being able to backport this without engagement from somebody on macOS.

@addaleax addaleax deleted the dummy-stdio branch December 7, 2018 13:38
@BridgeAR
Copy link
Member

BridgeAR commented Dec 7, 2018

@addaleax no, sorry. I have no idea why it works on master. But on 11 it failed on two CIs in a row.

@nodejs/platform-macos PTAL

addaleax added a commit to addaleax/node that referenced this pull request Jan 5, 2019
The only known condition where we could not provide appropriate
stdio streams so far were non-console Windows applications.
Since this issue has come up a few times in our issue tracker now,
switch to providing dummy streams for these cases instead.

If there are other valid cases in which `uv_guess_handle` fails,
and where there is a more sensible way to provide stdio,
we’ll probably still find out because the streams don’t work
properly either way.

Refs: nodejs/help#1251

PR-URL: nodejs#20640
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@addaleax
Copy link
Member Author

addaleax commented Jan 5, 2019

Opened a backport PR at #25356, let’s see if it still fails (and if so, try to figure out why).

BridgeAR pushed a commit that referenced this pull request Jan 10, 2019
The only known condition where we could not provide appropriate
stdio streams so far were non-console Windows applications.
Since this issue has come up a few times in our issue tracker now,
switch to providing dummy streams for these cases instead.

If there are other valid cases in which `uv_guess_handle` fails,
and where there is a more sensible way to provide stdio,
we’ll probably still find out because the streams don’t work
properly either way.

Refs: nodejs/help#1251

PR-URL: #20640
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
addaleax added a commit that referenced this pull request Jan 14, 2019
The only known condition where we could not provide appropriate
stdio streams so far were non-console Windows applications.
Since this issue has come up a few times in our issue tracker now,
switch to providing dummy streams for these cases instead.

If there are other valid cases in which `uv_guess_handle` fails,
and where there is a more sensible way to provide stdio,
we’ll probably still find out because the streams don’t work
properly either way.

Refs: nodejs/help#1251

PR-URL: #20640
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
The only known condition where we could not provide appropriate
stdio streams so far were non-console Windows applications.
Since this issue has come up a few times in our issue tracker now,
switch to providing dummy streams for these cases instead.

If there are other valid cases in which `uv_guess_handle` fails,
and where there is a more sensible way to provide stdio,
we’ll probably still find out because the streams don’t work
properly either way.

Refs: nodejs/help#1251

PR-URL: nodejs#20640
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
The only known condition where we could not provide appropriate
stdio streams so far were non-console Windows applications.
Since this issue has come up a few times in our issue tracker now,
switch to providing dummy streams for these cases instead.

If there are other valid cases in which `uv_guess_handle` fails,
and where there is a more sensible way to provide stdio,
we’ll probably still find out because the streams don’t work
properly either way.

Refs: nodejs/help#1251

PR-URL: nodejs#20640
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
@Trott
Copy link
Member

Trott commented Jul 25, 2019

This has a backport-requested label, but it cherry-picks cleanly to the current staging branch. (Didn't check to see if the tests pass, but the backport to 11.x for this also lands cleanly on the 10.x-staging branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. process Issues and PRs related to the process subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.