process: provide dummy stdio for non-console Windows apps by addaleax · Pull Request #20640 · nodejs/node

addaleax · 2018-05-09T18:52:48Z

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

mscdex · 2018-05-09T21:46:40Z

s/knonw/known/ in commit message

doc/api/errors.md

Fishrock123

otherwise lgtm

gireeshpunathil · 2018-05-10T01:43:01Z

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

bnoordhuis

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

doc/api/errors.md

addaleax · 2018-05-10T11:20:20Z

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.

lpinca · 2018-05-10T19:55:23Z

lib/internal/process/stdio.js

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

@lpinca done!

addaleax · 2018-05-13T20:32:18Z

@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 · 2018-05-14T05:13:47Z

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 · 2018-05-14T16:17:44Z

OS X re-run: https://ci.nodejs.org/job/node-test-commit-osx/18546/
Windows re-run: https://ci.nodejs.org/job/node-test-commit-windows-fanned/17969/

jasnell · 2018-05-23T18:42:18Z

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

BridgeAR · 2018-05-30T17:10:54Z

Seems like there are related test failures.

jasnell · 2018-09-10T18:12:42Z

Ping @addaleax

addaleax · 2018-10-13T21:16:35Z

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

Trott · 2018-12-04T19:24:42Z

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19205/ ✔️

Trott · 2018-12-04T21:09:15Z

Landed in ab6c09b

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 <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

BridgeAR · 2018-12-07T12:15:54Z

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 · 2018-12-07T12:28:40Z

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

addaleax · 2018-12-07T13:38:43Z

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

BridgeAR · 2018-12-07T13:50:03Z

@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

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 <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

addaleax · 2019-01-05T22:00:15Z

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

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 <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Trott · 2019-07-25T05:32:06Z

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.

addaleax requested a review from gireeshpunathil May 9, 2018 18:52

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

jasnell requested review from Fishrock123 and bnoordhuis May 9, 2018 22:30

jasnell approved these changes May 9, 2018

View reviewed changes

Fishrock123 reviewed May 9, 2018

View reviewed changes

doc/api/errors.md Outdated Show resolved Hide resolved

Fishrock123 approved these changes May 9, 2018

View reviewed changes

bnoordhuis approved these changes May 10, 2018

View reviewed changes

doc/api/errors.md Outdated Show resolved Hide resolved

addaleax added dont-land-on-v6.x labels May 10, 2018

lpinca approved these changes May 10, 2018

View reviewed changes

addaleax force-pushed the dummy-stdio branch from 4df7637 to f49144c Compare May 13, 2018 20:26

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

jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018

addaleax force-pushed the dummy-stdio branch 2 times, most recently from fc2a431 to 13904dc Compare October 13, 2018 20:58

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 4, 2018

Trott closed this Dec 4, 2018

This was referenced Dec 5, 2018

CI failures: 20181201-20181205 nodejs/reliability#19

Closed

addon build failures on Windows machines nodejs/build#1622

Closed

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

BridgeAR added the backport-requested-v11.x label Dec 7, 2018

BridgeAR mentioned this pull request Dec 7, 2018

deps: upgrade to libuv 1.24.0 #24332

Merged

2 tasks

addaleax deleted the dummy-stdio branch December 7, 2018 13:38

addaleax mentioned this pull request Jan 5, 2019

[v11.x] process: provide dummy stdio for non-console Windows apps #25356

Closed

BridgeAR added backported-to-v11.x and removed backport-requested-v11.x labels Jan 10, 2019

BridgeAR mentioned this pull request Jan 16, 2019

v11.7.0 proposal #25537

Merged

MylesBorins mentioned this pull request Jan 24, 2019

v11.8.0 proposal #25687

Merged

BethGriggs added the backport-requested-v10.x label Jul 3, 2019

Uh oh!

Conversation

addaleax commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mscdex commented May 9, 2018

Uh oh!

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil commented May 10, 2018

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

addaleax commented May 10, 2018

Uh oh!

lpinca May 10, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax May 13, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax commented May 13, 2018

Uh oh!

gireeshpunathil commented May 14, 2018

Uh oh!

addaleax commented May 14, 2018

Uh oh!

jasnell commented May 23, 2018

Uh oh!

BridgeAR commented May 30, 2018

Uh oh!

jasnell commented Sep 10, 2018

Uh oh!

addaleax commented Oct 13, 2018

Uh oh!

Trott commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Dec 4, 2018

Uh oh!

BridgeAR commented Dec 7, 2018

Uh oh!

BridgeAR commented Dec 7, 2018

Uh oh!

addaleax commented Dec 7, 2018

Uh oh!

BridgeAR commented Dec 7, 2018

Uh oh!

addaleax commented Jan 5, 2019

Uh oh!

Trott commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

addaleax commented May 9, 2018 •

edited

Loading

Trott commented Dec 4, 2018 •

edited

Loading

Trott commented Jul 25, 2019 •

edited

Loading