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

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

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)

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

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.

Can you also add a test for this?

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?

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

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.

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.

PR ready for final review.

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

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

aduh95 left a comment •

Loading

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

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

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

Choose a reason for hiding this comment

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

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.

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

@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

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

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

Fixes: nodejs#29238