From c1c98fb1594ed7695918ca2f0d66eb19b80fb7ea Mon Sep 17 00:00:00 2001 From: dr-js Date: Mon, 25 Jan 2021 11:43:20 +0800 Subject: [PATCH 1/2] doc,child_process: `pid` can be `undefined` when `ENOENT` From the code `nodejs@8` and up should behave the same: github.com/nodejs/node/blame/v8.17.0/lib/internal/child_process.js#L290 And a short test snippet: ```js const { spawn } = require('child_process') const subProcess = spawn('non-exist-command') subProcess.on('error', (error) => console.warn('mute Unhandled "error" event:', error)) console.log('- pid:', subProcess.pid) process.nextTick(() => console.log('- after error emit')) console.log('== end of test ==') ``` Note: the sync spawn result `pid` currently do not follow this pattern. Co-authored-by: Rich Trott PR-URL: https://github.com/nodejs/node/pull/37014 Reviewed-By: Antoine du Hamel --- doc/api/child_process.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/api/child_process.md b/doc/api/child_process.md index 8d3186cbc4090d..ee8f0b4f0bc939 100644 --- a/doc/api/child_process.md +++ b/doc/api/child_process.md @@ -1253,9 +1253,11 @@ does not indicate that the child process has been terminated. added: v0.1.90 --> -* {integer} +* {integer|undefined} -Returns the process identifier (PID) of the child process. +Returns the process identifier (PID) of the child process. If the child process +fails to spawn due to errors, then the value is `undefined` and `error` is +emitted. ```js const { spawn } = require('child_process'); From 1e0cfa3361cfb24341096eb907ae47b6b19fcd8f Mon Sep 17 00:00:00 2001 From: dr-js Date: Mon, 25 Jan 2021 11:44:05 +0800 Subject: [PATCH 2/2] test,child_process: add check for `subProcess.pid` Note: this only add checks for async spawn, as the sync spawn do not return a `subProcess`. PR-URL: https://github.com/nodejs/node/pull/37014 Reviewed-By: Antoine du Hamel --- test/parallel/test-child-process-cwd.js | 17 ++++++++++------- test/parallel/test-child-process-exec-error.js | 14 +++++++++----- test/parallel/test-child-process-spawn-error.js | 3 +++ 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/test/parallel/test-child-process-cwd.js b/test/parallel/test-child-process-cwd.js index fb782234539db4..232f1d0f3d5316 100644 --- a/test/parallel/test-child-process-cwd.js +++ b/test/parallel/test-child-process-cwd.js @@ -28,11 +28,14 @@ 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, expectCode = 0, 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. @@ -57,18 +60,18 @@ function testCwd(options, expectCode = 0, expectData) { // Assume does-not-exist doesn't exist, expect exitCode=-1 and errno=ENOENT { - testCwd({ cwd: 'does-not-exist' }, -1) + testCwd({ cwd: 'does-not-exist' }, 'undefined', -1) .on('error', common.mustCall(function(e) { assert.strictEqual(e.code, 'ENOENT'); })); } // Assume these exist, and 'pwd' gives us the right directory back -testCwd({ cwd: tmpdir.path }, 0, tmpdir.path); +testCwd({ cwd: tmpdir.path }, 'number', 0, tmpdir.path); const shouldExistDir = common.isWindows ? process.env.windir : '/dev'; -testCwd({ cwd: shouldExistDir }, 0, shouldExistDir); +testCwd({ cwd: shouldExistDir }, 'number', 0, shouldExistDir); // Spawn() shouldn't try to chdir() to invalid arg, so this should just work -testCwd({ cwd: '' }); -testCwd({ cwd: undefined }); -testCwd({ cwd: null }); +testCwd({ cwd: '' }, 'number'); +testCwd({ cwd: undefined }, 'number'); +testCwd({ cwd: null }, 'number'); diff --git a/test/parallel/test-child-process-exec-error.js b/test/parallel/test-child-process-exec-error.js index eaddeb6614e921..cd45f3071c2920 100644 --- a/test/parallel/test-child-process-exec-error.js +++ b/test/parallel/test-child-process-exec-error.js @@ -24,17 +24,21 @@ const common = require('../common'); const assert = require('assert'); const child_process = require('child_process'); -function test(fn, code) { - fn('does-not-exist', common.mustCall(function(err) { +function test(fn, code, expectPidType = 'number') { + const child = fn('does-not-exist', common.mustCall(function(err) { assert.strictEqual(err.code, code); assert(err.cmd.includes('does-not-exist')); })); + + assert.strictEqual(typeof child.pid, expectPidType); } +// With `shell: true`, expect pid (of the shell) if (common.isWindows) { - test(child_process.exec, 1); // Exit code of cmd.exe + test(child_process.exec, 1, 'number'); // Exit code of cmd.exe } else { - test(child_process.exec, 127); // Exit code of /bin/sh + test(child_process.exec, 127, 'number'); // Exit code of /bin/sh } -test(child_process.execFile, 'ENOENT'); +// With `shell: false`, expect no pid +test(child_process.execFile, 'ENOENT', 'undefined'); diff --git a/test/parallel/test-child-process-spawn-error.js b/test/parallel/test-child-process-spawn-error.js index a3464a505d4f7c..ed1c8fac9f97ed 100644 --- a/test/parallel/test-child-process-spawn-error.js +++ b/test/parallel/test-child-process-spawn-error.js @@ -41,6 +41,9 @@ assert.strictEqual(enoentChild.stdio[0], enoentChild.stdin); assert.strictEqual(enoentChild.stdio[1], enoentChild.stdout); assert.strictEqual(enoentChild.stdio[2], enoentChild.stderr); +// Verify pid is not assigned. +assert.strictEqual(enoentChild.pid, undefined); + enoentChild.on('spawn', common.mustNotCall()); enoentChild.on('error', common.mustCall(function(err) {