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.

Sign up for GitHub

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

Jump to bottom

lib: finish assigning codes to errors thrown from JS #19373

Closed
wants to merge 2 commits into from
Closed

lib: finish assigning codes to errors thrown from JS #19373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6337ec4
fs,cluster,net: assign error codes to remaining errors
targos Mar 15, 2018
957c493
tools,lib: forbid native Error constructors
targos Mar 15, 2018
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
1 change: 1 addition & 0 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ module.exports = {
}
],
/* eslint-disable max-len, quotes */
// If this list is modified, please copy the change to lib/.eslintrc.yaml
'no-restricted-syntax': [
'error',
{
Expand Down
6 changes: 6 additions & 0 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,6 +777,12 @@ Encoding provided to `util.TextDecoder()` API was not one of the
A `Promise` that was callbackified via `util.callbackify()` was rejected with a
falsy value.

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

An attempt has been made to read a file whose size is larger than the maximum
allowed size for a `Buffer`.

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

Expand Down
18 changes: 18 additions & 0 deletions lib/.eslintrc.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,22 @@
rules:
no-restricted-syntax:
# Config copied from .eslintrc.js
- error
- selector: "CallExpression[callee.object.name='assert'][callee.property.name='doesNotThrow']"
message: "Please replace `assert.doesNotThrow()` and add a comment next to the code instead."
- selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.1.type='Literal']:not([arguments.1.regex])"
message: "use a regular expression for second argument of assert.throws()"
- selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.length<2]"
message: "assert.throws() must be invoked with at least two arguments."
- selector: "CallExpression[callee.name='setTimeout'][arguments.length<2]"
message: "setTimeout() must be invoked with at least two arguments."
- selector: "CallExpression[callee.name='setInterval'][arguments.length<2]"
message: "setInterval() must be invoked with at least 2 arguments."
- selector: "ThrowStatement > CallExpression[callee.name=/Error$/]"
message: "Use new keyword when throwing an Error."
# Config specific to lib
- selector: "NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError)$/])"
message: "Use an error exported by the internal/errors module."
# Custom rules in tools/eslint-rules
node-core/require-buffer: error
node-core/buffer-constructor: error
Expand Down
1 change: 1 addition & 0 deletions lib/_http_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ function emitAbortNT() {


function createHangUpError() {
// eslint-disable-next-line no-restricted-syntax
var error = new Error('socket hang up');
error.code = 'ECONNRESET';
return error;
Expand Down
2 changes: 2 additions & 0 deletions lib/_tls_wrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,7 @@ function onSocketClose(err) {
// Emit ECONNRESET
if (!this._controlReleased && !this[kErrorEmitted]) {
this[kErrorEmitted] = true;
// eslint-disable-next-line no-restricted-syntax
const connReset = new Error('socket hang up');
connReset.code = 'ECONNRESET';
this._tlsOptions.server.emit('tlsClientError', connReset, this);
Expand Down Expand Up @@ -1103,6 +1104,7 @@ function onConnectEnd() {
if (!this._hadError) {
const options = this[kConnectOptions];
this._hadError = true;
// eslint-disable-next-line no-restricted-syntax
const error = new Error('Client network socket disconnected before ' +
'secure TLS connection was established');
error.code = 'ECONNRESET';
Expand Down
1 change: 1 addition & 0 deletions lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ function innerOk(fn, argLen, value, message) {
} else if (message == null) {
// Use the call as error message if possible.
// This does not work with e.g. the repl.
// eslint-disable-next-line no-restricted-syntax
const err = new Error();
// Make sure the limit is set to 1. Otherwise it could fail (<= 0) or it
// does to much work.
Expand Down
1 change: 1 addition & 0 deletions lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,6 +1073,7 @@ if (process.binding('config').hasIntl) {
return result;

const code = icuErrName(result);
// eslint-disable-next-line no-restricted-syntax
const err = new Error(`Unable to transcode Buffer [${code}]`);
err.code = code;
err.errno = result;
Expand Down
2 changes: 2 additions & 0 deletions lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ exports.execFile = function(file /*, args, options, callback*/) {
cmd += ` ${args.join(' ')}`;

if (!ex) {
// eslint-disable-next-line no-restricted-syntax
ex = new Error('Command failed: ' + cmd + '\n' + stderr);
ex.killed = child.killed || killed;
ex.code = code < 0 ? getSystemErrorName(code) : code;
Expand Down Expand Up @@ -588,6 +589,7 @@ function checkExecSyncError(ret, args, cmd) {
msg += cmd || args.join(' ');
if (ret.stderr && ret.stderr.length > 0)
msg += `\n${ret.stderr.toString()}`;
// eslint-disable-next-line no-restricted-syntax
err = new Error(msg);
}
if (err) {
Expand Down
1 change: 1 addition & 0 deletions lib/domain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ if (process.hasUncaughtExceptionCaptureCallback()) {
}

// Get the stack trace at the point where `domain` was required.
// eslint-disable-next-line no-restricted-syntax
const domainRequireStack = new Error('require(`domain`) at this point').stack;

const { setUncaughtExceptionCaptureCallback } = process;
Expand Down
1 change: 1 addition & 0 deletions lib/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ function _addListener(target, type, listener, prepend) {
if (m && m > 0 && existing.length > m) {
existing.warned = true;
// No error code for this since it is a Warning
// eslint-disable-next-line no-restricted-syntax
const w = new Error('Possible EventEmitter memory leak detected. ' +
`${existing.length} ${String(type)} listeners ` +
'added. Use emitter.setMaxListeners() to ' +
Expand Down
7 changes: 5 additions & 2 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const fs = exports;
const { Buffer } = require('buffer');
const errors = require('internal/errors');
const {
ERR_FS_FILE_TOO_LARGE,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_CALLBACK,
ERR_OUT_OF_RANGE
Expand Down Expand Up @@ -347,8 +348,7 @@ function readFileAfterStat(err) {
}

if (size > kMaxLength) {
err = new RangeError('File size is greater than possible Buffer: ' +
`0x${kMaxLength.toString(16)} bytes`);
err = new ERR_FS_FILE_TOO_LARGE(size);
return context.close(err);
}

Expand Down Expand Up @@ -421,6 +421,9 @@ function tryCreateBuffer(size, fd, isUserFd) {
var threw = true;
var buffer;
try {
if (size > kMaxLength) {
throw new ERR_FS_FILE_TOO_LARGE(size);
}
buffer = Buffer.allocUnsafe(size);
Copy link
Member

Choose a reason for hiding this comment

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

Since we now check for kMaxLength: is it still possible for the alloc to throw an error? If not, then the try / catch should be obsolete.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it can still throw if there is not enough memory available to construct the buffer?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, I thought it might crash in that case but it will actually throw Array buffer allocation failed.

threw = false;
} finally {
Expand Down
4 changes: 2 additions & 2 deletions lib/fs/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const {
const binding = process.binding('fs');
const { Buffer, kMaxLength } = require('buffer');
const {
ERR_BUFFER_TOO_LARGE,
ERR_FS_FILE_TOO_LARGE,
ERR_INVALID_ARG_TYPE,
ERR_METHOD_NOT_IMPLEMENTED,
ERR_OUT_OF_RANGE
Expand Down Expand Up @@ -143,7 +143,7 @@ async function readFileHandle(filehandle, options) {
return Buffer.alloc(0);

if (size > kMaxLength)
throw new ERR_BUFFER_TOO_LARGE();
throw new ERR_FS_FILE_TOO_LARGE(size);

const chunks = [];
const chunkSize = Math.min(size, 16384);
Expand Down
1 change: 1 addition & 0 deletions lib/internal/bootstrap/loaders.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@
// Model the error off the internal/errors.js model, but
// do not use that module given that it could actually be
// the one causing the error if there's a bug in Node.js
// eslint-disable-next-line no-restricted-syntax
const err = new Error(`No such built-in module: ${id}`);
err.code = 'ERR_UNKNOWN_BUILTIN_MODULE';
err.name = 'Error [ERR_UNKNOWN_BUILTIN_MODULE]';
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/cluster/master.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const RoundRobinHandle = require('internal/cluster/round_robin_handle');
const SharedHandle = require('internal/cluster/shared_handle');
const Worker = require('internal/cluster/worker');
const { internal, sendHelper, handles } = require('internal/cluster/utils');
const { ERR_SOCKET_BAD_PORT } = require('internal/errors').codes;
const keys = Object.keys;
const cluster = new EventEmitter();
const intercom = new EventEmitter();
Expand Down Expand Up @@ -115,8 +116,7 @@ function createWorkerProcess(id, env) {
inspectPort = cluster.settings.inspectPort;

if (!isLegalPort(inspectPort)) {
throw new TypeError('cluster.settings.inspectPort' +
' is invalid');
throw new ERR_SOCKET_BAD_PORT(inspectPort);
}
} else {
inspectPort = process.debugPort + debugPortOffset;
Expand Down
7 changes: 7 additions & 0 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,7 @@ function uvException(ctx) {

// Pass the message to the constructor instead of setting it on the object
// to make sure it is the same as the one created in C++
// eslint-disable-next-line no-restricted-syntax
const err = new Error(message);

for (const prop of Object.keys(ctx)) {
Expand Down Expand Up @@ -482,6 +483,7 @@ function errnoException(err, syscall, original) {
const message = original ?
`${syscall} ${code} ${original}` : `${syscall} ${code}`;

// eslint-disable-next-line no-restricted-syntax
const ex = new Error(message);
// TODO(joyeecheung): errno is supposed to err, like in uvException
ex.code = ex.errno = code;
Expand Down Expand Up @@ -517,6 +519,7 @@ function exceptionWithHostPort(err, syscall, address, port, additional) {
details += ` - Local (${additional})`;
}

// eslint-disable-next-line no-restricted-syntax
const ex = new Error(`${syscall} ${code}${details}`);
// TODO(joyeecheung): errno is supposed to err, like in uvException
ex.code = ex.errno = code;
Expand All @@ -537,6 +540,7 @@ function exceptionWithHostPort(err, syscall, address, port, additional) {
* @returns {Error}
*/
function dnsException(err, syscall, hostname) {
// eslint-disable-next-line no-restricted-syntax
const ex = new Error();
// FIXME(bnoordhuis) Remove this backwards compatibility nonsense and pass
// the true error to the user. ENOTFOUND is not even a proper POSIX error!
Expand Down Expand Up @@ -651,6 +655,9 @@ E('ERR_ENCODING_INVALID_ENCODED_DATA',
E('ERR_ENCODING_NOT_SUPPORTED', 'The "%s" encoding is not supported',
RangeError);
E('ERR_FALSY_VALUE_REJECTION', 'Promise was rejected with falsy value', Error);
E('ERR_FS_FILE_TOO_LARGE', 'File size (%s) is greater than possible Buffer: ' +
`${kMaxLength} bytes`,
RangeError);
E('ERR_FS_INVALID_SYMLINK_TYPE',
'Symlink type must be one of "dir", "file", or "junction". Received "%s"',
Error); // Switch to TypeError. The current implementation does not seem right
Expand Down
1 change: 1 addition & 0 deletions lib/internal/modules/cjs/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,7 @@ Module._resolveFilename = function(request, parent, isMain, options) {
// look up the filename first, since that's the cache key.
var filename = Module._findPath(request, paths, isMain);
if (!filename) {
// eslint-disable-next-line no-restricted-syntax
var err = new Error(`Cannot find module '${request}'`);
err.code = 'MODULE_NOT_FOUND';
throw err;
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/process/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ function handledRejection(promise) {
if (promiseInfo.warned) {
const { uid } = promiseInfo;
// Generate the warning object early to get a good stack trace.
// eslint-disable-next-line no-restricted-syntax
const warning = new Error('Promise rejection was handled ' +
`asynchronously (rejection id: ${uid})`);
warning.name = 'PromiseRejectionHandledWarning';
Expand All @@ -53,6 +54,7 @@ function emitWarning(uid, reason) {
// ignored
}

// eslint-disable-next-line no-restricted-syntax
const warning = new Error(
'Unhandled promise rejection. This error originated either by ' +
'throwing inside of an async function without a catch block, ' +
Expand Down
1 change: 1 addition & 0 deletions lib/internal/process/warning.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ function setupProcessWarnings() {
if (type !== undefined && typeof type !== 'string')
throw new ERR_INVALID_ARG_TYPE('type', 'string');
if (warning === undefined || typeof warning === 'string') {
// eslint-disable-next-line no-restricted-syntax
warning = new Error(warning);
warning.name = String(type || 'Warning');
if (code !== undefined) warning.code = code;
Expand Down
3 changes: 2 additions & 1 deletion lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,7 @@ function writeAfterFIN(chunk, encoding, cb) {
encoding = null;
}

// eslint-disable-next-line no-restricted-syntax
var er = new Error('This socket has been ended by the other party');
er.code = 'EPIPE';
// TODO: defer error events consistently everywhere, not just the cb
Expand Down Expand Up @@ -935,7 +936,7 @@ function internalConnect(
localAddress = localAddress || '::';
err = self._handle.bind6(localAddress, localPort);
} else {
self.destroy(new TypeError('Invalid addressType: ' + addressType));
self.destroy(new ERR_INVALID_ADDRESS_FAMILY(addressType));
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is unreachable now, so I removed the test. I'm going to replace it by a CHECK when #19503 is fixed.

return;
}
debug('binding to localAddress: %s and localPort: %d (addressType: %d)',
Expand Down
1 change: 1 addition & 0 deletions lib/zlib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ function zlibOnError(message, errno) {
_close(self);
self._hadError = true;

// eslint-disable-next-line no-restricted-syntax
const error = new Error(message);
error.errno = errno;
error.code = codes[errno];
Expand Down
25 changes: 13 additions & 12 deletions test/sequential/test-inspector-port-cluster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ function testRunnerMain() {
function masterProcessMain() {
const workers = JSON.parse(process.env.workers);
const clusterSettings = JSON.parse(process.env.clusterSettings);
const badPortError = { type: RangeError, code: 'ERR_SOCKET_BAD_PORT' };
let debugPort = process.debugPort;

for (const worker of workers) {
Expand Down Expand Up @@ -234,36 +235,36 @@ function masterProcessMain() {
clusterSettings.inspectPort = 'string';
cluster.setupMaster(clusterSettings);

assert.throws(() => {
common.expectsError(() => {
cluster.fork(params).on('exit', common.mustCall(checkExitCode));
}, TypeError);
}, badPortError);

return;
} else if (clusterSettings.inspectPort === 'null') {
clusterSettings.inspectPort = null;
cluster.setupMaster(clusterSettings);

assert.throws(() => {
common.expectsError(() => {
cluster.fork(params).on('exit', common.mustCall(checkExitCode));
}, TypeError);
}, badPortError);

return;
} else if (clusterSettings.inspectPort === 'bignumber') {
clusterSettings.inspectPort = 1293812;
cluster.setupMaster(clusterSettings);

assert.throws(() => {
common.expectsError(() => {
cluster.fork(params).on('exit', common.mustCall(checkExitCode));
}, TypeError);
}, badPortError);

return;
} else if (clusterSettings.inspectPort === 'negativenumber') {
clusterSettings.inspectPort = -9776;
cluster.setupMaster(clusterSettings);

assert.throws(() => {
common.expectsError(() => {
cluster.fork(params).on('exit', common.mustCall(checkExitCode));
}, TypeError);
}, badPortError);

return;
} else if (clusterSettings.inspectPort === 'bignumberfunc') {
Expand All @@ -274,9 +275,9 @@ function masterProcessMain() {

cluster.setupMaster(clusterSettings);

assert.throws(() => {
common.expectsError(() => {
cluster.fork(params).on('exit', common.mustCall(checkExitCode));
}, TypeError);
}, badPortError);

return;
} else if (clusterSettings.inspectPort === 'strfunc') {
Expand All @@ -287,9 +288,9 @@ function masterProcessMain() {

cluster.setupMaster(clusterSettings);

assert.throws(() => {
common.expectsError(() => {
Copy link
Member

Choose a reason for hiding this comment

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

As a recommendation for the future: assert.throws is actually becoming more and more powerful and using it is probably better than using common.expectsError (even though that is still useful as a callback function).

Copy link
Member Author

Choose a reason for hiding this comment

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

What should I do to make it work with assert.throws?

This is what I get:

AssertionError [ERR_ASSERTION]: type: expected [Function: RangeError], not undefined
    at masterProcessMain (/home/mzasso/git/nodejs/node/test/sequential/test-inspector-port-cluster.js:238:16)
    at Object.<anonymous> (/home/mzasso/git/nodejs/node/test/sequential/test-inspector-port-cluster.js:347:3)

Copy link
Member

Choose a reason for hiding this comment

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

Right now there are some special handled properties in common.expectsError that do not exist in assert.throws. One is the message property: it is either a strict equal comparison or you can use a regular expression. Using regular expressions is not (yet) possible (I am still considering adding options to accept those as well) with assert.throws.
The other difference is the type property: it will actually check for the specific error type. That is not possible at all with assert.throws and instead you can check for the name property.

cluster.fork(params).on('exit', common.mustCall(checkExitCode));
}, TypeError);
}, badPortError);

return;
}
Expand Down