Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

errors, child_process: use more internal/errors codes #14998

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,17 @@ Node.js was unable to watch for the `SIGINT` signal.

A child process was closed before the parent received a reply.

<a id="ERR_CHILD_PROCESS_IPC_REQUIRED"></a>
### ERR_CHILD_PROCESS_IPC_REQUIRED

Used when a child process is being forked without specifying an IPC channel.

<a id="ERR_CHILD_PROCESS_STDIO_MAXBUFFER"></a>
### ERR_CHILD_PROCESS_STDIO_MAXBUFFER

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.

<a id="ERR_CONSOLE_WRITABLE_STREAM"></a>
### ERR_CONSOLE_WRITABLE_STREAM

Expand Down
86 changes: 63 additions & 23 deletions lib/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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);
}
}

Expand All @@ -63,7 +64,9 @@ 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_ARG_VALUE',
`arguments[${pos}]`,
arguments[pos]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one feels super odd to me... will think about this a bit more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Felt odd to me too, and still mulling over how to properly present this state to users 😞

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a ERR_INVALID_ARG_TYPE though? Judging from the code below I would say it's options that has the wrong type?

}

options = util._extend({}, arguments[pos++]);
Expand Down Expand Up @@ -91,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 TypeError('Forked processes must have an IPC channel');
throw new errors.Error('ERR_CHILD_PROCESS_IPC_REQUIRED',
'options.stdio',
options.stdio);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message looks like this: Forked processes must have an IPC channel options.stdio stdioValue. Is it intended? Maybe just using ERR_INVALID_OPT_VALUE is enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I prefer a separate error code -- I see the issue as not just "invalid option for stdio", but "forked process cannot be started because there's no ipc channel"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with a separate code, but the message still looks weird

}

options.execPath = options.execPath || process.execPath;
Expand Down Expand Up @@ -195,7 +200,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_ARG_VALUE', 'args', arguments[pos]);
}

// Validate the timeout, if present.
Expand Down Expand Up @@ -322,7 +327,8 @@ exports.execFile = function(file /*, args, options, callback*/) {
stdoutLen += encoding ? Buffer.byteLength(chunk, encoding) : chunk.length;

if (stdoutLen > options.maxBuffer) {
ex = new Error('stdout maxBuffer exceeded');
ex = new errors.RangeError('ERR_CHILD_PROCESS_STDIO_MAXBUFFER',
'stdout');
kill();
} else if (encoding) {
_stdout += chunk;
Expand All @@ -340,7 +346,8 @@ exports.execFile = function(file /*, args, options, callback*/) {
stderrLen += encoding ? Buffer.byteLength(chunk, encoding) : chunk.length;

if (stderrLen > options.maxBuffer) {
ex = new Error('stderr maxBuffer exceeded');
ex = new errors.RangeError('ERR_CHILD_PROCESS_STDIO_MAXBUFFER',
'stderr');
kill();
} else if (encoding) {
_stderr += chunk;
Expand Down Expand Up @@ -377,13 +384,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_ARG_TYPE', 'file', 'string', 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_ARG_TYPE', 'args', 'object', args);
} else {
options = args;
args = [];
Expand All @@ -392,41 +399,62 @@ 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.
Expand All @@ -438,7 +466,10 @@ 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.
Expand Down Expand Up @@ -549,10 +580,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);
}
}
}
Expand Down Expand Up @@ -620,14 +651,20 @@ exports.execSync = execSync;

function validateTimeout(timeout) {
if (timeout != null && !(Number.isInteger(timeout) && timeout >= 0)) {
throw new TypeError('"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)) {
throw new TypeError('"maxBuffer" must be a positive number');
throw new errors.RangeError('ERR_VALUE_OUT_OF_RANGE',
'options.maxBuffer',
'a positive number',
maxBuffer);
}
}

Expand All @@ -636,6 +673,9 @@ 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);
}
}
3 changes: 3 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +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',
'Forked processes must have an IPC channel, %s is %s');
Copy link
Member

@apapirovski apapirovski Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used in one place, it doesn't feel like %s is %s is really that helpful. It should probably just say something like Forked processes must have an IPC channel, missing value 'ipc' in %s.

(This accepts an array so the %s would be [value, value, value].)

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');
Expand Down
5 changes: 3 additions & 2 deletions test/parallel/test-child-process-exec-maxBuffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
}

Expand Down
5 changes: 3 additions & 2 deletions test/parallel/test-child-process-fork-stdio-string-variant.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ 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);
common.expectsError(
() => fork(childScript, malFormedOpts),
{ code: 'ERR_INVALID_OPT_VALUE', type: TypeError });

function test(stringVariant) {
const child = fork(childScript, { stdio: stringVariant });
Expand Down
6 changes: 3 additions & 3 deletions test/parallel/test-child-process-fork-stdio.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ if (process.argv[2] === 'child') {
process.send(data);
});
} else {
assert.throws(() => {
cp.fork(__filename, { stdio: ['pipe', 'pipe', 'pipe', 'pipe'] });
}, /Forked processes must have an IPC channel/);
common.expectsError(
() => cp.fork(__filename, { stdio: ['pipe', 'pipe', 'pipe', 'pipe'] }),
{ code: 'ERR_CHILD_PROCESS_IPC_REQUIRED', type: Error });

let ipc = '';
let stderr = '';
Expand Down
60 changes: 31 additions & 29 deletions test/parallel/test-child-process-spawn-typeerror.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,15 @@ 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 invalidArgValueError =
common.expectsError({ code: 'ERR_INVALID_ARG_VALUE', type: TypeError }, 13);

const invalidArgTypeError =
common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 11);

assert.throws(function() {
const child = spawn(invalidcmd, 'this is not an array');
child.on('error', common.mustNotCall());
Expand All @@ -58,32 +60,32 @@ assert.doesNotThrow(function() {
// verify that invalid argument combinations throw
assert.throws(function() {
spawn();
}, invalidFileMsg);
}, invalidArgTypeError);

assert.throws(function() {
spawn('');
}, invalidFileMsg);
}, invalidArgTypeError);

assert.throws(function() {
const file = { toString() { throw new Error('foo'); } };
const file = { toString() { return null; } };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional? What happens now if the error is thrown like in the old test?

spawn(file);
}, invalidFileMsg);
}, invalidArgTypeError);

assert.throws(function() {
spawn(cmd, null);
}, invalidArgsMsg);
}, invalidArgTypeError);

assert.throws(function() {
spawn(cmd, true);
}, invalidArgsMsg);
}, invalidArgTypeError);

assert.throws(function() {
spawn(cmd, [], null);
}, invalidOptionsMsg);
}, invalidArgTypeError);

assert.throws(function() {
spawn(cmd, [], 1);
}, invalidOptionsMsg);
}, invalidArgTypeError);

// Argument types for combinatorics
const a = [];
Expand All @@ -107,11 +109,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); }, invalidArgTypeError);
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); }, invalidArgTypeError);
assert.throws(function() { spawn(cmd, a, s); }, invalidArgTypeError);


// verify that execFile has same argument parsing behaviour as spawn
Expand Down Expand Up @@ -160,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); }, 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); }, 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); });


Expand All @@ -192,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); }, TypeError);
assert.throws(function() { fork(empty, a, s); }, TypeError);
assert.throws(function() { fork(empty, s); }, invalidArgValueError);
assert.throws(function() { fork(empty, a, s); }, invalidArgValueError);
Loading