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

lib: fix error name #26738

Closed
wants to merge 6 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
2 changes: 2 additions & 0 deletions lib/.eslintrc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ rules:
# Config specific to lib
- selector: "NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError)$/])"
message: "Use an error exported by the internal/errors module."
- selector: "CallExpression[callee.object.name='Error'][callee.property.name='captureStackTrace']"
Copy link
Member

Choose a reason for hiding this comment

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

Given that there are 17 exceptions to this rule in lib, are we sure this is a good rule to have?

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be difficult to identify the cases which should not use Error.captureStackTrace without this rule.

I count 16 exceptions while 7 of those are all inside internal/errors and I would normally just add a file wide exception explicitly for this case but that would require to write an individual rule instead of using the no-restricted-syntax rule.

Three more cases would ideally also switch to using hideStackFrames() but C++ errors still work quite differently than our JS errors (besides N-API errors which try to mirror the JS side) and handling errors inside callbacks is not yet possible. I already have an idea to improve those further if this PR lands.

That leaves six other cases of which five seem to be exceptions to the rule in a way that they are neither directly related to our errors nor should they ever use hideStackFrames().

Copy link
Member Author

@BridgeAR BridgeAR Mar 20, 2019

Choose a reason for hiding this comment

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

I updated the hideStackFrames() function and now there is one are two exception less than before.

message: "Please use `require('internal/errors').hideStackFrames()` instead."
# Custom rules in tools/eslint-rules
node-core/require-globals: error
node-core/no-let-in-for-declaration: error
Expand Down
59 changes: 22 additions & 37 deletions lib/_http_outgoing.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,19 @@ const {
symbols: { async_id_symbol }
} = require('internal/async_hooks');
const {
ERR_HTTP_HEADERS_SENT,
ERR_HTTP_INVALID_HEADER_VALUE,
ERR_HTTP_TRAILER_INVALID,
ERR_INVALID_HTTP_TOKEN,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_CHAR,
ERR_METHOD_NOT_IMPLEMENTED,
ERR_STREAM_CANNOT_PIPE,
ERR_STREAM_WRITE_AFTER_END
} = require('internal/errors').codes;
codes: {
ERR_HTTP_HEADERS_SENT,
ERR_HTTP_INVALID_HEADER_VALUE,
ERR_HTTP_TRAILER_INVALID,
ERR_INVALID_HTTP_TOKEN,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_CHAR,
ERR_METHOD_NOT_IMPLEMENTED,
ERR_STREAM_CANNOT_PIPE,
ERR_STREAM_WRITE_AFTER_END
},
hideStackFrames
} = require('internal/errors');
const { validateString } = require('internal/validators');

const { CRLF, debug } = common;
Expand Down Expand Up @@ -443,39 +446,21 @@ function matchHeader(self, state, field, value) {
}
}

function validateHeaderName(name) {
const validateHeaderName = hideStackFrames((name) => {
if (typeof name !== 'string' || !name || !checkIsHttpToken(name)) {
// Reducing the limit improves the performance significantly. We do not
// lose the stack frames due to the `captureStackTrace()` function that is
// called later.
const tmpLimit = Error.stackTraceLimit;
Error.stackTraceLimit = 0;
const err = new ERR_INVALID_HTTP_TOKEN('Header name', name);
Error.stackTraceLimit = tmpLimit;
Error.captureStackTrace(err, validateHeaderName);
throw err;
throw new ERR_INVALID_HTTP_TOKEN('Header name', name);
}
}
});

function validateHeaderValue(name, value) {
let err;
// Reducing the limit improves the performance significantly. We do not loose
// the stack frames due to the `captureStackTrace()` function that is called
// later.
const tmpLimit = Error.stackTraceLimit;
Error.stackTraceLimit = 0;
const validateHeaderValue = hideStackFrames((name, value) => {
if (value === undefined) {
err = new ERR_HTTP_INVALID_HEADER_VALUE(value, name);
} else if (checkInvalidHeaderChar(value)) {
debug('Header "%s" contains invalid characters', name);
err = new ERR_INVALID_CHAR('header content', name);
throw new ERR_HTTP_INVALID_HEADER_VALUE(value, name);
}
Error.stackTraceLimit = tmpLimit;
if (err !== undefined) {
Error.captureStackTrace(err, validateHeaderValue);
throw err;
if (checkInvalidHeaderChar(value)) {
debug('Header "%s" contains invalid characters', name);
throw new ERR_INVALID_CHAR('header content', name);
}
}
});

OutgoingMessage.prototype.setHeader = function setHeader(name, value) {
if (this._header) {
Expand Down
1 change: 1 addition & 0 deletions lib/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ function getErrMessage(message, fn) {
// We only need the stack trace. To minimize the overhead use an object
// instead of an error.
const err = {};
// eslint-disable-next-line no-restricted-syntax
Error.captureStackTrace(err, fn);
Error.stackTraceLimit = tmpLimit;

Expand Down
37 changes: 17 additions & 20 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,18 @@ const {
} = require('internal/util/inspect');

const {
ERR_BUFFER_OUT_OF_BOUNDS,
ERR_OUT_OF_RANGE,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_INVALID_BUFFER_SIZE,
ERR_INVALID_OPT_VALUE,
ERR_NO_LONGER_SUPPORTED,
ERR_UNKNOWN_ENCODING
} = require('internal/errors').codes;
codes: {
ERR_BUFFER_OUT_OF_BOUNDS,
ERR_OUT_OF_RANGE,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_INVALID_BUFFER_SIZE,
ERR_INVALID_OPT_VALUE,
ERR_NO_LONGER_SUPPORTED,
ERR_UNKNOWN_ENCODING
},
hideStackFrames
} = require('internal/errors');
const { validateString } = require('internal/validators');

const {
Expand Down Expand Up @@ -247,20 +250,14 @@ Object.setPrototypeOf(Buffer, Uint8Array);
// The 'assertSize' method will remove itself from the callstack when an error
// occurs. This is done simply to keep the internal details of the
// implementation from bleeding out to users.
function assertSize(size) {
let err = null;

const assertSize = hideStackFrames((size) => {
if (typeof size !== 'number') {
err = new ERR_INVALID_ARG_TYPE('size', 'number', size);
} else if (!(size >= 0 && size <= kMaxLength)) {
err = new ERR_INVALID_OPT_VALUE.RangeError('size', size);
throw new ERR_INVALID_ARG_TYPE('size', 'number', size);
}

if (err !== null) {
Error.captureStackTrace(err, assertSize);
throw err;
if (!(size >= 0 && size <= kMaxLength)) {
throw new ERR_INVALID_OPT_VALUE.RangeError('size', size);
}
}
});

/**
* Creates a new filled Buffer instance.
Expand Down
1 change: 1 addition & 0 deletions lib/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
try {
const { kExpandStackSymbol } = require('internal/util');
const capture = {};
// eslint-disable-next-line no-restricted-syntax
Error.captureStackTrace(capture, EventEmitter.prototype.emit);
Object.defineProperty(er, kExpandStackSymbol, {
value: enhanceStackTrace.bind(null, er, capture),
Expand Down
23 changes: 14 additions & 9 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,15 @@ const pathModule = require('path');
const { isArrayBufferView } = require('internal/util/types');
const binding = internalBinding('fs');
const { Buffer, kMaxLength } = require('buffer');
const errors = require('internal/errors');
const {
ERR_FS_FILE_TOO_LARGE,
ERR_INVALID_ARG_VALUE,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_CALLBACK
} = errors.codes;
codes: {
ERR_FS_FILE_TOO_LARGE,
ERR_INVALID_ARG_VALUE,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_CALLBACK
},
uvException
} = require('internal/errors');

const { FSReqCallback, statValues } = binding;
const { toPathIfFileURL } = require('internal/url');
Expand Down Expand Up @@ -114,13 +116,16 @@ function showTruncateDeprecation() {

function handleErrorFromBinding(ctx) {
if (ctx.errno !== undefined) { // libuv error numbers
const err = errors.uvException(ctx);
const err = uvException(ctx);
// eslint-disable-next-line no-restricted-syntax
Error.captureStackTrace(err, handleErrorFromBinding);
throw err;
} else if (ctx.error !== undefined) { // errors created in C++ land.
}
if (ctx.error !== undefined) { // errors created in C++ land.
// TODO(joyeecheung): currently, ctx.error are encoding errors
// usually caused by memory problems. We need to figure out proper error
// code(s) for this.
// eslint-disable-next-line no-restricted-syntax
Error.captureStackTrace(ctx.error, handleErrorFromBinding);
throw ctx.error;
}
Expand Down Expand Up @@ -310,7 +315,7 @@ function tryStatSync(fd, isUserFd) {
const stats = binding.fstat(fd, false, undefined, ctx);
if (ctx.errno !== undefined && !isUserFd) {
fs.closeSync(fd);
throw errors.uvException(ctx);
throw uvException(ctx);
}
return stats;
}
Expand Down
21 changes: 20 additions & 1 deletion lib/internal/assert/assertion_error.js
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,9 @@ class AssertionError extends Error {
stackStartFn
} = options;

const limit = Error.stackTraceLimit;
Error.stackTraceLimit = 0;

if (message != null) {
super(String(message));
} else {
Expand Down Expand Up @@ -387,13 +390,29 @@ class AssertionError extends Error {
}
}

Error.stackTraceLimit = limit;

this.generatedMessage = !message;
this.name = 'AssertionError [ERR_ASSERTION]';
Object.defineProperty(this, 'name', {
value: 'AssertionError [ERR_ASSERTION]',
enumerable: false,
writable: true,
configurable: true
});
this.code = 'ERR_ASSERTION';
this.actual = actual;
this.expected = expected;
this.operator = operator;
// eslint-disable-next-line no-restricted-syntax
Error.captureStackTrace(this, stackStartFn);
// Create error message including the error code in the name.
this.stack;
// Reset the name.
this.name = 'AssertionError';
}

toString() {
return `${this.name} [${this.code}]: ${this.message}`;
}

[inspect.custom](recurseTimes, ctx) {
Expand Down
1 change: 1 addition & 0 deletions lib/internal/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ function fatalError(e) {
process._rawDebug(e.stack);
} else {
const o = { message: e };
// eslint-disable-next-line no-restricted-syntax
Error.captureStackTrace(o, fatalError);
process._rawDebug(o.stack);
}
Expand Down
1 change: 1 addition & 0 deletions lib/internal/console/constructor.js
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ const consoleMethods = {
name: 'Trace',
message: this[kFormatForStderr](args)
};
// eslint-disable-next-line no-restricted-syntax
Error.captureStackTrace(err, this.trace);
this.error(err.stack);
},
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/crypto/pbkdf2.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ function check(password, salt, iterations, keylen, digest) {

password = validateArrayBufferView(password, 'password');
salt = validateArrayBufferView(salt, 'salt');
iterations = validateUint32(iterations, 'iterations', 0);
keylen = validateUint32(keylen, 'keylen', 0);
validateUint32(iterations, 'iterations', 0);
validateUint32(keylen, 'keylen', 0);

return { password, salt, iterations, keylen, digest };
}
Expand Down
Loading