Skip to content

Commit

Permalink
lib: refactor Error.captureStackTrace() usage
Browse files Browse the repository at this point in the history 
When using `Errors.captureStackFrames` the error's stack property
is set again. This adds a helper function that wraps this functionality
in a simple API that does not only set the stack including the `code`
property but it also improves the performance to create the error.
The helper works for thrown errors and errors returned from wrapped
functions in case they are Node.js core errors.

PR-URL: #26738
Fixes: #26669
Fixes: #20253
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
  • Loading branch information
@BridgeAR
BridgeAR committed Mar 23, 2019
1 parent 1ed3c54 commit bfbce28
Show file tree
Hide file tree
Showing 15 changed files with 314 additions and 337 deletions.
59 changes: 22 additions & 37 deletions lib/_http_outgoing.js
View file
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
37 changes: 17 additions & 20 deletions lib/buffer.js
View file
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
21 changes: 12 additions & 9 deletions lib/fs.js
View file
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,10 +116,11 @@ function showTruncateDeprecation() {

function handleErrorFromBinding(ctx) {
if (ctx.errno !== undefined) { // libuv error numbers
const err = errors.uvException(ctx);
const err = uvException(ctx);
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.
Expand Down Expand Up @@ -310,7 +313,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
4 changes: 2 additions & 2 deletions lib/internal/crypto/pbkdf2.js
View file
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
79 changes: 59 additions & 20 deletions lib/internal/errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const codes = {};
const { kMaxLength } = internalBinding('buffer');
const { defineProperty } = Object;

let useOriginalName = false;
let excludedStackFn;

// Lazily loaded
let util;
Expand Down Expand Up @@ -49,7 +49,15 @@ function lazyBuffer() {
// and may have .path and .dest.
class SystemError extends Error {
constructor(key, context) {
super();
if (excludedStackFn === undefined) {
super();
} else {
const limit = Error.stackTraceLimit;
Error.stackTraceLimit = 0;
super();
// Reset the limit and setting the name property.
Error.stackTraceLimit = limit;
}
const prefix = getMessage(key, [], this);
let message = `${prefix}: ${context.syscall} returned ` +
`${context.code} (${context.message})`;
Expand Down Expand Up @@ -148,7 +156,15 @@ function makeSystemErrorWithCode(key) {
function makeNodeErrorWithCode(Base, key) {
return class NodeError extends Base {
constructor(...args) {
super();
if (excludedStackFn === undefined) {
super();
} else {
const limit = Error.stackTraceLimit;
Error.stackTraceLimit = 0;
super();
// Reset the limit and setting the name property.
Error.stackTraceLimit = limit;
}
const message = getMessage(key, args, this);
Object.defineProperty(this, 'message', {
value: message,
Expand Down Expand Up @@ -178,9 +194,30 @@ function makeNodeErrorWithCode(Base, key) {
};
}

// This function removes unnecessary frames from Node.js core errors.
function hideStackFrames(fn) {
return function hidden(...args) {
// Make sure the most outer `hideStackFrames()` function is used.
let setStackFn = false;
if (excludedStackFn === undefined) {
excludedStackFn = hidden;
setStackFn = true;
}
try {
return fn(...args);
} finally {
if (setStackFn === true) {
excludedStackFn = undefined;
}
}
};
}

function addCodeToName(err, name, code) {
if (useOriginalName) {
return;
// Set the stack
if (excludedStackFn !== undefined) {
// eslint-disable-next-line no-restricted-syntax
Error.captureStackTrace(err, excludedStackFn);
}
// Add the error code to the name to include it in the stack trace.
err.name = `${name} [${code}]`;
Expand Down Expand Up @@ -308,6 +345,7 @@ function uvException(ctx) {
err[prop] = ctx[prop];
}

// TODO(BridgeAR): Show the `code` property as part of the stack.
err.code = code;
if (path) {
err.path = path;
Expand All @@ -316,7 +354,7 @@ function uvException(ctx) {
err.dest = dest;
}

Error.captureStackTrace(err, uvException);
Error.captureStackTrace(err, excludedStackFn || uvException);
return err;
}

Expand Down Expand Up @@ -358,7 +396,7 @@ function uvExceptionWithHostPort(err, syscall, address, port) {
ex.port = port;
}

Error.captureStackTrace(ex, uvExceptionWithHostPort);
Error.captureStackTrace(ex, excludedStackFn || uvExceptionWithHostPort);
return ex;
}

Expand Down Expand Up @@ -386,7 +424,7 @@ function errnoException(err, syscall, original) {
ex.code = ex.errno = code;
ex.syscall = syscall;

Error.captureStackTrace(ex, errnoException);
Error.captureStackTrace(ex, excludedStackFn || errnoException);
return ex;
}

Expand Down Expand Up @@ -434,7 +472,7 @@ function exceptionWithHostPort(err, syscall, address, port, additional) {
ex.port = port;
}

Error.captureStackTrace(ex, exceptionWithHostPort);
Error.captureStackTrace(ex, excludedStackFn || exceptionWithHostPort);
return ex;
}

Expand Down Expand Up @@ -473,7 +511,8 @@ function dnsException(code, syscall, hostname) {
if (hostname) {
ex.hostname = hostname;
}
Error.captureStackTrace(ex, dnsException);

Error.captureStackTrace(ex, excludedStackFn || dnsException);
return ex;
}

Expand Down Expand Up @@ -523,21 +562,19 @@ function oneOf(expected, thing) {
}

module.exports = {
addCodeToName, // Exported for NghttpError
codes,
dnsException,
errnoException,
exceptionWithHostPort,
getMessage,
hideStackFrames,
isStackOverflowError,
uvException,
uvExceptionWithHostPort,
isStackOverflowError,
getMessage,
SystemError,
codes,
// This is exported only to facilitate testing.
E,
// This allows us to tell the type of the errors without using
// instanceof, which is necessary in WPT harness.
get useOriginalName() { return useOriginalName; },
set useOriginalName(value) { useOriginalName = value; }
E
};

// To declare an error message, use the E(sym, val, def) function above. The sym
Expand All @@ -556,7 +593,6 @@ module.exports = {
// Note: Please try to keep these in alphabetical order
//
// Note: Node.js specific errors must begin with the prefix ERR_

E('ERR_AMBIGUOUS_ARGUMENT', 'The "%s" argument is ambiguous. %s', TypeError);
E('ERR_ARG_NOT_ITERABLE', '%s must be iterable', TypeError);
E('ERR_ASSERTION', '%s', Error);
Expand Down Expand Up @@ -630,7 +666,10 @@ E('ERR_ENCODING_INVALID_ENCODED_DATA', function(encoding, ret) {
}, TypeError);
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_FALSY_VALUE_REJECTION', function(reason) {
this.reason = reason;
return 'Promise was rejected with falsy value';
}, Error);
E('ERR_FS_FILE_TOO_LARGE', 'File size (%s) is greater than possible Buffer: ' +
`${kMaxLength} bytes`,
RangeError);
Expand Down
Loading

0 comments on commit bfbce28

Please sign in to comment.