child_process: add 'overlapped' stdio flag by tarruda · Pull Request #29412 · nodejs/node

tarruda · 2019-09-02T18:58:05Z

The 'overlapped' value sets the UV_OVERLAPPED_PIPE libuv flag in the child process stdio.

Fixes: #29238

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests added
documentation is changed or added
commit message follows commit guidelines

mscdex · 2019-09-02T19:40:27Z

Why not just always set UV_OVERLAPPED_PIPE for 'pipe' instead? Are there any drawbacks/incompatibilities with that?

tarruda · 2019-09-02T19:53:12Z

Why not just always set UV_OVERLAPPED_PIPE for 'pipe' instead? Are there any drawbacks/incompatibilities with that?

Yes, it can possibly break existing programs that run child process that are not prepared for overlapped I/O: libuv/libuv#95 (comment)

mscdex · 2019-09-02T23:30:43Z

Ok, perhaps we can just shorten the name to 'overlapped' instead?

tarruda · 2019-09-03T12:02:50Z

Ok, perhaps we can just shorten the name to 'overlapped' instead?

Added a fixup. If this is acceptable I will squash and edit the commit message.

mscdex · 2019-09-03T14:19:17Z

Can you also add a test for this?

tarruda · 2019-09-03T15:04:26Z

Can you also add a test for this?

Do you have any suggestions on how I should proceed to testing this?

The new option simply activates the UV_OVERLAPPED_PIPE libuv flag, is there any existing test infrastructure to verify that the correct flags were passed by the new option? I couldn't find any reference to UV_CREATE_PIPE in the tests, so I imagine the answer is "no".

I suppose I could just add a functional test that verifies overlapped I/O on the child process, but that would probably be duplicating one of the libuv tests.

Also, the code path should be close to the one followed by the'pipe' value. Would it be enough to copy one of the 'pipe' tests but pass 'overlapped' instead?

mscdex · 2019-09-05T17:29:25Z

At the very least there should be a test that specifying 'overlapped' (via stdio: 'overlapped' or manually expanded out to stdio: ['overlapped', 'overlapped', 'overlapped']) does not throw an exception (since the code currently throws on unsupported modes).

Pending tests as mscdex mentioned.

tarruda · 2019-09-05T18:02:20Z

At the very least there should be a test that specifying 'overlapped' (via stdio: 'overlapped' or manually expanded out to stdio: ['overlapped', 'overlapped', 'overlapped']) does not throw an exception (since the code currently throws on unsupported modes).

👍

I also want to add a test that runs a C++ windows program that will not work if the stdio handles are not overlapped pipes.

tarruda · 2019-09-06T21:54:14Z

I've added "overlapped-checker", a program which is used to verify the FILE_FLAG_OVERLAPPED status from the child. The new test uses this program to verify that the flag is correctly passed.

tarruda · 2019-09-06T23:30:47Z

PR ready for final review.

tarruda · 2020-12-16T12:11:03Z

Following @joaocgreis suggestion on how to make the test find overlapped-checker.exe:

To move forward, if I'm right about this, the easiest way is to find overlapped-checker relative to the executable, like what is done for openssl-cli in

nodejs-github-bot · 2020-12-16T17:37:05Z

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

tarruda · 2020-12-16T18:26:47Z

it seems @joaocgreis's suggestion worked, rebasing on master a squashing the fixup

nodejs-github-bot · 2020-12-17T09:47:47Z

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

aduh95

RSLGTM because Ben approved it already, but I'd feel more confident if we can get more approvals from other collaborators before landing.

tarruda · 2020-12-28T12:20:59Z

@aduh95 Not sure if it helps, but I can try to explain the PR to make you more confident about merging.

The "overlapped" stdio option is simply exposing libuv's UV_OVERLAPPED_PIPE, which itself is a wrapper around FILE_FLAG_OVERLAPPED win32 flag. All this flag does it enable non-blocking I/O on child process's std handles and has no effect on Unix systems (it is the same as "pipe" on Unix).

Most of the code in the PR is dedicated to testing that the flag is passed correctly (AFAIR, libuv doesn't have any test covering it). The way the test works is that it will hang if the FILE_FLAG_OVERLAPPED flag is not passed to the child process. If you want, I can push a temporary commit that will modify the test to pass pipe instead of overlapped and we can see the test hang.

tarruda · 2020-12-28T20:13:25Z

@aduh95 I've added a temporary commit that removes the "overlapped" flag. If you request-ci, we should see windows test hang

nodejs-github-bot · 2020-12-29T00:38:56Z

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

aduh95

Thanks for the explanation, it's clear to me now.

Can you remove the timer from the test please? There is a default timeout set by the test.py script, so the timeout can be customized by the test runner.

node/tools/test.py

Lines 1333 to 1334 in e57d8af

result.add_option("-t", "--timeout", help="Timeout in seconds", 
default=120, type="int") 

aduh95 · 2020-12-29T10:25:30Z

@tarruda Do you want to revert 45e83ab to have this ready to land?

tarruda · 2020-12-29T10:40:45Z

@tarruda Do you want to revert 45e83ab to have this ready to land?

Removed it and added a fixup with the timeout removal on the tests

nodejs-github-bot · 2020-12-29T11:04:46Z

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

tarruda · 2020-12-29T11:42:57Z

@aduh95node-test-linux-linked-debug failed, but it seems unrelated to this PR. Logs show that the ld process was killed during linking of cctest

aduh95 · 2020-12-31T08:48:38Z

@aduh95node-test-linux-linked-debug failed, but it seems unrelated to this PR. Logs show that the ld process was killed during linking of cctest

Probably unrelated indeed, it happens sometimes. I've kicked off another CI to make sure.

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

The 'overlapped' value sets the UV_OVERLAPPED_PIPE libuv flag in the child process stdio. Fixes: nodejs#29238

nodejs-github-bot · 2021-01-03T18:04:09Z

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

aduh95 · 2021-01-03T21:42:33Z

Landed in 25b21e4 🎉

tarruda · 2021-01-04T09:20:38Z

Thanks @aduh95 @joaocgreis @bnoordhuis for the help with this PR

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. labels Sep 2, 2019

tarruda force-pushed the expose-stdio-overlapped-pipe branch from 9bbddd4 to 57c0f01Compare September 2, 2019 19:00

tarruda changed the title ~~child_process: Add 'pipe+overlapped' stdio flag~~child_process: add 'pipe+overlapped' stdio flagSep 2, 2019

tarruda force-pushed the expose-stdio-overlapped-pipe branch from 57c0f01 to d4eb7bcCompare September 2, 2019 19:55

tarruda changed the title ~~child_process: add 'pipe+overlapped' stdio flag~~child_process: add 'overlapped' stdio flagSep 3, 2019

tarruda force-pushed the expose-stdio-overlapped-pipe branch from 0a5b520 to 196c6eaCompare September 3, 2019 12:59

Fishrock123 previously approved these changes Sep 5, 2019
View reviewed changes

Trott added the wip Issues and PRs that are still a work in progress. label Sep 6, 2019

tarruda force-pushed the expose-stdio-overlapped-pipe branch 5 times, most recently from 31ac422 to e06685eCompare September 6, 2019 21:46

tarruda force-pushed the expose-stdio-overlapped-pipe branch 3 times, most recently from 9ad1420 to a28f5a3Compare September 6, 2019 23:17

tarruda requested a review from Fishrock123 September 6, 2019 23:52

Trott removed the wip Issues and PRs that are still a work in progress. label Sep 7, 2019

tarruda force-pushed the expose-stdio-overlapped-pipe branch 2 times, most recently from c654e7a to 7094378Compare December 16, 2020 11:51

aduh95 approved these changes Dec 17, 2020
View reviewed changes

aduh95 reviewed Dec 29, 2020
View reviewed changes

Comment threadtest/parallel/test-child-process-stdio-overlapped.js Outdated
Comment threadtest/parallel/test-child-process-stdio-overlapped.js Outdated

aduh95 mentioned this pull request Dec 31, 2020
Cannot start CI jobs nodejs/build#2506
Closed

tarruda added 4 commits December 31, 2020 11:14

child_process: add 'overlapped' stdio flag
5972bd6
The 'overlapped' value sets the UV_OVERLAPPED_PIPE libuv flag in the child process stdio. Fixes: nodejs#29238

test: Add test for 'overlapped' stdio
cf574e6

doc: add changes entry in child_process.md
044d1e2

doc: cleanup child_process stdio opt list
ff04dee

jasnell approved these changes Dec 31, 2020
View reviewed changes

danielleadams mentioned this pull request Jan 12, 2021
v15.6.0 proposal #36889
Merged

targos mentioned this pull request Sep 4, 2021
v14.18.0 release proposal #39990
Merged

This was referenced Jun 17, 2023
Unreachable code reached when using {stdio: "overlapped"} #48476
Open
src: support sync 'overlapped' stdio option #48479
Open

theanarkh mentioned this pull request Mar 30, 2024
Process crashes when using child_process.spawnSync(..., {stdio: ['pipe', 'overlapped', 'pipe']})#52265
Closed

	result.add_option("-t", "--timeout", help="Timeout in seconds",
	default=120, type="int")

Uh oh!

Uh oh!

Conversation

tarruda commented Sep 2, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mscdex commented Sep 2, 2019

Uh oh!

tarruda commented Sep 2, 2019

Uh oh!

mscdex commented Sep 2, 2019

Uh oh!

tarruda commented Sep 3, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Sep 3, 2019

Uh oh!

tarruda commented Sep 3, 2019

Uh oh!

mscdex commented Sep 5, 2019

Uh oh!

tarruda commented Sep 5, 2019

Uh oh!

tarruda commented Sep 6, 2019

Uh oh!

tarruda commented Sep 6, 2019

Uh oh!

tarruda commented Dec 16, 2020

Uh oh!

nodejs-github-bot commented Dec 16, 2020

Uh oh!

tarruda commented Dec 16, 2020

Uh oh!

nodejs-github-bot commented Dec 17, 2020

Uh oh!

aduh95 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarruda commented Dec 28, 2020

Uh oh!

tarruda commented Dec 28, 2020

Uh oh!

nodejs-github-bot commented Dec 29, 2020

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aduh95 commented Dec 29, 2020

Uh oh!

tarruda commented Dec 29, 2020

Uh oh!

nodejs-github-bot commented Dec 29, 2020

Uh oh!

tarruda commented Dec 29, 2020

Uh oh!

aduh95 commented Dec 31, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 3, 2021

Uh oh!

aduh95 commented Jan 3, 2021

Uh oh!

tarruda commented Jan 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

tarruda commented Sep 2, 2019•
edited
Loading

tarruda commented Sep 3, 2019•
edited
Loading

aduh95 left a comment •
edited
Loading

aduh95 commented Dec 31, 2020•
edited
Loading