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: migrate to using internal/errors #11300

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
61 changes: 61 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,27 @@ Node.js API that consumes `file:` URLs (such as certain functions in the
[`fs`][] module) encounters a file URL with an incompatible path. The exact
semantics for determining whether a path can be used is platform-dependent.

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

The '`ERR_INVALID_HANDLE_TYPE`' error code is used when an attempt is made to
send an unsupported "handle" over an IPC communication channel to a child
process. See [`child.send()`] and [`process.send()`] for more information.

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

The `'ERR_INVALID_OPT_VALUE'` error code is used generically to identify when
an invalid or unexpected value has been passed in an options object.

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

The `'ERR_INVALID_SYNC_FORK_INPUT'` error code is used when a `Buffer`,
`Uint8Array` or `string` is provided as stdio input to a synchronous
fork. See the documentation for the [`child_process`](child_process.html)
module for more information.

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

Expand Down Expand Up @@ -642,6 +663,36 @@ It is currently only used in the [WHATWG URL API][] support in the [`fs`][]
module (which only accepts URLs with `'file'` scheme), but may be used in other
Node.js APIs as well in the future.

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

The `'ERR_IPC_CHANNEL_CLOSED'` error code is used when an attempt is made to use
an IPC communication channel that has already been closed.

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

The `'ERR_IPC_DISCONNECTED'` error code is used when an attempt is made to
disconnect an already disconnected IPC communication channel between two
Node.js processes. See the documentation for the
[`child_process`](child_process.html) module for more information.

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

The `'ERR_IPC_ONE_PIPE'` error code is used when an attempt is made to create
a child Node.js process using more than one IPC communication channel.
See the documentation for the [`child_process`](child_process.html)
module for more information.

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

The `'ERR_IPC_SYNC_FORK'` error code is used when an attempt is made to open
an IPC communication channel with a synchronous forked Node.js process.
See the documentation for the [`child_process`](child_process.html)
module for more information.

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

Expand Down Expand Up @@ -674,6 +725,13 @@ kind of internal Node.js error that should not typically be triggered by user
code. Instances of this error point to an internal bug within the Node.js
binary itself.

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

The `'ERR_UNKNOWN_SIGNAL`' error code is used when an invalid or unknown
process signal is passed to an API expecting a valid signal (such as
[`child.kill()`][]).

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

Expand All @@ -693,6 +751,9 @@ in user code, although it is not impossible. Occurrences of this error are most
likely an indication of a bug within Node.js itself.


[`child.kill()`]: child_process.html#child_process_child_kill_signal
[`child.send()`]: child_process.html#child_process_child_send_message_sendhandle_options_callback
[`process.send()`]: process.html#process_process_send_message_sendhandle_options_callback
[`fs.readdir`]: fs.html#fs_fs_readdir_path_options_callback
[`fs.readFileSync`]: fs.html#fs_fs_readfilesync_file_options
[`fs.unlink`]: fs.html#fs_fs_unlink_path_callback
Expand Down
32 changes: 17 additions & 15 deletions lib/internal/child_process.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

const errors = require('internal/errors');
const StringDecoder = require('string_decoder').StringDecoder;
const EventEmitter = require('events');
const net = require('net');
Expand Down Expand Up @@ -367,6 +368,7 @@ function onErrorNT(self, err) {


ChildProcess.prototype.kill = function(sig) {

const signal = sig === 0 ? sig :
convertToValidSignal(sig === undefined ? 'SIGTERM' : sig);

Expand Down Expand Up @@ -538,15 +540,15 @@ function setupChannel(target, channel) {
options = undefined;
} else if (options !== undefined &&
(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 = Object.assign({swallowErrors: false}, options);

if (this.connected) {
return this._send(message, handle, options, callback);
}
const ex = new Error('channel closed');
const ex = new errors.Error('ERR_IPC_CHANNEL_CLOSED');
if (typeof callback === 'function') {
process.nextTick(callback, ex);
} else {
Expand All @@ -559,7 +561,7 @@ function setupChannel(target, channel) {
assert(this.connected || this.channel);

if (message === undefined)
throw new TypeError('"message" argument cannot be undefined');
throw new errors.TypeError('ERR_MISSING_ARGS', 'message');

// Support legacy function signature
if (typeof options === 'boolean') {
Expand All @@ -586,7 +588,7 @@ function setupChannel(target, channel) {
} else if (handle instanceof UDP) {
message.type = 'dgram.Native';
} else {
throw new TypeError('This handle type can\'t be sent');
throw new errors.TypeError('ERR_INVALID_HANDLE_TYPE');
}

// Queue-up message and handle if we haven't received ACK yet.
Expand Down Expand Up @@ -686,7 +688,7 @@ function setupChannel(target, channel) {

target.disconnect = function() {
if (!this.connected) {
this.emit('error', new Error('IPC channel is already disconnected'));
this.emit('error', new errors.Error('ERR_IPC_DISCONNECTED'));
return;
}

Expand Down Expand Up @@ -766,11 +768,12 @@ function _validateStdio(stdio, sync) {
case 'ignore': stdio = ['ignore', 'ignore', 'ignore']; break;
case 'pipe': stdio = ['pipe', 'pipe', 'pipe']; break;
case 'inherit': stdio = [0, 1, 2]; break;
default: throw new TypeError('Incorrect value of stdio option: ' + stdio);
default:
throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'stdio', stdio);
}
} else if (!Array.isArray(stdio)) {
throw new TypeError('Incorrect value of stdio option: ' +
util.inspect(stdio));
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
'stdio', util.inspect(stdio));
}

// At least 3 stdio will be created
Expand Down Expand Up @@ -812,9 +815,9 @@ function _validateStdio(stdio, sync) {
// Cleanup previously created pipes
cleanup();
if (!sync)
throw new Error('Child process can have only one IPC pipe');
throw new errors.Error('ERR_IPC_ONE_PIPE');
else
throw new Error('You cannot use IPC with synchronous forks');
throw new errors.Error('ERR_IPC_SYNC_FORK');
}

ipc = new Pipe(true);
Expand Down Expand Up @@ -849,15 +852,14 @@ function _validateStdio(stdio, sync) {
} else if (isUint8Array(stdio) || typeof stdio === 'string') {
if (!sync) {
cleanup();
throw new TypeError('Asynchronous forks do not support ' +
'Buffer, Uint8Array or string input: ' +
util.inspect(stdio));
throw new errors.TypeError('ERR_INVALID_SYNC_FORK_INPUT',
util.inspect(stdio));
}
} else {
// Cleanup
cleanup();
throw new TypeError('Incorrect value for stdio stream: ' +
util.inspect(stdio));
throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'stdio',
util.inspect(stdio));
}

return acc;
Expand Down
28 changes: 25 additions & 3 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
// value statically and permanently identifies the error. While the error
// message may change, the code should not.

const assert = require('assert');
const kCode = Symbol('code');
const messages = new Map();

Expand All @@ -17,6 +16,13 @@ function lazyUtil() {
return util;
}

var assert;
function lazyAssert() {
if (!assert)
assert = require('assert');
return assert;
}

function makeNodeError(Base) {
return class NodeError extends Base {
constructor(key, ...args) {
Expand All @@ -36,6 +42,7 @@ function makeNodeError(Base) {
}

function message(key, args) {
const assert = lazyAssert();
assert.strictEqual(typeof key, 'string');
const util = lazyUtil();
const msg = messages.get(key);
Expand All @@ -54,7 +61,6 @@ function message(key, args) {
// Utility function for registering the error codes. Only used here. Exported
// *only* to allow for testing.
function E(sym, val) {
assert(messages.has(sym) === false, `Error symbol: ${sym} was already used.`);
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove this check? (The test for it is removed in the second commit btw)

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of issues in the loading order of the errors.js module relative to assert.js on startup. There is a circular dependency that happens that causes the util.js used within assert.js to fail depending on when assert is loaded. I could replace this with a simple throw rather than the call to assert if you'd be more comfortable with that.

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 fine with removing it. I was just curious. The risk of reusing the same code is low if we keep them together and in alphabetical order.

messages.set(sym, typeof val === 'function' ? val : String(val));
}

Expand Down Expand Up @@ -85,20 +91,36 @@ E('ERR_INVALID_ARG_TYPE', invalidArgType);
E('ERR_INVALID_CALLBACK', 'callback must be a function');
E('ERR_INVALID_FILE_URL_HOST', 'File URL host %s');
E('ERR_INVALID_FILE_URL_PATH', 'File URL path %s');
E('ERR_INVALID_HANDLE_TYPE', 'This handle type cannot be sent');
E('ERR_INVALID_OPT_VALUE',
(name, value) => {
return `The value "${String(value)}" is invalid for option "${name}"`;
});
E('ERR_INVALID_SYNC_FORK_INPUT',
(value) => {
return 'Asynchronous forks do not support Buffer, Uint8Array or string' +
`input: ${value}`;
});
E('ERR_INVALID_THIS', 'Value of "this" must be of type %s');
E('ERR_INVALID_TUPLE', '%s must be an iterable %s tuple');
E('ERR_INVALID_URL', 'Invalid URL: %s');
E('ERR_INVALID_URL_SCHEME',
(expected) => `The URL must be ${oneOf(expected, 'scheme')}`);
E('ERR_IPC_CHANNEL_CLOSED', 'channel closed');
E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected');
E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe');
E('ERR_IPC_SYNC_FORK', 'IPC cannot be used with synchronous forks');
E('ERR_MISSING_ARGS', missingArgs);
E('ERR_STDERR_CLOSE', 'process.stderr cannot be closed');
E('ERR_STDOUT_CLOSE', 'process.stdout cannot be closed');
E('ERR_UNKNOWN_BUILTIN_MODULE', (id) => `No such built-in module: ${id}`);
E('ERR_UNKNOWN_SIGNAL', (signal) => `Unknown signal: ${signal}`);
E('ERR_UNKNOWN_STDIN_TYPE', 'Unknown stdin file type');
E('ERR_UNKNOWN_STREAM_TYPE', 'Unknown stream file type');
E('ERR_UNKNOWN_BUILTIN_MODULE', (id) => `No such built-in module: ${id}`);
// Add new errors from here...

function invalidArgType(name, expected, actual) {
const assert = lazyAssert();
assert(name, 'name is required');
var msg = `The "${name}" argument must be ${oneOf(expected, 'type')}`;
if (arguments.length >= 3) {
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/util.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

const errors = require('internal/errors');
const binding = process.binding('util');
const signals = process.binding('constants').os.signals;

Expand Down Expand Up @@ -177,7 +178,7 @@ function convertToValidSignal(signal) {
if (signalName) return signalName;
}

throw new Error('Unknown signal: ' + signal);
throw new errors.Error('ERR_UNKNOWN_SIGNAL', signal);
}

function getConstructorOf(obj) {
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-child-process-constructor.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

require('../common');
const common = require('../common');
const assert = require('assert');
const { ChildProcess } = require('child_process');
assert.strictEqual(typeof ChildProcess, 'function');
Expand Down Expand Up @@ -64,6 +64,6 @@ assert(Number.isInteger(child.pid));
// try killing with invalid signal
assert.throws(() => {
child.kill('foo');
}, /^Error: Unknown signal: foo$/);
}, common.expectsError({ code: 'ERR_UNKNOWN_SIGNAL' }));

assert.strictEqual(child.kill(), true);
2 changes: 1 addition & 1 deletion test/parallel/test-child-process-send-type-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const cp = require('child_process');
function fail(proc, args) {
assert.throws(() => {
proc.send.apply(proc, args);
}, /"options" argument must be an object/);
}, common.expectsError({code: 'ERR_INVALID_ARG_TYPE', type: TypeError}));
}

let target = process;
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-child-process-spawnsync-kill-signal.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ if (process.argv[2] === 'child') {
// Verify that an error is thrown for unknown signals.
assert.throws(() => {
spawn('SIG_NOT_A_REAL_SIGNAL');
}, /Error: Unknown signal: SIG_NOT_A_REAL_SIGNAL/);
}, common.expectsError({ code: 'ERR_UNKNOWN_SIGNAL' }));

// Verify that the default kill signal is SIGTERM.
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,8 @@ if (!common.isWindows) {
{
// Validate the killSignal option
const typeErr = /^TypeError: "killSignal" must be a string or number$/;
const unknownSignalErr = /^Error: Unknown signal:/;
const unknownSignalErr =
common.expectsError({ code: 'ERR_UNKNOWN_SIGNAL' });

pass('killSignal', undefined);
pass('killSignal', null);
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-child-process-stdio.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,4 @@ assert.deepStrictEqual(options, {stdio: 'ignore'});

assert.throws(() => {
common.spawnPwd({stdio: ['pipe', 'pipe', 'pipe', 'ipc', 'ipc']});
}, /^Error: Child process can have only one IPC pipe$/);
}, common.expectsError({code: 'ERR_IPC_ONE_PIPE', type: Error}));
18 changes: 8 additions & 10 deletions test/parallel/test-child-process-validate-stdio.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
'use strict';
// Flags: --expose_internals

require('../common');
const common = require('../common');
const assert = require('assert');
const _validateStdio = require('internal/child_process')._validateStdio;

const expectedError =
common.expectsError({code: 'ERR_INVALID_OPT_VALUE', type: TypeError});

// should throw if string and not ignore, pipe, or inherit
assert.throws(function() {
_validateStdio('foo');
}, /Incorrect value of stdio option/);
assert.throws(() => _validateStdio('foo'), expectedError);

// should throw if not a string or array
assert.throws(function() {
_validateStdio(600);
}, /Incorrect value of stdio option/);
assert.throws(() => _validateStdio(600), expectedError);

// should populate stdio with undefined if len < 3
{
Expand All @@ -27,9 +26,8 @@ assert.throws(function() {

// should throw if stdio has ipc and sync is true
const stdio2 = ['ipc', 'ipc', 'ipc'];
assert.throws(function() {
_validateStdio(stdio2, true);
}, /You cannot use IPC with synchronous forks/);
assert.throws(() => _validateStdio(stdio2, true),
common.expectsError({code: 'ERR_IPC_SYNC_FORK', type: Error}));

{
const stdio3 = [process.stdin, process.stdout, process.stderr];
Expand Down
6 changes: 0 additions & 6 deletions test/parallel/test-internal-errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,6 @@ assert.throws(() => {
message: /^Error for testing 2/ }));
}, /AssertionError: .+ does not match \S/);

assert.doesNotThrow(() => errors.E('TEST_ERROR_USED_SYMBOL'));
Copy link
Member

Choose a reason for hiding this comment

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

please bundle this change with the same commit that removed the error

assert.throws(
() => errors.E('TEST_ERROR_USED_SYMBOL'),
/^AssertionError: Error symbol: TEST_ERROR_USED_SYMBOL was already used\.$/
);

// // Test ERR_INVALID_ARG_TYPE
assert.strictEqual(errors.message('ERR_INVALID_ARG_TYPE', ['a', 'b']),
'The "a" argument must be of type b');
Expand Down