From df21bda4d469ff022d9fea0739568f211ed5f8e1 Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Wed, 23 Aug 2017 15:20:54 -0700 Subject: [PATCH 01/15] errors, child_process: use more internal/errors codes --- lib/child_process.js | 49 ++--- ...child-process-fork-stdio-string-variant.js | 6 +- .../parallel/test-child-process-fork-stdio.js | 5 +- .../test-child-process-spawn-typeerror.js | 59 +++--- .../test-child-process-spawnsync-input.js | 7 +- ...ild-process-spawnsync-validation-errors.js | 177 ++++++++---------- 6 files changed, 155 insertions(+), 148 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 7b11d953d56559..43f49c05d7dc91 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -29,6 +29,7 @@ const { createPromise, const debug = util.debuglog('child_process'); const { Buffer } = require('buffer'); const { Pipe, constants: PipeConstants } = process.binding('pipe_wrap'); +const errors = require('internal/errors'); const { errname } = process.binding('uv'); const child_process = require('internal/child_process'); const { @@ -46,7 +47,7 @@ function stdioStringToArray(option) { case 'inherit': return [option, option, option, 'ipc']; default: - throw new TypeError('Incorrect value of stdio option: ' + option); + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'stdio', option); } } @@ -63,7 +64,7 @@ exports.fork = function(modulePath /*, args, options*/) { if (pos < arguments.length && arguments[pos] != null) { if (typeof arguments[pos] !== 'object') { - throw new TypeError('Incorrect value of args option'); + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', ('arguments[' + pos + ']'), arguments[pos]); } options = util._extend({}, arguments[pos++]); @@ -91,7 +92,7 @@ exports.fork = function(modulePath /*, args, options*/) { options.stdio = options.silent ? stdioStringToArray('pipe') : stdioStringToArray('inherit'); } else if (options.stdio.indexOf('ipc') === -1) { - throw new TypeError('Forked processes must have an IPC channel'); + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'options.stdio', options.stdio); } options.execPath = options.execPath || process.execPath; @@ -195,7 +196,7 @@ exports.execFile = function(file /*, args, options, callback*/) { } if (!callback && pos < arguments.length && arguments[pos] != null) { - throw new TypeError('Incorrect value of args option'); + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'args' , arguments); } // Validate the timeout, if present. @@ -322,6 +323,7 @@ exports.execFile = function(file /*, args, options, callback*/) { stdoutLen += encoding ? Buffer.byteLength(chunk, encoding) : chunk.length; if (stdoutLen > options.maxBuffer) { + // TODO helpful to convert to ERR_BUFFER_OUT_OF_BOUNDS? can't specify stderr/stdout. ex = new Error('stdout maxBuffer exceeded'); kill(); } else if (encoding) { @@ -340,6 +342,7 @@ exports.execFile = function(file /*, args, options, callback*/) { stderrLen += encoding ? Buffer.byteLength(chunk, encoding) : chunk.length; if (stderrLen > options.maxBuffer) { + // TODO helpful to convert to ERR_BUFFER_OUT_OF_BOUNDS? can't specify stderr/stdout. ex = new Error('stderr maxBuffer exceeded'); kill(); } else if (encoding) { @@ -377,13 +380,13 @@ function _convertCustomFds(options) { function normalizeSpawnArguments(file, args, options) { if (typeof file !== 'string' || file.length === 0) - throw new TypeError('"file" argument must be a non-empty string'); + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'file', file); if (Array.isArray(args)) { args = args.slice(0); } else if (args !== undefined && (args === null || typeof args !== 'object')) { - throw new TypeError('Incorrect value of args option'); + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'args', args); } else { options = args; args = []; @@ -392,41 +395,41 @@ function normalizeSpawnArguments(file, args, options) { if (options === undefined) options = {}; else if (options === null || typeof options !== 'object') - throw new TypeError('"options" argument must be an object'); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'object', options); // Validate the cwd, if present. if (options.cwd != null && typeof options.cwd !== 'string') { - throw new TypeError('"cwd" must be a string'); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.cwd', 'string', options.cwd); } // Validate detached, if present. if (options.detached != null && typeof options.detached !== 'boolean') { - throw new TypeError('"detached" must be a boolean'); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.detached', 'boolean', options.detached); } // Validate the uid, if present. if (options.uid != null && !Number.isInteger(options.uid)) { - throw new TypeError('"uid" must be an integer'); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.uid', 'integer', options.uid); } // Validate the gid, if present. if (options.gid != null && !Number.isInteger(options.gid)) { - throw new TypeError('"gid" must be an integer'); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.gid', 'integer', options.gid); } // Validate the shell, if present. if (options.shell != null && typeof options.shell !== 'boolean' && typeof options.shell !== 'string') { - throw new TypeError('"shell" must be a boolean or string'); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.shell', ['boolean', 'string'], options.shell); } // Validate argv0, if present. if (options.argv0 != null && typeof options.argv0 !== 'string') { - throw new TypeError('"argv0" must be a string'); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.argv0', 'string', options.argv0); } // Validate windowsHide, if present. @@ -438,7 +441,7 @@ function normalizeSpawnArguments(file, args, options) { // Validate windowsVerbatimArguments, if present. if (options.windowsVerbatimArguments != null && typeof options.windowsVerbatimArguments !== 'boolean') { - throw new TypeError('"windowsVerbatimArguments" must be a boolean'); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.windowsVerbatimArguments', 'boolean', options.windowsVerbatimArguments); } // Make a shallow copy so we don't clobber the user's options object. @@ -549,10 +552,10 @@ function spawnSync(/*file, args, options*/) { } else if (typeof input === 'string') { pipe.input = Buffer.from(input, options.encoding); } else { - throw new TypeError(util.format( - 'stdio[%d] should be Buffer, Uint8Array or string not %s', - i, - typeof input)); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', + ('options.stdio[' + i + ']'), + ['Buffer', 'Uint8Array', 'string'], + input); } } } @@ -620,14 +623,18 @@ exports.execSync = execSync; function validateTimeout(timeout) { if (timeout != null && !(Number.isInteger(timeout) && timeout >= 0)) { - throw new TypeError('"timeout" must be an unsigned integer'); + // TODO should this be a RangeError? + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'options.timeout', timeout); + //"timeout" must be an unsigned integer'); } } function validateMaxBuffer(maxBuffer) { if (maxBuffer != null && !(typeof maxBuffer === 'number' && maxBuffer >= 0)) { - throw new TypeError('"maxBuffer" must be a positive number'); + // TODO should this be a RangeError? + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'options.maxBuffer', maxBuffer); + //'"maxBuffer" must be a positive number'); } } @@ -636,6 +643,6 @@ function sanitizeKillSignal(killSignal) { if (typeof killSignal === 'string' || typeof killSignal === 'number') { return convertToValidSignal(killSignal); } else if (killSignal != null) { - throw new TypeError('"killSignal" must be a string or number'); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.killSignal', ['string', 'number'], killSignal); } } diff --git a/test/parallel/test-child-process-fork-stdio-string-variant.js b/test/parallel/test-child-process-fork-stdio-string-variant.js index cdeb15975a7b5a..0d8b522d6d9129 100644 --- a/test/parallel/test-child-process-fork-stdio-string-variant.js +++ b/test/parallel/test-child-process-fork-stdio-string-variant.js @@ -10,11 +10,13 @@ const fork = require('child_process').fork; const fixtures = require('../common/fixtures'); const childScript = fixtures.path('child-process-spawn-node'); -const errorRegexp = /^TypeError: Incorrect value of stdio option:/; const malFormedOpts = { stdio: '33' }; const payload = { hello: 'world' }; -assert.throws(() => fork(childScript, malFormedOpts), errorRegexp); +const expectedError = + common.expectsError({ code: 'ERR_INVALID_OPT_VALUE', type: TypeError }); + +assert.throws(() => fork(childScript, malFormedOpts), expectedError); function test(stringVariant) { const child = fork(childScript, { stdio: stringVariant }); diff --git a/test/parallel/test-child-process-fork-stdio.js b/test/parallel/test-child-process-fork-stdio.js index be0bdf19c35832..16c98bbe85aa92 100644 --- a/test/parallel/test-child-process-fork-stdio.js +++ b/test/parallel/test-child-process-fork-stdio.js @@ -19,9 +19,12 @@ if (process.argv[2] === 'child') { process.send(data); }); } else { + const expectedError = + common.expectsError({ code: 'ERR_INVALID_OPT_VALUE', type: TypeError }); + assert.throws(() => { cp.fork(__filename, { stdio: ['pipe', 'pipe', 'pipe', 'pipe'] }); - }, /Forked processes must have an IPC channel/); + }, expectedError); let ipc = ''; let stderr = ''; diff --git a/test/parallel/test-child-process-spawn-typeerror.js b/test/parallel/test-child-process-spawn-typeerror.js index d8ae4c758ce3a1..83808811d048d6 100644 --- a/test/parallel/test-child-process-spawn-typeerror.js +++ b/test/parallel/test-child-process-spawn-typeerror.js @@ -26,13 +26,16 @@ const { spawn, fork, execFile } = require('child_process'); const fixtures = require('../common/fixtures'); const cmd = common.isWindows ? 'rundll32' : 'ls'; const invalidcmd = 'hopefully_you_dont_have_this_on_your_machine'; -const invalidArgsMsg = /Incorrect value of args option/; const invalidOptionsMsg = /"options" argument must be an object/; -const invalidFileMsg = - /^TypeError: "file" argument must be a non-empty string$/; const empty = fixtures.path('empty.js'); +const invalidOptValueError = + common.expectsError({ code: 'ERR_INVALID_OPT_VALUE', type: TypeError }, 20); + +const invalidArgTypeError = + common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 4); + assert.throws(function() { const child = spawn(invalidcmd, 'this is not an array'); child.on('error', common.mustNotCall()); @@ -58,32 +61,32 @@ assert.doesNotThrow(function() { // verify that invalid argument combinations throw assert.throws(function() { spawn(); -}, invalidFileMsg); +}, invalidOptValueError); assert.throws(function() { spawn(''); -}, invalidFileMsg); +}, invalidOptValueError); assert.throws(function() { - const file = { toString() { throw new Error('foo'); } }; + const file = { toString() { return null; } }; spawn(file); -}, invalidFileMsg); +}, invalidOptValueError); assert.throws(function() { spawn(cmd, null); -}, invalidArgsMsg); +}, invalidOptValueError); assert.throws(function() { spawn(cmd, true); -}, invalidArgsMsg); +}, invalidOptValueError); assert.throws(function() { spawn(cmd, [], null); -}, invalidOptionsMsg); +}, invalidArgTypeError); assert.throws(function() { spawn(cmd, [], 1); -}, invalidOptionsMsg); +}, invalidArgTypeError); // Argument types for combinatorics const a = []; @@ -107,11 +110,11 @@ assert.doesNotThrow(function() { spawn(cmd, o); }); assert.doesNotThrow(function() { spawn(cmd, u, o); }); assert.doesNotThrow(function() { spawn(cmd, a, u); }); -assert.throws(function() { spawn(cmd, n, o); }, TypeError); -assert.throws(function() { spawn(cmd, a, n); }, TypeError); +assert.throws(function() { spawn(cmd, n, o); }, invalidOptValueError); +assert.throws(function() { spawn(cmd, a, n); }, invalidArgTypeError); -assert.throws(function() { spawn(cmd, s); }, TypeError); -assert.throws(function() { spawn(cmd, a, s); }, TypeError); +assert.throws(function() { spawn(cmd, s); }, invalidOptValueError); +assert.throws(function() { spawn(cmd, a, s); }, invalidArgTypeError); // verify that execFile has same argument parsing behaviour as spawn @@ -160,17 +163,17 @@ assert.doesNotThrow(function() { execFile(cmd, c, n); }); // string is invalid in arg position (this may seem strange, but is // consistent across node API, cf. `net.createServer('not options', 'not // callback')` -assert.throws(function() { execFile(cmd, s, o, c); }, TypeError); -assert.throws(function() { execFile(cmd, a, s, c); }, TypeError); -assert.throws(function() { execFile(cmd, a, o, s); }, TypeError); -assert.throws(function() { execFile(cmd, a, s); }, TypeError); -assert.throws(function() { execFile(cmd, o, s); }, TypeError); -assert.throws(function() { execFile(cmd, u, u, s); }, TypeError); -assert.throws(function() { execFile(cmd, n, n, s); }, TypeError); -assert.throws(function() { execFile(cmd, a, u, s); }, TypeError); -assert.throws(function() { execFile(cmd, a, n, s); }, TypeError); -assert.throws(function() { execFile(cmd, u, o, s); }, TypeError); -assert.throws(function() { execFile(cmd, n, o, s); }, TypeError); +assert.throws(function() { execFile(cmd, s, o, c); }, invalidOptValueError); +assert.throws(function() { execFile(cmd, a, s, c); }, invalidOptValueError); +assert.throws(function() { execFile(cmd, a, o, s); }, invalidOptValueError); +assert.throws(function() { execFile(cmd, a, s); }, invalidOptValueError); +assert.throws(function() { execFile(cmd, o, s); }, invalidOptValueError); +assert.throws(function() { execFile(cmd, u, u, s); }, invalidOptValueError); +assert.throws(function() { execFile(cmd, n, n, s); }, invalidOptValueError); +assert.throws(function() { execFile(cmd, a, u, s); }, invalidOptValueError); +assert.throws(function() { execFile(cmd, a, n, s); }, invalidOptValueError); +assert.throws(function() { execFile(cmd, u, o, s); }, invalidOptValueError); +assert.throws(function() { execFile(cmd, n, o, s); }, invalidOptValueError); assert.doesNotThrow(function() { execFile(cmd, c, s); }); @@ -192,5 +195,5 @@ assert.doesNotThrow(function() { fork(empty, n, n); }); assert.doesNotThrow(function() { fork(empty, n, o); }); assert.doesNotThrow(function() { fork(empty, a, n); }); -assert.throws(function() { fork(empty, s); }, TypeError); -assert.throws(function() { fork(empty, a, s); }, TypeError); +assert.throws(function() { fork(empty, s); }, invalidOptValueError); +assert.throws(function() { fork(empty, a, s); }, invalidOptValueError); diff --git a/test/parallel/test-child-process-spawnsync-input.js b/test/parallel/test-child-process-spawnsync-input.js index b44c2363847f09..ddf3f49c19002a 100644 --- a/test/parallel/test-child-process-spawnsync-input.js +++ b/test/parallel/test-child-process-spawnsync-input.js @@ -20,7 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); @@ -76,9 +76,12 @@ let options = { input: 1234 }; +const expectedError = + common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 1); + assert.throws(function() { spawnSync('cat', [], options); -}, /TypeError:.*should be Buffer, Uint8Array or string not number/); +}, expectedError); options = { diff --git a/test/parallel/test-child-process-spawnsync-validation-errors.js b/test/parallel/test-child-process-spawnsync-validation-errors.js index 0a1cc2ca577307..4a362b06a400b1 100644 --- a/test/parallel/test-child-process-spawnsync-validation-errors.js +++ b/test/parallel/test-child-process-spawnsync-validation-errors.js @@ -4,6 +4,12 @@ const assert = require('assert'); const spawnSync = require('child_process').spawnSync; const signals = process.binding('constants').os.signals; +const invalidOptValueError = + common.expectsError({ code: 'ERR_INVALID_OPT_VALUE', type: TypeError }, 20); + +const invalidArgTypeError = + common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 56); + function pass(option, value) { // Run the command with the specified option. Since it's not a real command, // spawnSync() should run successfully but return an ENOENT error. @@ -20,108 +26,96 @@ function fail(option, value, message) { { // Validate the cwd option - const err = /^TypeError: "cwd" must be a string$/; - pass('cwd', undefined); pass('cwd', null); pass('cwd', __dirname); - fail('cwd', 0, err); - fail('cwd', 1, err); - fail('cwd', true, err); - fail('cwd', false, err); - fail('cwd', [], err); - fail('cwd', {}, err); - fail('cwd', common.mustNotCall(), err); + fail('cwd', 0, invalidArgTypeError); + fail('cwd', 1, invalidArgTypeError); + fail('cwd', true, invalidArgTypeError); + fail('cwd', false, invalidArgTypeError); + fail('cwd', [], invalidArgTypeError); + fail('cwd', {}, invalidArgTypeError); + fail('cwd', common.mustNotCall(), invalidArgTypeError); } { // Validate the detached option - const err = /^TypeError: "detached" must be a boolean$/; - pass('detached', undefined); pass('detached', null); pass('detached', true); pass('detached', false); - fail('detached', 0, err); - fail('detached', 1, err); - fail('detached', __dirname, err); - fail('detached', [], err); - fail('detached', {}, err); - fail('detached', common.mustNotCall(), err); + fail('detached', 0, invalidArgTypeError); + fail('detached', 1, invalidArgTypeError); + fail('detached', __dirname, invalidArgTypeError); + fail('detached', [], invalidArgTypeError); + fail('detached', {}, invalidArgTypeError); + fail('detached', common.mustNotCall(), invalidArgTypeError); } if (!common.isWindows) { { // Validate the uid option if (process.getuid() !== 0) { - const err = /^TypeError: "uid" must be an integer$/; - pass('uid', undefined); pass('uid', null); pass('uid', process.getuid()); - fail('uid', __dirname, err); - fail('uid', true, err); - fail('uid', false, err); - fail('uid', [], err); - fail('uid', {}, err); - fail('uid', common.mustNotCall(), err); - fail('uid', NaN, err); - fail('uid', Infinity, err); - fail('uid', 3.1, err); - fail('uid', -3.1, err); + fail('uid', __dirname, invalidArgTypeError); + fail('uid', true, invalidArgTypeError); + fail('uid', false, invalidArgTypeError); + fail('uid', [], invalidArgTypeError); + fail('uid', {}, invalidArgTypeError); + fail('uid', common.mustNotCall(), invalidArgTypeError); + fail('uid', NaN, invalidArgTypeError); + fail('uid', Infinity, invalidArgTypeError); + fail('uid', 3.1, invalidArgTypeError); + fail('uid', -3.1, invalidArgTypeError); } } { // Validate the gid option if (process.getgid() !== 0) { - const err = /^TypeError: "gid" must be an integer$/; - pass('gid', undefined); pass('gid', null); pass('gid', process.getgid()); - fail('gid', __dirname, err); - fail('gid', true, err); - fail('gid', false, err); - fail('gid', [], err); - fail('gid', {}, err); - fail('gid', common.mustNotCall(), err); - fail('gid', NaN, err); - fail('gid', Infinity, err); - fail('gid', 3.1, err); - fail('gid', -3.1, err); + fail('gid', __dirname, invalidArgTypeError); + fail('gid', true, invalidArgTypeError); + fail('gid', false, invalidArgTypeError); + fail('gid', [], invalidArgTypeError); + fail('gid', {}, invalidArgTypeError); + fail('gid', common.mustNotCall(), invalidArgTypeError); + fail('gid', NaN, invalidArgTypeError); + fail('gid', Infinity, invalidArgTypeError); + fail('gid', 3.1, invalidArgTypeError); + fail('gid', -3.1, invalidArgTypeError); } } } { // Validate the shell option - const err = /^TypeError: "shell" must be a boolean or string$/; - pass('shell', undefined); pass('shell', null); pass('shell', false); - fail('shell', 0, err); - fail('shell', 1, err); - fail('shell', [], err); - fail('shell', {}, err); - fail('shell', common.mustNotCall(), err); + fail('shell', 0, invalidArgTypeError); + fail('shell', 1, invalidArgTypeError); + fail('shell', [], invalidArgTypeError); + fail('shell', {}, invalidArgTypeError); + fail('shell', common.mustNotCall(), invalidArgTypeError); } { // Validate the argv0 option - const err = /^TypeError: "argv0" must be a string$/; - pass('argv0', undefined); pass('argv0', null); pass('argv0', 'myArgv0'); - fail('argv0', 0, err); - fail('argv0', 1, err); - fail('argv0', true, err); - fail('argv0', false, err); - fail('argv0', [], err); - fail('argv0', {}, err); - fail('argv0', common.mustNotCall(), err); + fail('argv0', 0, invalidArgTypeError); + fail('argv0', 1, invalidArgTypeError); + fail('argv0', true, invalidArgTypeError); + fail('argv0', false, invalidArgTypeError); + fail('argv0', [], invalidArgTypeError); + fail('argv0', {}, invalidArgTypeError); + fail('argv0', common.mustNotCall(), invalidArgTypeError); } { @@ -142,44 +136,40 @@ if (!common.isWindows) { { // Validate the windowsVerbatimArguments option - const err = /^TypeError: "windowsVerbatimArguments" must be a boolean$/; - pass('windowsVerbatimArguments', undefined); pass('windowsVerbatimArguments', null); pass('windowsVerbatimArguments', true); pass('windowsVerbatimArguments', false); - fail('windowsVerbatimArguments', 0, err); - fail('windowsVerbatimArguments', 1, err); - fail('windowsVerbatimArguments', __dirname, err); - fail('windowsVerbatimArguments', [], err); - fail('windowsVerbatimArguments', {}, err); - fail('windowsVerbatimArguments', common.mustNotCall(), err); + fail('windowsVerbatimArguments', 0, invalidArgTypeError); + fail('windowsVerbatimArguments', 1, invalidArgTypeError); + fail('windowsVerbatimArguments', __dirname, invalidArgTypeError); + fail('windowsVerbatimArguments', [], invalidArgTypeError); + fail('windowsVerbatimArguments', {}, invalidArgTypeError); + fail('windowsVerbatimArguments', common.mustNotCall(), invalidArgTypeError); } { // Validate the timeout option - const err = /^TypeError: "timeout" must be an unsigned integer$/; - pass('timeout', undefined); pass('timeout', null); pass('timeout', 1); pass('timeout', 0); - fail('timeout', -1, err); - fail('timeout', true, err); - fail('timeout', false, err); - fail('timeout', __dirname, err); - fail('timeout', [], err); - fail('timeout', {}, err); - fail('timeout', common.mustNotCall(), err); - fail('timeout', NaN, err); - fail('timeout', Infinity, err); - fail('timeout', 3.1, err); - fail('timeout', -3.1, err); + fail('timeout', -1, invalidOptValueError); + fail('timeout', true, invalidOptValueError); + fail('timeout', false, invalidOptValueError); + fail('timeout', __dirname, invalidOptValueError); + fail('timeout', [], invalidOptValueError); + fail('timeout', {}, invalidOptValueError); + fail('timeout', common.mustNotCall(), invalidOptValueError); + fail('timeout', NaN, invalidOptValueError); + fail('timeout', Infinity, invalidOptValueError); + fail('timeout', 3.1, invalidOptValueError); + fail('timeout', -3.1, invalidOptValueError); } { // Validate the maxBuffer option - const err = /^TypeError: "maxBuffer" must be a positive number$/; + //const err = /^TypeError: "maxBuffer" must be a positive number$/; pass('maxBuffer', undefined); pass('maxBuffer', null); @@ -187,20 +177,19 @@ if (!common.isWindows) { pass('maxBuffer', 0); pass('maxBuffer', Infinity); pass('maxBuffer', 3.14); - fail('maxBuffer', -1, err); - fail('maxBuffer', NaN, err); - fail('maxBuffer', -Infinity, err); - fail('maxBuffer', true, err); - fail('maxBuffer', false, err); - fail('maxBuffer', __dirname, err); - fail('maxBuffer', [], err); - fail('maxBuffer', {}, err); - fail('maxBuffer', common.mustNotCall(), err); + fail('maxBuffer', -1, invalidOptValueError); + fail('maxBuffer', NaN, invalidOptValueError); + fail('maxBuffer', -Infinity, invalidOptValueError); + fail('maxBuffer', true, invalidOptValueError); + fail('maxBuffer', false, invalidOptValueError); + fail('maxBuffer', __dirname, invalidOptValueError); + fail('maxBuffer', [], invalidOptValueError); + fail('maxBuffer', {}, invalidOptValueError); + fail('maxBuffer', common.mustNotCall(), invalidOptValueError); } { // Validate the killSignal option - const typeErr = /^TypeError: "killSignal" must be a string or number$/; const unknownSignalErr = common.expectsError({ code: 'ERR_UNKNOWN_SIGNAL', type: TypeError }, 17); @@ -208,11 +197,11 @@ if (!common.isWindows) { pass('killSignal', null); pass('killSignal', 'SIGKILL'); fail('killSignal', 'SIGNOTAVALIDSIGNALNAME', unknownSignalErr); - fail('killSignal', true, typeErr); - fail('killSignal', false, typeErr); - fail('killSignal', [], typeErr); - fail('killSignal', {}, typeErr); - fail('killSignal', common.mustNotCall(), typeErr); + fail('killSignal', true, invalidArgTypeError); + fail('killSignal', false, invalidArgTypeError); + fail('killSignal', [], invalidArgTypeError); + fail('killSignal', {}, invalidArgTypeError); + fail('killSignal', common.mustNotCall(), invalidArgTypeError); // Invalid signal names and numbers should fail fail('killSignal', 500, unknownSignalErr); From d160c524ca9b7134b38bda393c377c5777d899b7 Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Wed, 23 Aug 2017 18:30:27 -0700 Subject: [PATCH 02/15] [squash] fix linter --- lib/child_process.js | 69 ++++++++++++++----- .../test-child-process-spawn-typeerror.js | 1 - 2 files changed, 53 insertions(+), 17 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 43f49c05d7dc91..2fac021bc3aa53 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -64,7 +64,9 @@ exports.fork = function(modulePath /*, args, options*/) { if (pos < arguments.length && arguments[pos] != null) { if (typeof arguments[pos] !== 'object') { - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', ('arguments[' + pos + ']'), arguments[pos]); + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', + ('arguments[' + pos + ']'), + arguments[pos]); } options = util._extend({}, arguments[pos++]); @@ -92,7 +94,9 @@ exports.fork = function(modulePath /*, args, options*/) { options.stdio = options.silent ? stdioStringToArray('pipe') : stdioStringToArray('inherit'); } else if (options.stdio.indexOf('ipc') === -1) { - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'options.stdio', options.stdio); + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', + 'options.stdio', + options.stdio); } options.execPath = options.execPath || process.execPath; @@ -196,7 +200,7 @@ exports.execFile = function(file /*, args, options, callback*/) { } if (!callback && pos < arguments.length && arguments[pos] != null) { - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'args' , arguments); + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'args', arguments); } // Validate the timeout, if present. @@ -323,7 +327,8 @@ exports.execFile = function(file /*, args, options, callback*/) { stdoutLen += encoding ? Buffer.byteLength(chunk, encoding) : chunk.length; if (stdoutLen > options.maxBuffer) { - // TODO helpful to convert to ERR_BUFFER_OUT_OF_BOUNDS? can't specify stderr/stdout. + // TODO helpful to convert to ERR_BUFFER_OUT_OF_BOUNDS? + // can't specify stderr/stdout with that new error. ex = new Error('stdout maxBuffer exceeded'); kill(); } else if (encoding) { @@ -342,7 +347,8 @@ exports.execFile = function(file /*, args, options, callback*/) { stderrLen += encoding ? Buffer.byteLength(chunk, encoding) : chunk.length; if (stderrLen > options.maxBuffer) { - // TODO helpful to convert to ERR_BUFFER_OUT_OF_BOUNDS? can't specify stderr/stdout. + // TODO helpful to convert to ERR_BUFFER_OUT_OF_BOUNDS? + // can't specify stderr/stdout with that new error. ex = new Error('stderr maxBuffer exceeded'); kill(); } else if (encoding) { @@ -395,41 +401,62 @@ function normalizeSpawnArguments(file, args, options) { if (options === undefined) options = {}; else if (options === null || typeof options !== 'object') - throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'object', options); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', + 'options', + 'object', + options); // Validate the cwd, if present. if (options.cwd != null && typeof options.cwd !== 'string') { - throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.cwd', 'string', options.cwd); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', + 'options.cwd', + 'string', + options.cwd); } // Validate detached, if present. if (options.detached != null && typeof options.detached !== 'boolean') { - throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.detached', 'boolean', options.detached); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', + 'options.detached', + 'boolean', + options.detached); } // Validate the uid, if present. if (options.uid != null && !Number.isInteger(options.uid)) { - throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.uid', 'integer', options.uid); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', + 'options.uid', + 'integer', + options.uid); } // Validate the gid, if present. if (options.gid != null && !Number.isInteger(options.gid)) { - throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.gid', 'integer', options.gid); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', + 'options.gid', + 'integer', + options.gid); } // Validate the shell, if present. if (options.shell != null && typeof options.shell !== 'boolean' && typeof options.shell !== 'string') { - throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.shell', ['boolean', 'string'], options.shell); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', + 'options.shell', + ['boolean', 'string'], + options.shell); } // Validate argv0, if present. if (options.argv0 != null && typeof options.argv0 !== 'string') { - throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.argv0', 'string', options.argv0); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', + 'options.argv0', + 'string', + options.argv0); } // Validate windowsHide, if present. @@ -441,7 +468,10 @@ function normalizeSpawnArguments(file, args, options) { // Validate windowsVerbatimArguments, if present. if (options.windowsVerbatimArguments != null && typeof options.windowsVerbatimArguments !== 'boolean') { - throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.windowsVerbatimArguments', 'boolean', options.windowsVerbatimArguments); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', + 'options.windowsVerbatimArguments', + 'boolean', + options.windowsVerbatimArguments); } // Make a shallow copy so we don't clobber the user's options object. @@ -624,7 +654,9 @@ exports.execSync = execSync; function validateTimeout(timeout) { if (timeout != null && !(Number.isInteger(timeout) && timeout >= 0)) { // TODO should this be a RangeError? - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'options.timeout', timeout); + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', + 'options.timeout', + timeout); //"timeout" must be an unsigned integer'); } } @@ -633,7 +665,9 @@ function validateTimeout(timeout) { function validateMaxBuffer(maxBuffer) { if (maxBuffer != null && !(typeof maxBuffer === 'number' && maxBuffer >= 0)) { // TODO should this be a RangeError? - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'options.maxBuffer', maxBuffer); + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', + 'options.maxBuffer', + maxBuffer); //'"maxBuffer" must be a positive number'); } } @@ -643,6 +677,9 @@ function sanitizeKillSignal(killSignal) { if (typeof killSignal === 'string' || typeof killSignal === 'number') { return convertToValidSignal(killSignal); } else if (killSignal != null) { - throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.killSignal', ['string', 'number'], killSignal); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', + 'options.killSignal', + ['string', 'number'], + killSignal); } } diff --git a/test/parallel/test-child-process-spawn-typeerror.js b/test/parallel/test-child-process-spawn-typeerror.js index 83808811d048d6..47031e5df6d6c2 100644 --- a/test/parallel/test-child-process-spawn-typeerror.js +++ b/test/parallel/test-child-process-spawn-typeerror.js @@ -26,7 +26,6 @@ const { spawn, fork, execFile } = require('child_process'); const fixtures = require('../common/fixtures'); const cmd = common.isWindows ? 'rundll32' : 'ls'; const invalidcmd = 'hopefully_you_dont_have_this_on_your_machine'; -const invalidOptionsMsg = /"options" argument must be an object/; const empty = fixtures.path('empty.js'); From bd60e810c802e8e7b4959c227e5befc5fec9f22c Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Wed, 23 Aug 2017 22:32:07 -0700 Subject: [PATCH 03/15] [squash] simply & rm one-off expectedError vars --- .../test-child-process-fork-stdio-string-variant.js | 7 +++---- test/parallel/test-child-process-fork-stdio.js | 9 +++------ test/parallel/test-child-process-spawnsync-input.js | 10 +++------- 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/test/parallel/test-child-process-fork-stdio-string-variant.js b/test/parallel/test-child-process-fork-stdio-string-variant.js index 0d8b522d6d9129..2bfcfc59e6343c 100644 --- a/test/parallel/test-child-process-fork-stdio-string-variant.js +++ b/test/parallel/test-child-process-fork-stdio-string-variant.js @@ -13,10 +13,9 @@ const childScript = fixtures.path('child-process-spawn-node'); const malFormedOpts = { stdio: '33' }; const payload = { hello: 'world' }; -const expectedError = - common.expectsError({ code: 'ERR_INVALID_OPT_VALUE', type: TypeError }); - -assert.throws(() => fork(childScript, malFormedOpts), expectedError); +common.expectsError( + () => fork(childScript, malFormedOpts), + { code: 'ERR_INVALID_OPT_VALUE', type: TypeError }); function test(stringVariant) { const child = fork(childScript, { stdio: stringVariant }); diff --git a/test/parallel/test-child-process-fork-stdio.js b/test/parallel/test-child-process-fork-stdio.js index 16c98bbe85aa92..2a14a5c9a835d3 100644 --- a/test/parallel/test-child-process-fork-stdio.js +++ b/test/parallel/test-child-process-fork-stdio.js @@ -19,12 +19,9 @@ if (process.argv[2] === 'child') { process.send(data); }); } else { - const expectedError = - common.expectsError({ code: 'ERR_INVALID_OPT_VALUE', type: TypeError }); - - assert.throws(() => { - cp.fork(__filename, { stdio: ['pipe', 'pipe', 'pipe', 'pipe'] }); - }, expectedError); + common.expectsError( + () => cp.fork(__filename, { stdio: ['pipe', 'pipe', 'pipe', 'pipe'] }), + { code: 'ERR_INVALID_OPT_VALUE', type: TypeError }); let ipc = ''; let stderr = ''; diff --git a/test/parallel/test-child-process-spawnsync-input.js b/test/parallel/test-child-process-spawnsync-input.js index ddf3f49c19002a..a2c636b37111fe 100644 --- a/test/parallel/test-child-process-spawnsync-input.js +++ b/test/parallel/test-child-process-spawnsync-input.js @@ -76,13 +76,9 @@ let options = { input: 1234 }; -const expectedError = - common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 1); - -assert.throws(function() { - spawnSync('cat', [], options); -}, expectedError); - +common.expectsError( + () => spawnSync('cat', [], options), + { code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 1); options = { input: 'hello world' From 4790d4679206385239b390c9518173a8dfbbfc7f Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Wed, 23 Aug 2017 22:43:24 -0700 Subject: [PATCH 04/15] [squash] Create new ERR_CHILD_PROCESS_IPC_REQUIRED --- doc/api/errors.md | 5 +++++ lib/child_process.js | 6 +++--- lib/internal/errors.js | 2 ++ test/parallel/test-child-process-fork-stdio.js | 2 +- 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index d7b5de448ff876..8e62d612f16e04 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -642,6 +642,11 @@ Node.js was unable to watch for the `SIGINT` signal. A child process was closed before the parent received a reply. + +### ERR_CHILD_PROCESS_IPC_REQUIRED + +Used when a child process is being forked without specifying an IPC channel. + ### ERR_CONSOLE_WRITABLE_STREAM diff --git a/lib/child_process.js b/lib/child_process.js index 2fac021bc3aa53..37fb165299270c 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -94,9 +94,9 @@ exports.fork = function(modulePath /*, args, options*/) { options.stdio = options.silent ? stdioStringToArray('pipe') : stdioStringToArray('inherit'); } else if (options.stdio.indexOf('ipc') === -1) { - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', - 'options.stdio', - options.stdio); + throw new errors.Error('ERR_CHILD_PROCESS_IPC_REQUIRED', + 'options.stdio', + options.stdio); } options.execPath = options.execPath || process.execPath; diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 619732df5d6fbb..6e835d333450cf 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -272,6 +272,8 @@ E('ERR_BUFFER_TOO_LARGE', `Cannot create a Buffer larger than 0x${kMaxLength.toString(16)} bytes`); E('ERR_CANNOT_WATCH_SIGINT', 'Cannot watch for SIGINT signals'); E('ERR_CHILD_CLOSED_BEFORE_REPLY', 'Child closed before reply received'); +E('ERR_CHILD_PROCESS_IPC_REQUIRED', + 'Forked processes must have an IPC channel'); E('ERR_CONSOLE_WRITABLE_STREAM', 'Console expects a writable stream instance for %s'); E('ERR_CPU_USAGE', 'Unable to obtain cpu usage %s'); diff --git a/test/parallel/test-child-process-fork-stdio.js b/test/parallel/test-child-process-fork-stdio.js index 2a14a5c9a835d3..d8e7abfe28944d 100644 --- a/test/parallel/test-child-process-fork-stdio.js +++ b/test/parallel/test-child-process-fork-stdio.js @@ -21,7 +21,7 @@ if (process.argv[2] === 'child') { } else { common.expectsError( () => cp.fork(__filename, { stdio: ['pipe', 'pipe', 'pipe', 'pipe'] }), - { code: 'ERR_INVALID_OPT_VALUE', type: TypeError }); + { code: 'ERR_CHILD_PROCESS_IPC_REQUIRED', type: Error }); let ipc = ''; let stderr = ''; From 876321af31feab3d18465eba4815ef9da0d45986 Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Thu, 24 Aug 2017 23:28:39 -0700 Subject: [PATCH 05/15] [squash] introduce new ERR_CHILD_PROCESS_STDIO_MAXBUFFER error --- doc/api/errors.md | 6 ++++++ lib/child_process.js | 10 ++++------ lib/internal/errors.js | 2 ++ test/parallel/test-child-process-exec-maxBuffer.js | 5 +++-- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 8e62d612f16e04..5d8faea5099a61 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -647,6 +647,12 @@ A child process was closed before the parent received a reply. Used when a child process is being forked without specifying an IPC channel. + +### ERR_CHILD_PROCESS_STDIO_MAXBUFFER + +Used when the main process is trying to read data from the child process' +STDERR / STDOUT, and the data's length is longer than the `maxBuffer` option. + ### ERR_CONSOLE_WRITABLE_STREAM diff --git a/lib/child_process.js b/lib/child_process.js index 37fb165299270c..cdea0bcace6c4d 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -327,9 +327,8 @@ exports.execFile = function(file /*, args, options, callback*/) { stdoutLen += encoding ? Buffer.byteLength(chunk, encoding) : chunk.length; if (stdoutLen > options.maxBuffer) { - // TODO helpful to convert to ERR_BUFFER_OUT_OF_BOUNDS? - // can't specify stderr/stdout with that new error. - ex = new Error('stdout maxBuffer exceeded'); + ex = new errors.RangeError('ERR_CHILD_PROCESS_STDIO_MAXBUFFER', + 'stdout'); kill(); } else if (encoding) { _stdout += chunk; @@ -347,9 +346,8 @@ exports.execFile = function(file /*, args, options, callback*/) { stderrLen += encoding ? Buffer.byteLength(chunk, encoding) : chunk.length; if (stderrLen > options.maxBuffer) { - // TODO helpful to convert to ERR_BUFFER_OUT_OF_BOUNDS? - // can't specify stderr/stdout with that new error. - ex = new Error('stderr maxBuffer exceeded'); + ex = new errors.RangeError('ERR_CHILD_PROCESS_STDIO_MAXBUFFER', + 'stderr'); kill(); } else if (encoding) { _stderr += chunk; diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 6e835d333450cf..b45348a1d7f8bf 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -274,6 +274,8 @@ E('ERR_CANNOT_WATCH_SIGINT', 'Cannot watch for SIGINT signals'); E('ERR_CHILD_CLOSED_BEFORE_REPLY', 'Child closed before reply received'); E('ERR_CHILD_PROCESS_IPC_REQUIRED', 'Forked processes must have an IPC channel'); +E('ERR_CHILD_PROCESS_STDIO_MAXBUFFER', + (name) => `${name} maxBuffer length exceeded`); E('ERR_CONSOLE_WRITABLE_STREAM', 'Console expects a writable stream instance for %s'); E('ERR_CPU_USAGE', 'Unable to obtain cpu usage %s'); diff --git a/test/parallel/test-child-process-exec-maxBuffer.js b/test/parallel/test-child-process-exec-maxBuffer.js index 6418f91765028a..4a65ccf5be7510 100644 --- a/test/parallel/test-child-process-exec-maxBuffer.js +++ b/test/parallel/test-child-process-exec-maxBuffer.js @@ -5,8 +5,9 @@ const cp = require('child_process'); function checkFactory(streamName) { return common.mustCall((err) => { - const message = `${streamName} maxBuffer exceeded`; - assert.strictEqual(err.message, message); + assert.strictEqual(err.message, `${streamName} maxBuffer length exceeded`); + assert(err instanceof RangeError); + assert.strictEqual(err.code, 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER'); }); } From 2d08284929996d114a5558ba461aedc7e06b254d Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Mon, 25 Sep 2017 20:49:51 -0400 Subject: [PATCH 06/15] [squash] fix lots of comments --- doc/api/errors.md | 2 +- lib/child_process.js | 18 ++++++++---------- .../test-child-process-spawnsync-input.js | 2 +- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 5d8faea5099a61..b814d7058e5621 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -650,7 +650,7 @@ Used when a child process is being forked without specifying an IPC channel. ### ERR_CHILD_PROCESS_STDIO_MAXBUFFER -Used when the main process is trying to read data from the child process' +Used when the main process is trying to read data from the child process's STDERR / STDOUT, and the data's length is longer than the `maxBuffer` option. diff --git a/lib/child_process.js b/lib/child_process.js index cdea0bcace6c4d..b40c514fd5461a 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -651,22 +651,20 @@ exports.execSync = execSync; function validateTimeout(timeout) { if (timeout != null && !(Number.isInteger(timeout) && timeout >= 0)) { - // TODO should this be a RangeError? - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', - 'options.timeout', - timeout); - //"timeout" must be an unsigned integer'); + throw new errors.RangeError('ERR_VALUE_OUT_OF_RANGE', + 'timeout', + 'an unsigned integer', + timeout); } } function validateMaxBuffer(maxBuffer) { if (maxBuffer != null && !(typeof maxBuffer === 'number' && maxBuffer >= 0)) { - // TODO should this be a RangeError? - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', - 'options.maxBuffer', - maxBuffer); - //'"maxBuffer" must be a positive number'); + throw new errors.RangeError('ERR_VALUE_OUT_OF_RANGE', + 'options.maxBuffer', + 'a positive number', + maxBuffer); } } diff --git a/test/parallel/test-child-process-spawnsync-input.js b/test/parallel/test-child-process-spawnsync-input.js index a2c636b37111fe..3ea4eae2c449e3 100644 --- a/test/parallel/test-child-process-spawnsync-input.js +++ b/test/parallel/test-child-process-spawnsync-input.js @@ -78,7 +78,7 @@ let options = { common.expectsError( () => spawnSync('cat', [], options), - { code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 1); + { code: 'ERR_INVALID_ARG_TYPE', type: TypeError }); options = { input: 'hello world' From f9a5dd6032a7245298cd6890bc0297b8335b09ed Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Mon, 25 Sep 2017 22:01:56 -0400 Subject: [PATCH 07/15] [squash] fix more nits --- lib/child_process.js | 4 +- ...ild-process-spawnsync-validation-errors.js | 46 +++++++++---------- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index b40c514fd5461a..afbd2b2b6be714 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -65,7 +65,7 @@ exports.fork = function(modulePath /*, args, options*/) { if (pos < arguments.length && arguments[pos] != null) { if (typeof arguments[pos] !== 'object') { throw new errors.TypeError('ERR_INVALID_OPT_VALUE', - ('arguments[' + pos + ']'), + `arguments[${pos}]`, arguments[pos]); } @@ -200,7 +200,7 @@ exports.execFile = function(file /*, args, options, callback*/) { } if (!callback && pos < arguments.length && arguments[pos] != null) { - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'args', arguments); + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'args', arguments[pos]); } // Validate the timeout, if present. diff --git a/test/parallel/test-child-process-spawnsync-validation-errors.js b/test/parallel/test-child-process-spawnsync-validation-errors.js index 4a362b06a400b1..24cf39d95f2f5e 100644 --- a/test/parallel/test-child-process-spawnsync-validation-errors.js +++ b/test/parallel/test-child-process-spawnsync-validation-errors.js @@ -4,12 +4,12 @@ const assert = require('assert'); const spawnSync = require('child_process').spawnSync; const signals = process.binding('constants').os.signals; -const invalidOptValueError = - common.expectsError({ code: 'ERR_INVALID_OPT_VALUE', type: TypeError }, 20); - const invalidArgTypeError = common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 56); +const invalidRangeError = + common.expectsError({ code: 'ERR_VALUE_OUT_OF_RANGE', type: RangeError }, 20); + function pass(option, value) { // Run the command with the specified option. Since it's not a real command, // spawnSync() should run successfully but return an ENOENT error. @@ -154,17 +154,17 @@ if (!common.isWindows) { pass('timeout', null); pass('timeout', 1); pass('timeout', 0); - fail('timeout', -1, invalidOptValueError); - fail('timeout', true, invalidOptValueError); - fail('timeout', false, invalidOptValueError); - fail('timeout', __dirname, invalidOptValueError); - fail('timeout', [], invalidOptValueError); - fail('timeout', {}, invalidOptValueError); - fail('timeout', common.mustNotCall(), invalidOptValueError); - fail('timeout', NaN, invalidOptValueError); - fail('timeout', Infinity, invalidOptValueError); - fail('timeout', 3.1, invalidOptValueError); - fail('timeout', -3.1, invalidOptValueError); + fail('timeout', -1, invalidRangeError); + fail('timeout', true, invalidRangeError); + fail('timeout', false, invalidRangeError); + fail('timeout', __dirname, invalidRangeError); + fail('timeout', [], invalidRangeError); + fail('timeout', {}, invalidRangeError); + fail('timeout', common.mustNotCall(), invalidRangeError); + fail('timeout', NaN, invalidRangeError); + fail('timeout', Infinity, invalidRangeError); + fail('timeout', 3.1, invalidRangeError); + fail('timeout', -3.1, invalidRangeError); } { @@ -177,15 +177,15 @@ if (!common.isWindows) { pass('maxBuffer', 0); pass('maxBuffer', Infinity); pass('maxBuffer', 3.14); - fail('maxBuffer', -1, invalidOptValueError); - fail('maxBuffer', NaN, invalidOptValueError); - fail('maxBuffer', -Infinity, invalidOptValueError); - fail('maxBuffer', true, invalidOptValueError); - fail('maxBuffer', false, invalidOptValueError); - fail('maxBuffer', __dirname, invalidOptValueError); - fail('maxBuffer', [], invalidOptValueError); - fail('maxBuffer', {}, invalidOptValueError); - fail('maxBuffer', common.mustNotCall(), invalidOptValueError); + fail('maxBuffer', -1, invalidRangeError); + fail('maxBuffer', NaN, invalidRangeError); + fail('maxBuffer', -Infinity, invalidRangeError); + fail('maxBuffer', true, invalidRangeError); + fail('maxBuffer', false, invalidRangeError); + fail('maxBuffer', __dirname, invalidRangeError); + fail('maxBuffer', [], invalidRangeError); + fail('maxBuffer', {}, invalidRangeError); + fail('maxBuffer', common.mustNotCall(), invalidRangeError); } { From dba4e7c83cec04b6d3013111e511b27d4886b36d Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Mon, 25 Sep 2017 22:05:46 -0400 Subject: [PATCH 08/15] [squash] ERR_INVALID_OPT_VALUE --> ERR_INVALID_ARG_TYPE --- lib/child_process.js | 4 ++-- .../test-child-process-spawn-typeerror.js | 18 +++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index afbd2b2b6be714..dc97a67d5f6099 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -384,13 +384,13 @@ function _convertCustomFds(options) { function normalizeSpawnArguments(file, args, options) { if (typeof file !== 'string' || file.length === 0) - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'file', file); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'file', 'string', file); if (Array.isArray(args)) { args = args.slice(0); } else if (args !== undefined && (args === null || typeof args !== 'object')) { - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'args', args); + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'args', 'object', args); } else { options = args; args = []; diff --git a/test/parallel/test-child-process-spawn-typeerror.js b/test/parallel/test-child-process-spawn-typeerror.js index 47031e5df6d6c2..57ff67ad154654 100644 --- a/test/parallel/test-child-process-spawn-typeerror.js +++ b/test/parallel/test-child-process-spawn-typeerror.js @@ -30,10 +30,10 @@ const invalidcmd = 'hopefully_you_dont_have_this_on_your_machine'; const empty = fixtures.path('empty.js'); const invalidOptValueError = - common.expectsError({ code: 'ERR_INVALID_OPT_VALUE', type: TypeError }, 20); + common.expectsError({ code: 'ERR_INVALID_OPT_VALUE', type: TypeError }, 13); const invalidArgTypeError = - common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 4); + common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 11); assert.throws(function() { const child = spawn(invalidcmd, 'this is not an array'); @@ -60,24 +60,24 @@ assert.doesNotThrow(function() { // verify that invalid argument combinations throw assert.throws(function() { spawn(); -}, invalidOptValueError); +}, invalidArgTypeError); assert.throws(function() { spawn(''); -}, invalidOptValueError); +}, invalidArgTypeError); assert.throws(function() { const file = { toString() { return null; } }; spawn(file); -}, invalidOptValueError); +}, invalidArgTypeError); assert.throws(function() { spawn(cmd, null); -}, invalidOptValueError); +}, invalidArgTypeError); assert.throws(function() { spawn(cmd, true); -}, invalidOptValueError); +}, invalidArgTypeError); assert.throws(function() { spawn(cmd, [], null); @@ -109,10 +109,10 @@ assert.doesNotThrow(function() { spawn(cmd, o); }); assert.doesNotThrow(function() { spawn(cmd, u, o); }); assert.doesNotThrow(function() { spawn(cmd, a, u); }); -assert.throws(function() { spawn(cmd, n, o); }, invalidOptValueError); +assert.throws(function() { spawn(cmd, n, o); }, invalidArgTypeError); assert.throws(function() { spawn(cmd, a, n); }, invalidArgTypeError); -assert.throws(function() { spawn(cmd, s); }, invalidOptValueError); +assert.throws(function() { spawn(cmd, s); }, invalidArgTypeError); assert.throws(function() { spawn(cmd, a, s); }, invalidArgTypeError); From 12a18899fdb699859755f896b701fa5104134b07 Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Wed, 27 Sep 2017 18:27:36 -0400 Subject: [PATCH 09/15] [squash] remove comment --- test/parallel/test-child-process-spawnsync-validation-errors.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/parallel/test-child-process-spawnsync-validation-errors.js b/test/parallel/test-child-process-spawnsync-validation-errors.js index 24cf39d95f2f5e..b4cbf41f3ad549 100644 --- a/test/parallel/test-child-process-spawnsync-validation-errors.js +++ b/test/parallel/test-child-process-spawnsync-validation-errors.js @@ -169,8 +169,6 @@ if (!common.isWindows) { { // Validate the maxBuffer option - //const err = /^TypeError: "maxBuffer" must be a positive number$/; - pass('maxBuffer', undefined); pass('maxBuffer', null); pass('maxBuffer', 1); From cc218d866acccc418a1c5567aa10b6ea55e689ae Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Thu, 5 Oct 2017 19:58:11 -0400 Subject: [PATCH 10/15] [squash] change call count for windows --- .../test-child-process-spawnsync-validation-errors.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-child-process-spawnsync-validation-errors.js b/test/parallel/test-child-process-spawnsync-validation-errors.js index b4cbf41f3ad549..74e5abad072bcc 100644 --- a/test/parallel/test-child-process-spawnsync-validation-errors.js +++ b/test/parallel/test-child-process-spawnsync-validation-errors.js @@ -4,8 +4,13 @@ const assert = require('assert'); const spawnSync = require('child_process').spawnSync; const signals = process.binding('constants').os.signals; -const invalidArgTypeError = - common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 56); +if (common.isWindows()) { + const invalidArgTypeError = + common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 36); +} else { + const invalidArgTypeError = + common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 56); +} const invalidRangeError = common.expectsError({ code: 'ERR_VALUE_OUT_OF_RANGE', type: RangeError }, 20); From 6ecf0db43b107d9f574147517c2941acd1fb1746 Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Sun, 29 Oct 2017 22:15:11 -0400 Subject: [PATCH 11/15] [squash] isWindows is a prop, not function; use let for error --- .../test-child-process-spawnsync-validation-errors.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-child-process-spawnsync-validation-errors.js b/test/parallel/test-child-process-spawnsync-validation-errors.js index 74e5abad072bcc..7e38b549dc2e09 100644 --- a/test/parallel/test-child-process-spawnsync-validation-errors.js +++ b/test/parallel/test-child-process-spawnsync-validation-errors.js @@ -4,11 +4,13 @@ const assert = require('assert'); const spawnSync = require('child_process').spawnSync; const signals = process.binding('constants').os.signals; -if (common.isWindows()) { - const invalidArgTypeError = +let invalidArgTypeError; + +if (common.isWindows) { + invalidArgTypeError = common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 36); } else { - const invalidArgTypeError = + invalidArgTypeError = common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 56); } From b9b084302e2d4a8cc472c526b6245e42a696a31b Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Fri, 24 Nov 2017 17:31:17 -0500 Subject: [PATCH 12/15] ERR_INVALID_OPT_VALUE --> ERR_INVALID_ARG_VALUE --- lib/child_process.js | 4 +-- .../test-child-process-spawn-typeerror.js | 30 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index dc97a67d5f6099..a4952bbbc2a8d7 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -64,7 +64,7 @@ exports.fork = function(modulePath /*, args, options*/) { if (pos < arguments.length && arguments[pos] != null) { if (typeof arguments[pos] !== 'object') { - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', + throw new errors.TypeError('ERR_INVALID_ARG_VALUE', `arguments[${pos}]`, arguments[pos]); } @@ -200,7 +200,7 @@ exports.execFile = function(file /*, args, options, callback*/) { } if (!callback && pos < arguments.length && arguments[pos] != null) { - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'args', arguments[pos]); + throw new errors.TypeError('ERR_INVALID_ARG_VALUE', 'args', arguments[pos]); } // Validate the timeout, if present. diff --git a/test/parallel/test-child-process-spawn-typeerror.js b/test/parallel/test-child-process-spawn-typeerror.js index 57ff67ad154654..c1b1567d6509f6 100644 --- a/test/parallel/test-child-process-spawn-typeerror.js +++ b/test/parallel/test-child-process-spawn-typeerror.js @@ -29,8 +29,8 @@ const invalidcmd = 'hopefully_you_dont_have_this_on_your_machine'; const empty = fixtures.path('empty.js'); -const invalidOptValueError = - common.expectsError({ code: 'ERR_INVALID_OPT_VALUE', type: TypeError }, 13); +const invalidArgValueError = + common.expectsError({ code: 'ERR_INVALID_ARG_VALUE', type: TypeError }, 13); const invalidArgTypeError = common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 11); @@ -162,17 +162,17 @@ assert.doesNotThrow(function() { execFile(cmd, c, n); }); // string is invalid in arg position (this may seem strange, but is // consistent across node API, cf. `net.createServer('not options', 'not // callback')` -assert.throws(function() { execFile(cmd, s, o, c); }, invalidOptValueError); -assert.throws(function() { execFile(cmd, a, s, c); }, invalidOptValueError); -assert.throws(function() { execFile(cmd, a, o, s); }, invalidOptValueError); -assert.throws(function() { execFile(cmd, a, s); }, invalidOptValueError); -assert.throws(function() { execFile(cmd, o, s); }, invalidOptValueError); -assert.throws(function() { execFile(cmd, u, u, s); }, invalidOptValueError); -assert.throws(function() { execFile(cmd, n, n, s); }, invalidOptValueError); -assert.throws(function() { execFile(cmd, a, u, s); }, invalidOptValueError); -assert.throws(function() { execFile(cmd, a, n, s); }, invalidOptValueError); -assert.throws(function() { execFile(cmd, u, o, s); }, invalidOptValueError); -assert.throws(function() { execFile(cmd, n, o, s); }, invalidOptValueError); +assert.throws(function() { execFile(cmd, s, o, c); }, invalidArgValueError); +assert.throws(function() { execFile(cmd, a, s, c); }, invalidArgValueError); +assert.throws(function() { execFile(cmd, a, o, s); }, invalidArgValueError); +assert.throws(function() { execFile(cmd, a, s); }, invalidArgValueError); +assert.throws(function() { execFile(cmd, o, s); }, invalidArgValueError); +assert.throws(function() { execFile(cmd, u, u, s); }, invalidArgValueError); +assert.throws(function() { execFile(cmd, n, n, s); }, invalidArgValueError); +assert.throws(function() { execFile(cmd, a, u, s); }, invalidArgValueError); +assert.throws(function() { execFile(cmd, a, n, s); }, invalidArgValueError); +assert.throws(function() { execFile(cmd, u, o, s); }, invalidArgValueError); +assert.throws(function() { execFile(cmd, n, o, s); }, invalidArgValueError); assert.doesNotThrow(function() { execFile(cmd, c, s); }); @@ -194,5 +194,5 @@ assert.doesNotThrow(function() { fork(empty, n, n); }); assert.doesNotThrow(function() { fork(empty, n, o); }); assert.doesNotThrow(function() { fork(empty, a, n); }); -assert.throws(function() { fork(empty, s); }, invalidOptValueError); -assert.throws(function() { fork(empty, a, s); }, invalidOptValueError); +assert.throws(function() { fork(empty, s); }, invalidArgValueError); +assert.throws(function() { fork(empty, a, s); }, invalidArgValueError); From c48150a494555d40027c79e33f1570fa7569834f Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Sun, 26 Nov 2017 18:40:20 -0500 Subject: [PATCH 13/15] better error message --- lib/internal/errors.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index b45348a1d7f8bf..3e8a40545a28e8 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -272,8 +272,8 @@ E('ERR_BUFFER_TOO_LARGE', `Cannot create a Buffer larger than 0x${kMaxLength.toString(16)} bytes`); E('ERR_CANNOT_WATCH_SIGINT', 'Cannot watch for SIGINT signals'); E('ERR_CHILD_CLOSED_BEFORE_REPLY', 'Child closed before reply received'); -E('ERR_CHILD_PROCESS_IPC_REQUIRED', - 'Forked processes must have an IPC channel'); +E('ERR_CHILD_PROCESS_IPC_REQUIRED', (name, val) => + `Forked processes must have an IPC channel, ${name} is ${val}`); E('ERR_CHILD_PROCESS_STDIO_MAXBUFFER', (name) => `${name} maxBuffer length exceeded`); E('ERR_CONSOLE_WRITABLE_STREAM', From 6084ef48fb3ebf036f9046de871943f0f16c2803 Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Tue, 28 Nov 2017 15:59:32 -0500 Subject: [PATCH 14/15] fix lint error, switch to util.format style --- lib/internal/errors.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 3e8a40545a28e8..25a91aac7f4e65 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -272,10 +272,9 @@ E('ERR_BUFFER_TOO_LARGE', `Cannot create a Buffer larger than 0x${kMaxLength.toString(16)} bytes`); E('ERR_CANNOT_WATCH_SIGINT', 'Cannot watch for SIGINT signals'); E('ERR_CHILD_CLOSED_BEFORE_REPLY', 'Child closed before reply received'); -E('ERR_CHILD_PROCESS_IPC_REQUIRED', (name, val) => - `Forked processes must have an IPC channel, ${name} is ${val}`); -E('ERR_CHILD_PROCESS_STDIO_MAXBUFFER', - (name) => `${name} maxBuffer length exceeded`); +E('ERR_CHILD_PROCESS_IPC_REQUIRED', + 'Forked processes must have an IPC channel, %s is %s'); +E('ERR_CHILD_PROCESS_STDIO_MAXBUFFER', '%s maxBuffer length exceeded'); E('ERR_CONSOLE_WRITABLE_STREAM', 'Console expects a writable stream instance for %s'); E('ERR_CPU_USAGE', 'Unable to obtain cpu usage %s'); From 8c3eba6c906291a27ebd4f55caeabfad0a8c3f92 Mon Sep 17 00:00:00 2001 From: Jon Moss Date: Wed, 29 Nov 2017 10:04:00 -0500 Subject: [PATCH 15/15] slightly modify error message --- lib/child_process.js | 3 +-- lib/internal/errors.js | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index a4952bbbc2a8d7..c4512ab2f47c08 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -95,8 +95,7 @@ exports.fork = function(modulePath /*, args, options*/) { stdioStringToArray('inherit'); } else if (options.stdio.indexOf('ipc') === -1) { throw new errors.Error('ERR_CHILD_PROCESS_IPC_REQUIRED', - 'options.stdio', - options.stdio); + 'options.stdio'); } options.execPath = options.execPath || process.execPath; diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 25a91aac7f4e65..57fa6679cb61b6 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -273,7 +273,7 @@ E('ERR_BUFFER_TOO_LARGE', E('ERR_CANNOT_WATCH_SIGINT', 'Cannot watch for SIGINT signals'); E('ERR_CHILD_CLOSED_BEFORE_REPLY', 'Child closed before reply received'); E('ERR_CHILD_PROCESS_IPC_REQUIRED', - 'Forked processes must have an IPC channel, %s is %s'); + "Forked processes must have an IPC channel, missing value 'ipc' in %s"); E('ERR_CHILD_PROCESS_STDIO_MAXBUFFER', '%s maxBuffer length exceeded'); E('ERR_CONSOLE_WRITABLE_STREAM', 'Console expects a writable stream instance for %s');