From 5972bd6aa52b19b955386b3a5786c181c16de343 Mon Sep 17 00:00:00 2001 From: Thiago Padilha Date: Mon, 2 Sep 2019 15:51:29 -0300 Subject: [PATCH 1/4] child_process: add 'overlapped' stdio flag The 'overlapped' value sets the UV_OVERLAPPED_PIPE libuv flag in the child process stdio. Fixes: https://github.com/nodejs/node/issues/29238 --- doc/api/child_process.md | 19 +++++++++++++------ lib/internal/child_process.js | 6 ++++-- src/env.h | 1 + src/process_wrap.cc | 5 +++++ 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/doc/api/child_process.md b/doc/api/child_process.md index 02ea48df44d16d..e3bb69f24f8961 100644 --- a/doc/api/child_process.md +++ b/doc/api/child_process.md @@ -675,6 +675,7 @@ equal to `['pipe', 'pipe', 'pipe']`. For convenience, `options.stdio` may be one of the following strings: * `'pipe'`: equivalent to `['pipe', 'pipe', 'pipe']` (the default) +* `'overlapped'`: equivalent to `['overlapped', 'overlapped', 'overlapped']` * `'ignore'`: equivalent to `['ignore', 'ignore', 'ignore']` * `'inherit'`: equivalent to `['inherit', 'inherit', 'inherit']` or `[0, 1, 2]` @@ -688,7 +689,13 @@ pipes between the parent and child. The value is one of the following: `child_process` object as [`subprocess.stdio[fd]`][`subprocess.stdio`]. Pipes created for fds 0, 1, and 2 are also available as [`subprocess.stdin`][], [`subprocess.stdout`][] and [`subprocess.stderr`][], respectively. -2. `'ipc'`: Create an IPC channel for passing messages/file descriptors +2. `'overlapped'`: Same as `'pipe'` except that the `FILE_FLAG_OVERLAPPED` flag + is set on the handle. This is necessary for overlapped I/O on the child + process's stdio handles. See the + [docs](https://docs.microsoft.com/en-us/windows/win32/fileio/synchronous-and-asynchronous-i-o) + for more details. This is exactly the same as `'pipe'` on non-Windows + systems. +3. `'ipc'`: Create an IPC channel for passing messages/file descriptors between parent and child. A [`ChildProcess`][] may have at most one IPC stdio file descriptor. Setting this option enables the [`subprocess.send()`][] method. If the child is a Node.js process, the @@ -699,25 +706,25 @@ pipes between the parent and child. The value is one of the following: Accessing the IPC channel fd in any way other than [`process.send()`][] or using the IPC channel with a child process that is not a Node.js instance is not supported. -3. `'ignore'`: Instructs Node.js to ignore the fd in the child. While Node.js +4. `'ignore'`: Instructs Node.js to ignore the fd in the child. While Node.js will always open fds 0, 1, and 2 for the processes it spawns, setting the fd to `'ignore'` will cause Node.js to open `/dev/null` and attach it to the child's fd. -4. `'inherit'`: Pass through the corresponding stdio stream to/from the +5. `'inherit'`: Pass through the corresponding stdio stream to/from the parent process. In the first three positions, this is equivalent to `process.stdin`, `process.stdout`, and `process.stderr`, respectively. In any other position, equivalent to `'ignore'`. -5. {Stream} object: Share a readable or writable stream that refers to a tty, +6. {Stream} object: Share a readable or writable stream that refers to a tty, file, socket, or a pipe with the child process. The stream's underlying file descriptor is duplicated in the child process to the fd that corresponds to the index in the `stdio` array. The stream must have an underlying descriptor (file streams do not until the `'open'` event has occurred). -6. Positive integer: The integer value is interpreted as a file descriptor +7. Positive integer: The integer value is interpreted as a file descriptor that is currently open in the parent process. It is shared with the child process, similar to how {Stream} objects can be shared. Passing sockets is not supported on Windows. -7. `null`, `undefined`: Use default value. For stdio fds 0, 1, and 2 (in other +8. `null`, `undefined`: Use default value. For stdio fds 0, 1, and 2 (in other words, stdin, stdout, and stderr) a pipe is created. For fd 3 and up, the default is `'ignore'`. diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index 8dd85d287fc950..8512ae342fde65 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -231,6 +231,7 @@ function stdioStringToArray(stdio, channel) { switch (stdio) { case 'ignore': + case 'overlapped': case 'pipe': ArrayPrototypePush(options, stdio, stdio, stdio); break; case 'inherit': ArrayPrototypePush(options, 0, 1, 2); break; default: @@ -976,9 +977,10 @@ function getValidStdio(stdio, sync) { if (stdio === 'ignore') { ArrayPrototypePush(acc, { type: 'ignore' }); - } else if (stdio === 'pipe' || (typeof stdio === 'number' && stdio < 0)) { + } else if (stdio === 'pipe' || stdio === 'overlapped' || + (typeof stdio === 'number' && stdio < 0)) { const a = { - type: 'pipe', + type: stdio === 'overlapped' ? 'overlapped' : 'pipe', readable: i === 0, writable: i !== 0 }; diff --git a/src/env.h b/src/env.h index a0d59ff8728deb..b090021c4c1833 100644 --- a/src/env.h +++ b/src/env.h @@ -343,6 +343,7 @@ constexpr size_t kFsStatsBufferLength = V(options_string, "options") \ V(order_string, "order") \ V(output_string, "output") \ + V(overlapped_string, "overlapped") \ V(parse_error_string, "Parse Error") \ V(password_string, "password") \ V(path_string, "path") \ diff --git a/src/process_wrap.cc b/src/process_wrap.cc index 3d1065c9922183..bbb00153f7a8a8 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -125,6 +125,11 @@ class ProcessWrap : public HandleWrap { options->stdio[i].flags = static_cast( UV_CREATE_PIPE | UV_READABLE_PIPE | UV_WRITABLE_PIPE); options->stdio[i].data.stream = StreamForWrap(env, stdio); + } else if (type->StrictEquals(env->overlapped_string())) { + options->stdio[i].flags = static_cast( + UV_CREATE_PIPE | UV_READABLE_PIPE | UV_WRITABLE_PIPE | + UV_OVERLAPPED_PIPE); + options->stdio[i].data.stream = StreamForWrap(env, stdio); } else if (type->StrictEquals(env->wrap_string())) { options->stdio[i].flags = UV_INHERIT_STREAM; options->stdio[i].data.stream = StreamForWrap(env, stdio); From cf574e638d8dc1e54f867e9b5ff88ce97db37c61 Mon Sep 17 00:00:00 2001 From: Thiago Padilha Date: Fri, 6 Sep 2019 10:23:49 -0300 Subject: [PATCH 2/4] test: Add test for 'overlapped' stdio --- node.gyp | 18 ++++ test/overlapped-checker/main_unix.c | 51 +++++++++++ test/overlapped-checker/main_win.c | 85 +++++++++++++++++++ .../test-child-process-stdio-overlapped.js | 75 ++++++++++++++++ 4 files changed, 229 insertions(+) create mode 100644 test/overlapped-checker/main_unix.c create mode 100644 test/overlapped-checker/main_win.c create mode 100644 test/parallel/test-child-process-stdio-overlapped.js diff --git a/node.gyp b/node.gyp index 865a7de93176e5..5ad9443247691c 100644 --- a/node.gyp +++ b/node.gyp @@ -1469,6 +1469,24 @@ ], }, # embedtest + { + 'target_name': 'overlapped-checker', + 'type': 'executable', + + 'conditions': [ + ['OS=="win"', { + 'sources': [ + 'test/overlapped-checker/main_win.c' + ], + }], + ['OS!="win"', { + 'sources': [ + 'test/overlapped-checker/main_unix.c' + ], + }], + ] + }, # overlapped-checker + # TODO(joyeecheung): do not depend on node_lib, # instead create a smaller static library node_lib_base that does # just enough for node_native_module.cc and the cache builder to diff --git a/test/overlapped-checker/main_unix.c b/test/overlapped-checker/main_unix.c new file mode 100644 index 00000000000000..9d3d83af0f1068 --- /dev/null +++ b/test/overlapped-checker/main_unix.c @@ -0,0 +1,51 @@ +#include +#include +#include + +#include +#include + +static size_t r(char* buf, size_t buf_size) { + ssize_t read_count; + do + read_count = read(0, buf, buf_size); + while (read_count < 0 && errno == EINTR); + if (read_count <= 0) + abort(); + return (size_t)read_count; +} + +static void w(const char* buf, size_t count) { + const char* end = buf + count; + + while (buf < end) { + ssize_t write_count; + do + write_count = write(1, buf, count); + while (write_count < 0 && errno == EINTR); + if (write_count <= 0) + abort(); + buf += write_count; + } + + fprintf(stderr, "%zu", count); + fflush(stderr); +} + +int main(void) { + w("0", 1); + + while (1) { + char buf[256]; + size_t read_count = r(buf, sizeof(buf)); + // The JS part (test-child-process-stdio-overlapped.js) only writes the + // "exit" string when the buffer is empty, so the read is guaranteed to be + // atomic due to it being less than PIPE_BUF. + if (!strncmp(buf, "exit", read_count)) { + break; + } + w(buf, read_count); + } + + return 0; +} diff --git a/test/overlapped-checker/main_win.c b/test/overlapped-checker/main_win.c new file mode 100644 index 00000000000000..c38b7febe6ddc5 --- /dev/null +++ b/test/overlapped-checker/main_win.c @@ -0,0 +1,85 @@ +#include +#include +#include + +#include + +static char buf[256]; +static DWORD read_count; +static DWORD write_count; +static HANDLE stdin_h; +static OVERLAPPED stdin_o; + +static void die(const char* buf) { + fprintf(stderr, "%s\n", buf); + fflush(stderr); + exit(100); +} + +static void overlapped_read(void) { + if (ReadFile(stdin_h, buf, sizeof(buf), NULL, &stdin_o)) { + // Since we start the read operation immediately before requesting a write, + // it should never complete synchronously since no data would be available + die("read completed synchronously"); + } + if (GetLastError() != ERROR_IO_PENDING) { + die("overlapped read failed"); + } +} + +static void write(const char* buf, size_t buf_size) { + overlapped_read(); + DWORD write_count; + HANDLE stdout_h = GetStdHandle(STD_OUTPUT_HANDLE); + if (!WriteFile(stdout_h, buf, buf_size, &write_count, NULL)) { + die("overlapped write failed"); + } + fprintf(stderr, "%d", write_count); + fflush(stderr); +} + +int main(void) { + HANDLE event = CreateEvent(NULL, FALSE, FALSE, NULL); + if (event == NULL) { + die("failed to create event handle"); + } + + stdin_h = GetStdHandle(STD_INPUT_HANDLE); + stdin_o.hEvent = event; + + write("0", 1); + + while (1) { + DWORD result = WaitForSingleObject(event, INFINITE); + if (result == WAIT_OBJECT_0) { + if (!GetOverlappedResult(stdin_h, &stdin_o, &read_count, FALSE)) { + die("failed to get overlapped read result"); + } + if (strncmp(buf, "exit", read_count) == 0) { + break; + } + write(buf, read_count); + } else { + char emsg[0xfff]; + int ecode = GetLastError(); + DWORD rv = FormatMessage( + FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, + NULL, + ecode, + MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), + (LPSTR)emsg, + sizeof(emsg), + NULL); + if (rv > 0) { + snprintf(emsg, sizeof(emsg), + "WaitForSingleObject failed. Error %d (%s)", ecode, emsg); + } else { + snprintf(emsg, sizeof(emsg), + "WaitForSingleObject failed. Error %d", ecode); + } + die(emsg); + } + } + + return 0; +} diff --git a/test/parallel/test-child-process-stdio-overlapped.js b/test/parallel/test-child-process-stdio-overlapped.js new file mode 100644 index 00000000000000..70a6454ff8fc9a --- /dev/null +++ b/test/parallel/test-child-process-stdio-overlapped.js @@ -0,0 +1,75 @@ +// Test for "overlapped" stdio option. This test uses the "overlapped-checker" +// helper program which basically a specialized echo program. +// +// The test has two goals: +// +// - Verify that overlapped I/O works on windows. The test program will deadlock +// if stdin doesn't have the FILE_FLAG_OVERLAPPED flag set on startup (see +// test/overlapped-checker/main_win.c for more details). +// - Verify that "overlapped" stdio option works transparently as a pipe (on +// unix/windows) +// +// This is how the test works: +// +// - This script assumes only numeric strings are written to the test program +// stdout. +// - The test program will be spawned with "overlapped" set on stdin and "pipe" +// set on stdout/stderr and at startup writes a number to its stdout +// - When this script receives some data, it will parse the number, add 50 and +// write to the test program's stdin. +// - The test program will then echo the number back to us which will repeat the +// cycle until the number reaches 200, at which point we send the "exit" +// string, which causes the test program to exit. +// - Extra assertion: Every time the test program writes a string to its stdout, +// it will write the number of bytes written to stderr. +// - If overlapped I/O is not setup correctly, this test is going to hang. +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const child_process = require('child_process'); + +const exeExtension = process.platform === 'win32' ? '.exe' : ''; +const exe = 'overlapped-checker' + exeExtension; +const exePath = path.join(path.dirname(process.execPath), exe); + +const child = child_process.spawn(exePath, [], { + stdio: ['overlapped', 'pipe', 'pipe'] +}); + +child.stdin.setEncoding('utf8'); +child.stdout.setEncoding('utf8'); +child.stderr.setEncoding('utf8'); + +function writeNext(n) { + child.stdin.write((n + 50).toString()); +} + +child.stdout.on('data', (s) => { + const n = Number(s); + if (n >= 200) { + child.stdin.write('exit'); + return; + } + writeNext(n); +}); + +let stderr = ''; +child.stderr.on('data', (s) => { + stderr += s; +}); + +child.stderr.on('end', common.mustCall(() => { + // This is the sequence of numbers sent to us: + // - 0 (1 byte written) + // - 50 (2 bytes written) + // - 100 (3 bytes written) + // - 150 (3 bytes written) + // - 200 (3 bytes written) + assert.strictEqual(stderr, '12333'); +})); + +child.on('exit', common.mustCall((status) => { + // The test program will return the number of writes as status code. + assert.strictEqual(status, 0); +})); From 044d1e2944942c434219fa3d1c78d93920d89ae2 Mon Sep 17 00:00:00 2001 From: Thiago Padilha Date: Sun, 8 Nov 2020 15:33:08 -0300 Subject: [PATCH 3/4] doc: add changes entry in child_process.md --- doc/api/child_process.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/api/child_process.md b/doc/api/child_process.md index e3bb69f24f8961..cd361b630964de 100644 --- a/doc/api/child_process.md +++ b/doc/api/child_process.md @@ -660,6 +660,9 @@ subprocess.unref();