From 169c673a2f2376c6b80885aa1343d2a8df60acb2 Mon Sep 17 00:00:00 2001 From: pfg Date: Tue, 28 Jan 2025 19:23:39 -0800 Subject: [PATCH 01/12] test-child-process-spawn-shell.js --- .../test-child-process-spawn-shell.js | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 test/js/node/test/parallel/test-child-process-spawn-shell.js diff --git a/test/js/node/test/parallel/test-child-process-spawn-shell.js b/test/js/node/test/parallel/test-child-process-spawn-shell.js new file mode 100644 index 00000000000000..d96127678bd330 --- /dev/null +++ b/test/js/node/test/parallel/test-child-process-spawn-shell.js @@ -0,0 +1,65 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const cp = require('child_process'); + +// Verify that a shell is, in fact, executed +const doesNotExist = cp.spawn('does-not-exist', { shell: true }); + +assert.notStrictEqual(doesNotExist.spawnfile, 'does-not-exist'); +doesNotExist.on('error', common.mustNotCall()); +doesNotExist.on('exit', common.mustCall((code, signal) => { + assert.strictEqual(signal, null); + + if (common.isWindows) + assert.strictEqual(code, 1); // Exit code of cmd.exe + else + assert.strictEqual(code, 127); // Exit code of /bin/sh +})); + +// Verify that passing arguments works +const echo = cp.spawn('echo', ['foo'], { + encoding: 'utf8', + shell: true +}); +let echoOutput = ''; + +assert.strictEqual(echo.spawnargs[echo.spawnargs.length - 1].replace(/"/g, ''), + 'echo foo'); +echo.stdout.on('data', (data) => { + echoOutput += data; +}); +echo.on('close', common.mustCall((code, signal) => { + assert.strictEqual(echoOutput.trim(), 'foo'); +})); + +// Verify that shell features can be used +const cmd = 'echo bar | cat'; +const command = cp.spawn(cmd, { + encoding: 'utf8', + shell: true +}); +let commandOutput = ''; + +command.stdout.on('data', (data) => { + commandOutput += data; +}); +command.on('close', common.mustCall((code, signal) => { + assert.strictEqual(commandOutput.trim(), 'bar'); +})); + +// Verify that the environment is properly inherited +// edited to change `-pe` flag to `-p` +const env = cp.spawn(`"${common.isWindows ? process.execPath : '$NODE'}" -p process.env.BAZ`, { + env: { ...process.env, BAZ: 'buzz', NODE: process.execPath }, + encoding: 'utf8', + shell: true +}); +let envOutput = ''; + +env.stdout.on('data', (data) => { + envOutput += data; +}); +env.on('close', common.mustCall((code, signal) => { + assert.strictEqual(envOutput.trim(), 'buzz'); +})); From 17d61af5c173a9851bb21a2dc7cde6902536a489 Mon Sep 17 00:00:00 2001 From: pfg Date: Tue, 28 Jan 2025 20:40:33 -0800 Subject: [PATCH 02/12] test-child-process-spawn-argv0.js --- src/bun.js/api/bun/subprocess.zig | 9 +++---- src/js/node/child_process.ts | 8 +++--- .../test-child-process-spawn-argv0.js | 27 +++++++++++++++++++ 3 files changed, 33 insertions(+), 11 deletions(-) create mode 100644 test/js/node/test/parallel/test-child-process-spawn-argv0.js diff --git a/src/bun.js/api/bun/subprocess.zig b/src/bun.js/api/bun/subprocess.zig index 5a30592c49fc07..34f2950594fe86 100644 --- a/src/bun.js/api/bun/subprocess.zig +++ b/src/bun.js/api/bun/subprocess.zig @@ -1715,7 +1715,7 @@ pub const Subprocess = struct { // This is split into a separate function to conserve stack space. // On Windows, a single path buffer can take 64 KB. - fn getArgv0(globalThis: *JSC.JSGlobalObject, PATH: []const u8, cwd: []const u8, argv0: ?[*:0]const u8, first_cmd: JSValue, allocator: std.mem.Allocator) bun.JSError!struct { + fn getArgv0(globalThis: *JSC.JSGlobalObject, PATH: []const u8, cwd: []const u8, pretend_argv0: ?[*:0]const u8, first_cmd: JSValue, allocator: std.mem.Allocator) bun.JSError!struct { argv0: [:0]const u8, arg0: [:0]u8, } { @@ -1727,10 +1727,7 @@ pub const Subprocess = struct { var actual_argv0: [:0]const u8 = ""; - const argv0_to_use: []const u8 = if (argv0) |_argv0| - bun.sliceTo(_argv0, 0) - else - arg0.slice(); + const argv0_to_use: []const u8 = arg0.slice(); // This mimicks libuv's behavior, which mimicks execvpe // Only resolve from $PATH when the command is not an absolute path @@ -1757,7 +1754,7 @@ pub const Subprocess = struct { return .{ .argv0 = actual_argv0, - .arg0 = try allocator.dupeZ(u8, arg0.slice()), + .arg0 = if (pretend_argv0) |p| try allocator.dupeZ(u8, bun.sliceTo(p, 0)) else try allocator.dupeZ(u8, arg0.slice()), }; } diff --git a/src/js/node/child_process.ts b/src/js/node/child_process.ts index a94dfa43b18c0b..891fca1092de32 100644 --- a/src/js/node/child_process.ts +++ b/src/js/node/child_process.ts @@ -572,6 +572,7 @@ function spawnSync(file, args, options) { stdio: bunStdio, windowsVerbatimArguments: options.windowsVerbatimArguments, windowsHide: options.windowsHide, + argv0: options.argv0, }); } catch (err) { error = err; @@ -959,11 +960,7 @@ function normalizeSpawnArguments(file, args, options) { } // Handle argv0 - if (typeof options.argv0 === "string") { - ArrayPrototypeUnshift.$call(args, options.argv0); - } else { - ArrayPrototypeUnshift.$call(args, file); - } + ArrayPrototypeUnshift.$call(args, file); const env = options.env || process.env; const envPairs = {}; @@ -1010,6 +1007,7 @@ function normalizeSpawnArguments(file, args, options) { file, windowsHide: !!options.windowsHide, windowsVerbatimArguments: !!windowsVerbatimArguments, + argv0: options.argv0, }; } diff --git a/test/js/node/test/parallel/test-child-process-spawn-argv0.js b/test/js/node/test/parallel/test-child-process-spawn-argv0.js new file mode 100644 index 00000000000000..334896949623be --- /dev/null +++ b/test/js/node/test/parallel/test-child-process-spawn-argv0.js @@ -0,0 +1,27 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const cp = require('child_process'); + +// This test spawns itself with an argument to indicate when it is a child to +// easily and portably print the value of argv[0] +if (process.argv[2] === 'child') { + console.log(process.argv0); + return; +} + +const noArgv0 = cp.spawnSync(process.execPath, [__filename, 'child']); +assert.strictEqual(noArgv0.stdout.toString().trim(), process.execPath); + +const withArgv0 = cp.spawnSync(process.execPath, [__filename, 'child'], + { argv0: 'withArgv0' }); +assert.strictEqual(withArgv0.stdout.toString().trim(), 'withArgv0'); + +assert.throws(() => { + cp.spawnSync(process.execPath, [__filename, 'child'], { argv0: [] }); +}, { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: 'The "options.argv0" property must be of type string.' + + common.invalidArgTypeHelper([]) +}); From 63c7d45b08a2ee6fbec861227cb6e33c5ca2aae4 Mon Sep 17 00:00:00 2001 From: pfg Date: Wed, 29 Jan 2025 13:13:12 -0800 Subject: [PATCH 03/12] [] fix argv0 with spawn --- src/js/node/child_process.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/js/node/child_process.ts b/src/js/node/child_process.ts index 891fca1092de32..26db0889d5e4c0 100644 --- a/src/js/node/child_process.ts +++ b/src/js/node/child_process.ts @@ -1242,8 +1242,6 @@ class ChildProcess extends EventEmitter { validateString(options.file, "options.file"); // NOTE: This is confusing... So node allows you to pass a file name // But also allows you to pass a command in the args and it should execute - // To add another layer of confusion, they also give the option to pass an explicit "argv0" - // which overrides the actual command of the spawned process... var file; file = this.spawnfile = options.file; @@ -1257,7 +1255,7 @@ class ChildProcess extends EventEmitter { const stdio = options.stdio || ["pipe", "pipe", "pipe"]; const bunStdio = getBunStdioFromOptions(stdio); - const argv0 = file || options.argv0; + const argv0 = options.argv0 || file; const has_ipc = $isJSArray(stdio) && stdio[3] === "ipc"; var env = options.envPairs || process.env; From 1d869873367b186e56d1bd93c8347b9aa1724a0c Mon Sep 17 00:00:00 2001 From: pfg Date: Wed, 29 Jan 2025 12:56:19 -0800 Subject: [PATCH 04/12] test-child-process-send-cb.js --- src/bun.js/javascript.zig | 2 +- src/js/node/child_process.ts | 2 +- .../parallel/test-child-process-send-cb.js | 19 +++++++++++++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 test/js/node/test/parallel/test-child-process-send-cb.js diff --git a/src/bun.js/javascript.zig b/src/bun.js/javascript.zig index a5c975c2e9f40d..c5be7b43a986c3 100644 --- a/src/bun.js/javascript.zig +++ b/src/bun.js/javascript.zig @@ -470,7 +470,7 @@ pub fn Bun__Process__send_(globalObject: *JSGlobalObject, callFrame: *JSC.CallFr if (good) { if (callback.isFunction()) { - Bun__Process__queueNextTick1(zigGlobal, callback, .zero); + Bun__Process__queueNextTick1(zigGlobal, callback, .null); } } else { const ex = globalObject.createTypeErrorInstance("process.send() failed", .{}); diff --git a/src/js/node/child_process.ts b/src/js/node/child_process.ts index 26db0889d5e4c0..b04ad85c711deb 100644 --- a/src/js/node/child_process.ts +++ b/src/js/node/child_process.ts @@ -1348,7 +1348,7 @@ class ChildProcess extends EventEmitter { // Bun does not handle handles yet try { this.#handle.send(message); - if (callback) process.nextTick(callback); + if (callback) process.nextTick(callback, null); return true; } catch (error) { if (callback) { diff --git a/test/js/node/test/parallel/test-child-process-send-cb.js b/test/js/node/test/parallel/test-child-process-send-cb.js new file mode 100644 index 00000000000000..daecd722533f57 --- /dev/null +++ b/test/js/node/test/parallel/test-child-process-send-cb.js @@ -0,0 +1,19 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const fork = require('child_process').fork; + +if (process.argv[2] === 'child') { + process.send('ok', common.mustCall((err) => { + assert.strictEqual(err, null); + })); +} else { + const child = fork(process.argv[1], ['child']); + child.on('message', common.mustCall((message) => { + assert.strictEqual(message, 'ok'); + })); + child.on('exit', common.mustCall((exitCode, signalCode) => { + assert.strictEqual(exitCode, 0); + assert.strictEqual(signalCode, null); + })); +} From d8dc51f78b1408a1bb6a45d1d64254baae3d9b67 Mon Sep 17 00:00:00 2001 From: pfg Date: Wed, 29 Jan 2025 16:38:37 -0800 Subject: [PATCH 05/12] test-child-process-net-reuseport.js --- test/js/node/test/common/net.js | 23 ++++++++++++ .../test-child-process-net-reuseport.js | 35 +++++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 test/js/node/test/common/net.js create mode 100644 test/js/node/test/parallel/test-child-process-net-reuseport.js diff --git a/test/js/node/test/common/net.js b/test/js/node/test/common/net.js new file mode 100644 index 00000000000000..84eddd0966ed56 --- /dev/null +++ b/test/js/node/test/common/net.js @@ -0,0 +1,23 @@ +'use strict'; +const net = require('net'); + +const options = { port: 0, reusePort: true }; + +function checkSupportReusePort() { + return new Promise((resolve, reject) => { + const server = net.createServer().listen(options); + server.on('listening', () => { + server.close(resolve); + }); + server.on('error', (err) => { + console.log('The `reusePort` option is not supported:', err.message); + server.close(); + reject(err); + }); + }); +} + +module.exports = { + checkSupportReusePort, + options, +}; diff --git a/test/js/node/test/parallel/test-child-process-net-reuseport.js b/test/js/node/test/parallel/test-child-process-net-reuseport.js new file mode 100644 index 00000000000000..52309edd1b3be1 --- /dev/null +++ b/test/js/node/test/parallel/test-child-process-net-reuseport.js @@ -0,0 +1,35 @@ +'use strict'; +const common = require('../common'); +const { checkSupportReusePort, options } = require('../common/net'); +const assert = require('assert'); +const child_process = require('child_process'); +const net = require('net'); + +if (!process.env.isWorker) { + checkSupportReusePort().then(() => { + const server = net.createServer(); + server.listen(options, common.mustCall(() => { + const port = server.address().port; + const workerOptions = { env: { ...process.env, isWorker: 1, port } }; + let count = 2; + for (let i = 0; i < 2; i++) { + const worker = child_process.fork(__filename, workerOptions); + worker.on('exit', common.mustCall((code) => { + assert.strictEqual(code, 0); + if (--count === 0) { + server.close(); + } + })); + } + })); + }, () => { + common.skip('The `reusePort` option is not supported'); + }); + return; +} + +const server = net.createServer(); + +server.listen({ ...options, port: +process.env.port }, common.mustCall(() => { + server.close(); +})); From 97da14e0ad3b08509dd9ad0e7760e7d5a986d4b7 Mon Sep 17 00:00:00 2001 From: pfg Date: Wed, 29 Jan 2025 18:46:57 -0800 Subject: [PATCH 06/12] test-child-process-double-pipe.js --- .../test-child-process-double-pipe.js | 122 ++++++++++++++++++ 1 file changed, 122 insertions(+) create mode 100644 test/js/node/test/parallel/test-child-process-double-pipe.js diff --git a/test/js/node/test/parallel/test-child-process-double-pipe.js b/test/js/node/test/parallel/test-child-process-double-pipe.js new file mode 100644 index 00000000000000..7a432d3892acfc --- /dev/null +++ b/test/js/node/test/parallel/test-child-process-double-pipe.js @@ -0,0 +1,122 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +'use strict'; +const { + isWindows, + mustCall, + mustCallAtLeast, +} = require('../common'); +const assert = require('assert'); +const os = require('os'); +const spawn = require('child_process').spawn; +const debug = require('util').debuglog('test'); + +// We're trying to reproduce: +// $ echo "hello\nnode\nand\nworld" | grep o | sed s/o/a/ + +let grep, sed, echo; + +if (isWindows) { + grep = spawn('grep', ['--binary', 'o']); + sed = spawn('sed', ['--binary', 's/o/O/']); + echo = spawn('cmd.exe', + ['/c', 'echo', 'hello&&', 'echo', + 'node&&', 'echo', 'and&&', 'echo', 'world']); +} else { + grep = spawn('grep', ['o']); + sed = spawn('sed', ['s/o/O/']); + echo = spawn('echo', ['hello\nnode\nand\nworld\n']); +} + +// If the spawn function leaks file descriptors to subprocesses, grep and sed +// hang. +// This happens when calling pipe(2) and then forgetting to set the +// FD_CLOEXEC flag on the resulting file descriptors. +// +// This test checks child processes exit, meaning they don't hang like +// explained above. + + +// pipe echo | grep +echo.stdout.on('data', mustCallAtLeast((data) => { + debug(`grep stdin write ${data.length}`); + if (!grep.stdin.write(data)) { + echo.stdout.pause(); + } +})); + +// TODO(@jasnell): This does not appear to ever be +// emitted. It's not clear if it is necessary. +grep.stdin.on('drain', (data) => { + echo.stdout.resume(); +}); + +// Propagate end from echo to grep +echo.stdout.on('end', mustCall((code) => { + grep.stdin.end(); +})); + +echo.on('exit', mustCall(() => { + debug('echo exit'); +})); + +grep.on('exit', mustCall(() => { + debug('grep exit'); +})); + +sed.on('exit', mustCall(() => { + debug('sed exit'); +})); + + +// pipe grep | sed +grep.stdout.on('data', mustCallAtLeast((data) => { + debug(`grep stdout ${data.length}`); + if (!sed.stdin.write(data)) { + grep.stdout.pause(); + } +})); + +// TODO(@jasnell): This does not appear to ever be +// emitted. It's not clear if it is necessary. +sed.stdin.on('drain', (data) => { + grep.stdout.resume(); +}); + +// Propagate end from grep to sed +grep.stdout.on('end', mustCall((code) => { + debug('grep stdout end'); + sed.stdin.end(); +})); + + +let result = ''; + +// print sed's output +sed.stdout.on('data', mustCallAtLeast((data) => { + result += data.toString('utf8', 0, data.length); + debug(data); +})); + +sed.stdout.on('end', mustCall((code) => { + assert.strictEqual(result, `hellO${os.EOL}nOde${os.EOL}wOrld${os.EOL}`); +})); From 31a369aabe9b6ec72b9eb10478576569ef2c2203 Mon Sep 17 00:00:00 2001 From: pfg Date: Wed, 29 Jan 2025 19:30:30 -0800 Subject: [PATCH 07/12] test-child-process-stdout-ipc.js --- src/js/node/child_process.ts | 2 +- .../parallel/test-child-process-stdout-ipc.js | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 test/js/node/test/parallel/test-child-process-stdout-ipc.js diff --git a/src/js/node/child_process.ts b/src/js/node/child_process.ts index b04ad85c711deb..78b8b1e1357cfb 100644 --- a/src/js/node/child_process.ts +++ b/src/js/node/child_process.ts @@ -1257,7 +1257,7 @@ class ChildProcess extends EventEmitter { const bunStdio = getBunStdioFromOptions(stdio); const argv0 = options.argv0 || file; - const has_ipc = $isJSArray(stdio) && stdio[3] === "ipc"; + const has_ipc = $isJSArray(stdio) && stdio.includes("ipc"); var env = options.envPairs || process.env; const detachedOption = options.detached; diff --git a/test/js/node/test/parallel/test-child-process-stdout-ipc.js b/test/js/node/test/parallel/test-child-process-stdout-ipc.js new file mode 100644 index 00000000000000..c916be95b73fc0 --- /dev/null +++ b/test/js/node/test/parallel/test-child-process-stdout-ipc.js @@ -0,0 +1,18 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +const spawn = require('child_process').spawn; + +if (process.argv[2] === 'child') { + process.send('hahah'); + return; +} + +const proc = spawn(process.execPath, [__filename, 'child'], { + stdio: ['inherit', 'ipc', 'inherit'] +}); + +proc.on('exit', common.mustCall(function(code) { + assert.strictEqual(code, 0); +})); From 9b32f17183f30a2fd53d740973e75561a64e1f6b Mon Sep 17 00:00:00 2001 From: pfg Date: Wed, 29 Jan 2025 19:34:36 -0800 Subject: [PATCH 08/12] test-pipe-unref.js --- test/js/node/test/parallel/test-pipe-unref.js | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 test/js/node/test/parallel/test-pipe-unref.js diff --git a/test/js/node/test/parallel/test-pipe-unref.js b/test/js/node/test/parallel/test-pipe-unref.js new file mode 100644 index 00000000000000..78419a1d77076e --- /dev/null +++ b/test/js/node/test/parallel/test-pipe-unref.js @@ -0,0 +1,29 @@ +'use strict'; +const common = require('../common'); +const net = require('net'); +const assert = require('assert'); +const { fork } = require('child_process'); + +// This test should end immediately after `unref` is called +// The pipe will stay open as Node.js completes, thus run in a child process +// so that tmpdir can be cleaned up. + +const tmpdir = require('../common/tmpdir'); + +if (process.argv[2] !== 'child') { + // Parent + tmpdir.refresh(); + + // Run test + const child = fork(__filename, ['child'], { stdio: 'inherit' }); + child.on('exit', common.mustCall(function(code) { + assert.strictEqual(code, 0); + })); + + return; +} + +// Child +const s = net.Server(); +s.listen(common.PIPE); +s.unref(); From 6f3b08079c54bc05ee57ed5d670e0af27c6d9e47 Mon Sep 17 00:00:00 2001 From: pfg Date: Wed, 29 Jan 2025 19:39:44 -0800 Subject: [PATCH 09/12] test-child-process-send-after-close.js --- src/bun.js/api/bun/subprocess.zig | 2 +- .../test-child-process-send-after-close.js | 31 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 test/js/node/test/parallel/test-child-process-send-after-close.js diff --git a/src/bun.js/api/bun/subprocess.zig b/src/bun.js/api/bun/subprocess.zig index 34f2950594fe86..b8c46a9ed81b39 100644 --- a/src/bun.js/api/bun/subprocess.zig +++ b/src/bun.js/api/bun/subprocess.zig @@ -721,7 +721,7 @@ pub const Subprocess = struct { IPClog("Subprocess#doSend", .{}); const ipc_data = &(this.ipc_data orelse { if (this.hasExited()) { - return global.throw("Subprocess.send() cannot be used after the process has exited.", .{}); + return global.ERR_IPC_CHANNEL_CLOSED("Subprocess.send() cannot be used after the process has exited.", .{}).throw(); } else { return global.throw("Subprocess.send() can only be used if an IPC channel is open.", .{}); } diff --git a/test/js/node/test/parallel/test-child-process-send-after-close.js b/test/js/node/test/parallel/test-child-process-send-after-close.js new file mode 100644 index 00000000000000..01e4d34d1f7bd6 --- /dev/null +++ b/test/js/node/test/parallel/test-child-process-send-after-close.js @@ -0,0 +1,31 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const cp = require('child_process'); +const fixtures = require('../common/fixtures'); + +const fixture = fixtures.path('empty.js'); +const child = cp.fork(fixture); + +child.on('close', common.mustCall((code, signal) => { + assert.strictEqual(code, 0); + assert.strictEqual(signal, null); + + const testError = common.expectsError({ + name: 'Error', + // message: 'Channel closed', + code: 'ERR_IPC_CHANNEL_CLOSED' + }, 2); + + child.on('error', testError); + + { + const result = child.send('ping'); + assert.strictEqual(result, false); + } + + { + const result = child.send('pong', testError); + assert.strictEqual(result, false); + } +})); From 47647aa6b45b75048e3a43de3c6723c28fd3d354 Mon Sep 17 00:00:00 2001 From: pfg Date: Wed, 5 Feb 2025 17:08:36 -0800 Subject: [PATCH 10/12] test-child-process-cwd.js --- src/bun.js/api/bun/process.zig | 2 +- src/js/node/child_process.ts | 98 +++++++++-------- .../test/parallel/test-child-process-cwd.js | 102 ++++++++++++++++++ 3 files changed, 158 insertions(+), 44 deletions(-) create mode 100644 test/js/node/test/parallel/test-child-process-cwd.js diff --git a/src/bun.js/api/bun/process.zig b/src/bun.js/api/bun/process.zig index b6fed57c8a502d..2556fbf986a0c3 100644 --- a/src/bun.js/api/bun/process.zig +++ b/src/bun.js/api/bun/process.zig @@ -1236,7 +1236,7 @@ pub fn spawnProcessPosix( } if (options.cwd.len > 0) { - actions.chdir(options.cwd) catch return error.ChangingDirectoryFailed; + try actions.chdir(options.cwd); } var spawned = PosixSpawnResult{}; var extra_fds = std.ArrayList(bun.FileDescriptor).init(bun.default_allocator); diff --git a/src/js/node/child_process.ts b/src/js/node/child_process.ts index 78b8b1e1357cfb..d2674701ff3bb5 100644 --- a/src/js/node/child_process.ts +++ b/src/js/node/child_process.ts @@ -1266,55 +1266,63 @@ class ChildProcess extends EventEmitter { const stdioCount = stdio.length; const hasSocketsToEagerlyLoad = stdioCount >= 3; - this.#handle = Bun.spawn({ - cmd: spawnargs, - stdio: bunStdio, - cwd: options.cwd || undefined, - env: env, - detached: typeof detachedOption !== "undefined" ? !!detachedOption : false, - onExit: (handle, exitCode, signalCode, err) => { - this.#handle = handle; - this.pid = this.#handle.pid; - $debug("ChildProcess: onExit", exitCode, signalCode, err, this.pid); - - if (hasSocketsToEagerlyLoad) { - process.nextTick(() => { - this.stdio; - $debug("ChildProcess: onExit", exitCode, signalCode, err, this.pid); - }); - } + try { + this.#handle = Bun.spawn({ + cmd: spawnargs, + stdio: bunStdio, + cwd: options.cwd || undefined, + env: env, + detached: typeof detachedOption !== "undefined" ? !!detachedOption : false, + onExit: (handle, exitCode, signalCode, err) => { + this.#handle = handle; + this.pid = this.#handle.pid; + $debug("ChildProcess: onExit", exitCode, signalCode, err, this.pid); + + if (hasSocketsToEagerlyLoad) { + process.nextTick(() => { + this.stdio; + $debug("ChildProcess: onExit", exitCode, signalCode, err, this.pid); + }); + } - process.nextTick( - (exitCode, signalCode, err) => this.#handleOnExit(exitCode, signalCode, err), - exitCode, - signalCode, - err, - ); - }, - lazy: true, - ipc: has_ipc ? this.#emitIpcMessage.bind(this) : undefined, - onDisconnect: has_ipc ? ok => this.#disconnect(ok) : undefined, - serialization, - argv0, - windowsHide: !!options.windowsHide, - windowsVerbatimArguments: !!options.windowsVerbatimArguments, - }); - this.pid = this.#handle.pid; + process.nextTick( + (exitCode, signalCode, err) => this.#handleOnExit(exitCode, signalCode, err), + exitCode, + signalCode, + err, + ); + }, + lazy: true, + ipc: has_ipc ? this.#emitIpcMessage.bind(this) : undefined, + onDisconnect: has_ipc ? ok => this.#disconnect(ok) : undefined, + serialization, + argv0, + windowsHide: !!options.windowsHide, + windowsVerbatimArguments: !!options.windowsVerbatimArguments, + }); + this.pid = this.#handle.pid; - $debug("ChildProcess: spawn", this.pid, spawnargs); + $debug("ChildProcess: spawn", this.pid, spawnargs); - onSpawnNT(this); + onSpawnNT(this); - if (has_ipc) { - this.send = this.#send; - this.disconnect = this.#disconnect; - if (options[kFromNode]) this.#closesNeeded += 1; - } + if (has_ipc) { + this.send = this.#send; + this.disconnect = this.#disconnect; + if (options[kFromNode]) this.#closesNeeded += 1; + } - if (hasSocketsToEagerlyLoad) { - for (let item of this.stdio) { - item?.ref?.(); + if (hasSocketsToEagerlyLoad) { + for (let item of this.stdio) { + item?.ref?.(); + } } + } catch (ex) { + this.#handle = null; + process.nextTick(() => { + this.emit("error", ex); + this.emit("close", (ex as SystemError).errno ?? -1); + }); } } @@ -1595,6 +1603,10 @@ class ShimmedStdioOutStream extends EventEmitter { destroy() { return this; } + + setEncoding() { + return this; + } } //------------------------------------------------------------------------------ diff --git a/test/js/node/test/parallel/test-child-process-cwd.js b/test/js/node/test/parallel/test-child-process-cwd.js new file mode 100644 index 00000000000000..e876361b167ffb --- /dev/null +++ b/test/js/node/test/parallel/test-child-process-cwd.js @@ -0,0 +1,102 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +'use strict'; + +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +const assert = require('assert'); +const { spawn } = require('child_process'); + +// Spawns 'pwd' with given options, then test +// - whether the child pid is undefined or number, +// - whether the exit code equals expectCode, +// - optionally whether the trimmed stdout result matches expectData +function testCwd(options, expectPidType, expectCode = 0, expectData) { + const child = spawn(...common.pwdCommand, options); + + assert.strictEqual(typeof child.pid, expectPidType); + + child.stdout.setEncoding('utf8'); + + // No need to assert callback since `data` is asserted. + let data = ''; + child.stdout.on('data', function(chunk) { + data += chunk; + }); + + // Can't assert callback, as stayed in to API: + // _The 'exit' event may or may not fire after an error has occurred._ + child.on('exit', function(code, signal) { + assert.strictEqual(code, expectCode); + }); + + child.on('close', common.mustCall(function() { + if (expectData) { + // In Windows, compare without considering case + if (common.isWindows) { + assert.strictEqual(data.trim().toLowerCase(), expectData.toLowerCase()); + } else { + assert.strictEqual(data.trim(), expectData); + } + } + })); + + return child; +} + + +// Assume does-not-exist doesn't exist, expect exitCode=-1 and errno=ENOENT +{ + testCwd({ cwd: 'does-not-exist' }, 'undefined', -1) + .on('error', common.mustCall(function(e) { + assert.strictEqual(e.code, 'ENOENT'); + })); +} + +{ + assert.throws(() => { + testCwd({ + cwd: new URL('http://example.com/'), + }, 'number', 0, tmpdir.path); + }, /The URL must be of scheme file/); + + if (process.platform !== 'win32') { + assert.throws(() => { + testCwd({ + cwd: new URL('file://host/dev/null'), + }, 'number', 0, tmpdir.path); + }, /File URL host must be "localhost" or empty on/); + } +} + +// Assume these exist, and 'pwd' gives us the right directory back +testCwd({ cwd: tmpdir.path }, 'number', 0, tmpdir.path); +const shouldExistDir = common.isWindows ? process.env.windir : '/dev'; +testCwd({ cwd: shouldExistDir }, 'number', 0, shouldExistDir); +testCwd({ cwd: tmpdir.fileURL() }, 'number', 0, tmpdir.path); + +// Spawn() shouldn't try to chdir() to invalid arg, so this should just work +testCwd({ cwd: '' }, 'number'); +testCwd({ cwd: undefined }, 'number'); +testCwd({ cwd: null }, 'number'); From c0782be54c42b6f808eed159fc688f67c936c107 Mon Sep 17 00:00:00 2001 From: pfg Date: Thu, 6 Feb 2025 14:09:02 -0800 Subject: [PATCH 11/12] [ ] pass child_process-node.test.js --- src/js/node/child_process.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/js/node/child_process.ts b/src/js/node/child_process.ts index d2674701ff3bb5..1c9eec4ab15737 100644 --- a/src/js/node/child_process.ts +++ b/src/js/node/child_process.ts @@ -1318,6 +1318,7 @@ class ChildProcess extends EventEmitter { } } } catch (ex) { + if ((ex as Error).name !== "SystemError") throw ex; this.#handle = null; process.nextTick(() => { this.emit("error", ex); From a33b98ae78a513c8ac494a8278d3ed257c106dab Mon Sep 17 00:00:00 2001 From: pfg Date: Wed, 5 Feb 2025 20:39:20 -0800 Subject: [PATCH 12/12] test-child-process-detached.js --- .../parallel/test-child-process-detached.js | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 test/js/node/test/parallel/test-child-process-detached.js diff --git a/test/js/node/test/parallel/test-child-process-detached.js b/test/js/node/test/parallel/test-child-process-detached.js new file mode 100644 index 00000000000000..7249a4b0e41e58 --- /dev/null +++ b/test/js/node/test/parallel/test-child-process-detached.js @@ -0,0 +1,43 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +'use strict'; +require('../common'); +const assert = require('assert'); +const fixtures = require('../common/fixtures'); + +const spawn = require('child_process').spawn; +const childPath = fixtures.path('parent-process-nonpersistent.js'); +let persistentPid = -1; + +const child = spawn(process.execPath, [ childPath ]); + +child.stdout.on('data', function(data) { + persistentPid = parseInt(data, 10); +}); + +process.on('exit', function() { + assert.notStrictEqual(persistentPid, -1); + assert.throws(function() { + process.kill(child.pid); + }, /^Error: kill ESRCH$|ESRCH/); + process.kill(persistentPid); +});