From 046a2e1774ddfc4fafb373a5b6bcd152a6772126 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9C=A8=E6=9D=89?= Date: Wed, 8 Jun 2022 14:26:05 +0800 Subject: [PATCH 1/6] child_process: avoid repeated calls to `normalizeSpawnArguments` --- lib/child_process.js | 76 +++++++++++++------- test/parallel/test-child-process-execfile.js | 18 ++++- 2 files changed, 66 insertions(+), 28 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 605bd14660f521..59259ff96a9225 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -32,6 +32,7 @@ const { ArrayPrototypeSort, ArrayPrototypeSplice, ArrayPrototypeUnshift, + ArrayPrototypePushApply, NumberIsInteger, ObjectAssign, ObjectDefineProperty, @@ -249,6 +250,45 @@ ObjectDefineProperty(exec, promisify.custom, { value: customPromiseExecFunction(exec) }); +function normalizeExecFileArgs(file, args, options, callback) { + if (ArrayIsArray(args)) { + args = ArrayPrototypeSlice(args); + } else if (args != null && typeof args === 'object') { + callback = options; + options = args; + args = null; + } else if (typeof args === 'function') { + callback = args; + options = null; + args = null; + } + + if (args == null) { + args = []; + } + + if (typeof options === 'function') { + callback = options; + } else if (options != null) { + validateObject(options, 'options'); + } + + if (options == null) { + options = kEmptyObject; + } + + if (callback != null) { + validateFunction(callback, 'callback'); + } + + // Validate argv0, if present. + if (options.argv0 != null) { + validateString(options.argv0, 'options.argv0'); + } + + return { file, args, options, callback }; +} + /** * Spawns the specified file as a shell. * @param {string} file @@ -274,27 +314,8 @@ ObjectDefineProperty(exec, promisify.custom, { * ) => any} [callback] * @returns {ChildProcess} */ -function execFile(file, args = [], options, callback) { - if (args != null && typeof args === 'object' && !ArrayIsArray(args)) { - callback = options; - options = args; - args = null; - } else if (typeof args === 'function') { - callback = args; - options = null; - args = null; - } - - if (typeof options === 'function') { - callback = options; - options = null; - } else if (options != null) { - validateObject(options, 'options'); - } - - if (callback != null) { - validateFunction(callback, 'callback'); - } +function execFile(file, args, options, callback) { + ({ file, args, options, callback } = normalizeExecFileArgs(file, args, options, callback)); options = { encoding: 'utf8', @@ -824,7 +845,7 @@ function checkExecSyncError(ret, args, cmd) { /** * Spawns a file as a shell synchronously. - * @param {string} command + * @param {string} file * @param {string[]} [args] * @param {{ * cwd?: string; @@ -842,17 +863,18 @@ function checkExecSyncError(ret, args, cmd) { * }} [options] * @returns {Buffer | string} */ -function execFileSync(command, args, options) { - options = normalizeSpawnArguments(command, args, options); +function execFileSync(file, args, options) { + ({ file, args, options } = normalizeExecFileArgs(file, args, options)); const inheritStderr = !options.stdio; - const ret = spawnSync(options.file, - ArrayPrototypeSlice(options.args, 1), options); + const ret = spawnSync(file, args, options); if (inheritStderr && ret.stderr) process.stderr.write(ret.stderr); - const err = checkExecSyncError(ret, options.args, undefined); + const errArgs = [options.argv0 || file]; + ArrayPrototypePushApply(errArgs, args); + const err = checkExecSyncError(ret, errArgs); if (err) throw err; diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index 40cb8cd3afab01..d640930c065624 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -2,7 +2,7 @@ const common = require('../common'); const assert = require('assert'); -const execFile = require('child_process').execFile; +const { execFile, execFileSync } = require('child_process'); const { getEventListeners } = require('events'); const { getSystemErrorName } = require('util'); const fixtures = require('../common/fixtures'); @@ -99,3 +99,19 @@ const execOpts = { encoding: 'utf8', shell: true }; }); execFile(process.execPath, [fixture, 0], { signal }, callback); } + +// Verify the execFile() stdout is the same as execFileSync(). +{ + const file = 'echo'; + const args = ['foo', 'bar']; + + // Test with and without `{ shell: true }` + [null, { shell: true }].forEach((options) => { + const execFileSyncStdout = execFileSync(file, args, options); + assert.deepStrictEqual(execFileSyncStdout, Buffer.from('foo bar\n')); + + execFile(file, args, options, common.mustCall((_, stdout) => { + assert.deepStrictEqual(Buffer.from(stdout), execFileSyncStdout); + })); + }); +} From 746c01b3458a40ef949e4d8659590317b8ab0d55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9C=A8=E6=9D=89?= Date: Sun, 10 Jul 2022 19:30:24 +0800 Subject: [PATCH 2/6] fix tests on windows --- test/parallel/test-child-process-execfile.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index d640930c065624..d7afa78765a432 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -106,9 +106,9 @@ const execOpts = { encoding: 'utf8', shell: true }; const args = ['foo', 'bar']; // Test with and without `{ shell: true }` - [null, { shell: true }].forEach((options) => { + [{ shell: common.isWindows }, { shell: true }].forEach((options) => { const execFileSyncStdout = execFileSync(file, args, options); - assert.deepStrictEqual(execFileSyncStdout, Buffer.from('foo bar\n')); + assert.strictEqual(execFileSyncStdout.toString().trim(), 'foo bar'); execFile(file, args, options, common.mustCall((_, stdout) => { assert.deepStrictEqual(Buffer.from(stdout), execFileSyncStdout); From 87a08a203100f61ebaffc9f63dc7f429157ad2ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9C=A8=E6=9D=89?= Date: Sun, 10 Jul 2022 19:50:52 +0800 Subject: [PATCH 3/6] Update test/parallel/test-child-process-execfile.js Co-authored-by: Antoine du Hamel --- test/parallel/test-child-process-execfile.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index d7afa78765a432..0965b1f52d4141 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -106,7 +106,11 @@ const execOpts = { encoding: 'utf8', shell: true }; const args = ['foo', 'bar']; // Test with and without `{ shell: true }` - [{ shell: common.isWindows }, { shell: true }].forEach((options) => { + [ + // Skipping shell-less test on Windows because … + ...(common.isWindows ? [] : [{ encoding: 'utf8' }]), + { shell: true, encoding: 'utf8' }, +].forEach((options) => { const execFileSyncStdout = execFileSync(file, args, options); assert.strictEqual(execFileSyncStdout.toString().trim(), 'foo bar'); From 174f811862637ed411a28f1509c683c8c69253cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9C=A8=E6=9D=89?= Date: Sun, 10 Jul 2022 19:58:48 +0800 Subject: [PATCH 4/6] prefer os.eol --- test/parallel/test-child-process-execfile.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index 0965b1f52d4141..2897f491427f33 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -6,6 +6,7 @@ const { execFile, execFileSync } = require('child_process'); const { getEventListeners } = require('events'); const { getSystemErrorName } = require('util'); const fixtures = require('../common/fixtures'); +const os = require('os'); const fixture = fixtures.path('exit.js'); const echoFixture = fixtures.path('echo.js'); @@ -107,12 +108,12 @@ const execOpts = { encoding: 'utf8', shell: true }; // Test with and without `{ shell: true }` [ - // Skipping shell-less test on Windows because … - ...(common.isWindows ? [] : [{ encoding: 'utf8' }]), - { shell: true, encoding: 'utf8' }, -].forEach((options) => { + // Skipping shell-less test on Windows because … + ...(common.isWindows ? [] : [null]), + { shell: true }, + ].forEach((options) => { const execFileSyncStdout = execFileSync(file, args, options); - assert.strictEqual(execFileSyncStdout.toString().trim(), 'foo bar'); + assert.deepStrictEqual(execFileSyncStdout, Buffer.from(`foo bar${os.EOL}`)); execFile(file, args, options, common.mustCall((_, stdout) => { assert.deepStrictEqual(Buffer.from(stdout), execFileSyncStdout); From db3cfb23839a307c4a8ae612d346f869a17ed297 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9C=A8=E6=9D=89?= Date: Sun, 10 Jul 2022 20:08:00 +0800 Subject: [PATCH 5/6] add comments --- test/parallel/test-child-process-execfile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index 2897f491427f33..c9efce2d0de11a 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -108,7 +108,7 @@ const execOpts = { encoding: 'utf8', shell: true }; // Test with and without `{ shell: true }` [ - // Skipping shell-less test on Windows because … + // Skipping shell-less test on Windows because its echo command is a shell built-in command. ...(common.isWindows ? [] : [null]), { shell: true }, ].forEach((options) => { From fd8412591554a51ebea2f18c1d9e8b1502f3c448 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9C=A8=E6=9D=89?= Date: Sun, 10 Jul 2022 20:13:59 +0800 Subject: [PATCH 6/6] Update test/parallel/test-child-process-execfile.js Co-authored-by: Antoine du Hamel --- test/parallel/test-child-process-execfile.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index c9efce2d0de11a..f51c1729c9ac79 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -109,14 +109,14 @@ const execOpts = { encoding: 'utf8', shell: true }; // Test with and without `{ shell: true }` [ // Skipping shell-less test on Windows because its echo command is a shell built-in command. - ...(common.isWindows ? [] : [null]), - { shell: true }, + ...(common.isWindows ? [] : [{ encoding: 'utf8' }]), + { shell: true, encoding: 'utf8' }, ].forEach((options) => { const execFileSyncStdout = execFileSync(file, args, options); - assert.deepStrictEqual(execFileSyncStdout, Buffer.from(`foo bar${os.EOL}`)); + assert.strictEqual(execFileSyncStdout, `foo bar${os.EOL}`); execFile(file, args, options, common.mustCall((_, stdout) => { - assert.deepStrictEqual(Buffer.from(stdout), execFileSyncStdout); + assert.strictEqual(stdout, execFileSyncStdout); })); }); }