Skip to content

child_process: setup stdio on error when possible - #27696

Merged
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:26852
May 20, 2019
Merged

child_process: setup stdio on error when possible#27696
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:26852

Conversation

@cjihrig

@cjihrigcjihrig commented May 14, 2019

Copy link
Copy Markdown
Contributor

As more spawn() errors are classified as runtime errors, it's no longer appropriate to only check UV_ENOENT when determining if stdio can be setup. This commit reverses the check to look for EMFILE and ENFILE specifically.

Fixes: #26852

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

@nodejs-github-bot

nodejs-github-bot commented May 14, 2019

Copy link
Copy Markdown
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3518

@nodejs-github-botnodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label May 14, 2019
@Fishrock123

Fishrock123 commented May 14, 2019

Copy link
Copy Markdown
Contributor

Is this semver-major?

@cjihrig

cjihrig commented May 14, 2019

Copy link
Copy Markdown
ContributorAuthor

It's a breaking change if code relies on stdio streams NOT being configured when spawn fails with UV_EACCES or UV_EAGAIN. My guess is that it's not a breaking change in the real world.

@nodejs-github-bot

nodejs-github-bot commented May 14, 2019•
edited by cjihrig
Loading

Copy link
Copy Markdown
Collaborator

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

EDIT(cjihrig): CI was yellow.

@cjihrig

cjihrig commented May 16, 2019

Copy link
Copy Markdown
ContributorAuthor

Ping for reviews.

@Trott

Trott commented May 17, 2019

Copy link
Copy Markdown
Member

Optional suggestion: The current condition is covered in tests. Would be great if you could confirm that this is still covered with the change.

@cjihrig

cjihrig commented May 18, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@Trott both branches of the conditional are still covered. I also pushed up some assertions to verify it. We may miss coverage on the ENFILE check, but EMFILE is covered.

@nodejs-github-bot

nodejs-github-bot commented May 18, 2019

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 19, 2019

Copy link
Copy Markdown
Collaborator

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

bnoordhuis
bnoordhuis approved these changes May 20, 2019
View reviewed changes
Comment threadlib/internal/child_process.js Outdated

@bnoordhuisbnoordhuisMay 20, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh hey, it's that comment's fifth birthday tomorrow - that's long enough for me to change my mind. :-)

@nodejs-github-bot

nodejs-github-bot commented May 20, 2019•
edited by cjihrig
Loading

Copy link
Copy Markdown
Collaborator

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

EDIT(cjihrig): CI was yellow.

@cjihrig
child_process: setup stdio on error when possible
64182e9
As more spawn() errors are classified as runtime errors, it's
no longer appropriate to only check UV_ENOENT when determining
if stdio can be setup. This commit reverses the check to look
for EMFILE and ENFILE specifically.
PR-URL: nodejs#27696Fixes: nodejs#26852
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@cjihrig
cjihrig merged commit 64182e9 into nodejs:masterMay 20, 2019
@cjihrig
cjihrig deleted the 26852 branch May 20, 2019 13:35
@ehmickyehmicky mentioned this pull request May 20, 2019
Merged
BridgeAR pushed a commit that referenced this pull request May 21, 2019
@cjihrig@BridgeAR
child_process: setup stdio on error when possible
2bc177a
As more spawn() errors are classified as runtime errors, it's
no longer appropriate to only check UV_ENOENT when determining
if stdio can be setup. This commit reverses the check to look
for EMFILE and ENFILE specifically.
PR-URL: #27696Fixes: #26852
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@BridgeARBridgeAR mentioned this pull request May 21, 2019
Merged
4 tasks
@gireeshpunathilgireeshpunathil mentioned this pull request Jun 19, 2019
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes
@TrottTrottTrott approved these changes

Assignees

No one assigned

Labels

child_processIssues and PRs related to the child_process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

No error was thrown when spawn command was existed but non-executable

5 participants

@cjihrig@nodejs-github-bot@Fishrock123@Trott@bnoordhuis