From c13eadc2180a2df75c17ba46b2bdaee5829b4bc9 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Wed, 14 Oct 2020 17:39:21 +0200 Subject: [PATCH] errors: eliminate all overhead for hidden calls Eliminate all overhead for function calls that are to be hidden from the stack traces at the expense of reduced performance for the error case Fixes: https://github.com/nodejs/node/issues/35386 PR-URL: https://github.com/nodejs/node/pull/35644 Reviewed-By: Matteo Collina Reviewed-By: Rich Trott Reviewed-By: Vladimir de Turckheim Reviewed-By: Antoine du Hamel --- benchmark/misc/hidestackframes.js | 45 +++ lib/internal/errors.js | 303 +++++++++--------- lib/internal/fs/promises.js | 4 +- test/message/esm_loader_not_found.out | 4 +- .../esm_loader_not_found_cjs_hint_bare.out | 1 + ...esm_loader_not_found_cjs_hint_relative.out | 4 +- test/message/internal_assert.out | 1 + test/message/internal_assert_fail.out | 1 + 8 files changed, 211 insertions(+), 152 deletions(-) create mode 100644 benchmark/misc/hidestackframes.js diff --git a/benchmark/misc/hidestackframes.js b/benchmark/misc/hidestackframes.js new file mode 100644 index 00000000000000..5b14f2d95b22e2 --- /dev/null +++ b/benchmark/misc/hidestackframes.js @@ -0,0 +1,45 @@ +'use strict'; + +const common = require('../common.js'); + +const bench = common.createBenchmark(main, { + type: ['hide-stackframes-throw', 'direct-call-throw', + 'hide-stackframes-noerr', 'direct-call-noerr'], + n: [10e4] +}, { + flags: ['--expose-internals'] +}); + +function main({ n, type }) { + const { + hideStackFrames, + codes: { + ERR_INVALID_ARG_TYPE, + }, + } = require('internal/errors'); + + const testfn = (value) => { + if (typeof value !== 'number') { + throw new ERR_INVALID_ARG_TYPE('Benchmark', 'number', value); + } + }; + + let fn = testfn; + if (type.startsWith('hide-stackframe')) + fn = hideStackFrames(testfn); + let value = 42; + if (type.endsWith('-throw')) + value = 'err'; + + bench.start(); + + for (let i = 0; i < n; i++) { + try { + fn(value); + } catch { + // No-op + } + } + + bench.end(n); +} diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 94c08a101d3d19..cac666ae93a4ca 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -76,6 +76,8 @@ const kTypes = [ const MainContextError = Error; const overrideStackTrace = new SafeWeakMap(); const kNoOverride = Symbol('kNoOverride'); +let userStackTraceLimit; +const nodeInternalPrefix = '__node_internal_'; const prepareStackTrace = (globalThis, error, trace) => { // API for node internals to override error stack formatting // without interfering with userland code. @@ -85,6 +87,21 @@ const prepareStackTrace = (globalThis, error, trace) => { return f(error, trace); } + const firstFrame = trace[0]?.getFunctionName(); + if (firstFrame && StringPrototypeStartsWith(firstFrame, nodeInternalPrefix)) { + for (let l = trace.length - 1; l >= 0; l--) { + const fn = trace[l]?.getFunctionName(); + if (fn && StringPrototypeStartsWith(fn, nodeInternalPrefix)) { + ArrayPrototypeSplice(trace, 0, l + 1); + break; + } + } + // `userStackTraceLimit` is the user value for `Error.stackTraceLimit`, + // it is updated at every new exception in `captureLargerStackTrace`. + if (trace.length > userStackTraceLimit) + ArrayPrototypeSplice(trace, userStackTraceLimit); + } + const globalOverride = maybeOverridePrepareStackTrace(globalThis, error, trace); if (globalOverride !== kNoOverride) return globalOverride; @@ -119,8 +136,6 @@ const maybeOverridePrepareStackTrace = (globalThis, error, trace) => { return kNoOverride; }; -let excludedStackFn; - // Lazily loaded let util; let assert; @@ -148,6 +163,27 @@ function lazyBuffer() { return buffer; } +const addCodeToName = hideStackFrames(function addCodeToName(err, name, code) { + // Set the stack + err = captureLargerStackTrace(err); + // Add the error code to the name to include it in the stack trace. + err.name = `${name} [${code}]`; + // Access the stack to generate the error message including the error code + // from the name. + err.stack; // eslint-disable-line no-unused-expressions + // Reset the name to the actual name. + if (name === 'SystemError') { + ObjectDefineProperty(err, 'name', { + value: name, + enumerable: false, + writable: true, + configurable: true + }); + } else { + delete err.name; + } +}); + // A specialized Error that includes an additional info property with // additional information about the error condition. // It has the properties present in a UVException but with a custom error @@ -158,15 +194,11 @@ function lazyBuffer() { // and may have .path and .dest. class SystemError extends Error { constructor(key, context) { - 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 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})`; @@ -275,15 +307,11 @@ function makeSystemErrorWithCode(key) { function makeNodeErrorWithCode(Base, key) { return class NodeError extends Base { constructor(...args) { - 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 limit = Error.stackTraceLimit; + Error.stackTraceLimit = 0; + super(); + // Reset the limit and setting the name property. + Error.stackTraceLimit = limit; const message = getMessage(key, args, this); ObjectDefineProperty(this, 'message', { value: message, @@ -303,44 +331,11 @@ 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) { - // Set the stack - if (excludedStackFn !== undefined) { - ErrorCaptureStackTrace(err, excludedStackFn); - } - // Add the error code to the name to include it in the stack trace. - err.name = `${name} [${code}]`; - // Access the stack to generate the error message including the error code - // from the name. - err.stack; // eslint-disable-line no-unused-expressions - // Reset the name to the actual name. - if (name === 'SystemError') { - ObjectDefineProperty(err, 'name', { - value: name, - enumerable: false, - writable: true, - configurable: true - }); - } else { - delete err.name; - } + // We rename the functions that will be hidden to cut off the stacktrace + // at the outermost one + const hidden = nodeInternalPrefix + fn.name; + ObjectDefineProperty(fn, 'name', { value: hidden }); + return fn; } // Utility function for registering the error codes. Only used here. Exported @@ -410,6 +405,16 @@ function uvErrmapGet(name) { return MapPrototypeGet(uvBinding.errmap, name); } +const captureLargerStackTrace = hideStackFrames( + function captureLargerStackTrace(err) { + userStackTraceLimit = Error.stackTraceLimit; + Error.stackTraceLimit = Infinity; + ErrorCaptureStackTrace(err); + // Reset the limit + Error.stackTraceLimit = userStackTraceLimit; + + return err; + }); /** * This creates an error compatible with errors produced in the C++ @@ -420,7 +425,7 @@ function uvErrmapGet(name) { * @param {Object} ctx * @returns {Error} */ -function uvException(ctx) { +const uvException = hideStackFrames(function uvException(ctx) { const { 0: code, 1: uvmsg } = uvErrmapGet(ctx.errno) || uvUnmappedError; let message = `${code}: ${ctx.message || uvmsg}, ${ctx.syscall}`; @@ -460,9 +465,9 @@ function uvException(ctx) { if (dest) { err.dest = dest; } - ErrorCaptureStackTrace(err, excludedStackFn || uvException); - return err; -} + + return captureLargerStackTrace(err); +}); /** * This creates an error compatible with errors produced in the C++ @@ -475,35 +480,36 @@ function uvException(ctx) { * @param {number} [port] * @returns {Error} */ -function uvExceptionWithHostPort(err, syscall, address, port) { - const { 0: code, 1: uvmsg } = uvErrmapGet(err) || uvUnmappedError; - const message = `${syscall} ${code}: ${uvmsg}`; - let details = ''; - - if (port && port > 0) { - details = ` ${address}:${port}`; - } else if (address) { - details = ` ${address}`; - } +const uvExceptionWithHostPort = hideStackFrames( + function uvExceptionWithHostPort(err, syscall, address, port) { + const { 0: code, 1: uvmsg } = uvErrmapGet(err) || uvUnmappedError; + const message = `${syscall} ${code}: ${uvmsg}`; + let details = ''; + + if (port && port > 0) { + details = ` ${address}:${port}`; + } else if (address) { + details = ` ${address}`; + } - // 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; - // eslint-disable-next-line no-restricted-syntax - const ex = new Error(`${message}${details}`); - Error.stackTraceLimit = tmpLimit; - ex.code = code; - ex.errno = err; - ex.syscall = syscall; - ex.address = address; - if (port) { - ex.port = port; - } - ErrorCaptureStackTrace(ex, excludedStackFn || uvExceptionWithHostPort); - return ex; -} + // 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; + // eslint-disable-next-line no-restricted-syntax + const ex = new Error(`${message}${details}`); + Error.stackTraceLimit = tmpLimit; + ex.code = code; + ex.errno = err; + ex.syscall = syscall; + ex.address = address; + if (port) { + ex.port = port; + } + + return captureLargerStackTrace(ex); + }); /** * This used to be util._errnoException(). @@ -513,24 +519,28 @@ function uvExceptionWithHostPort(err, syscall, address, port) { * @param {string} [original] * @returns {Error} */ -function errnoException(err, syscall, original) { - // TODO(joyeecheung): We have to use the type-checked - // getSystemErrorName(err) to guard against invalid arguments from users. - // This can be replaced with [ code ] = errmap.get(err) when this method - // is no longer exposed to user land. - if (util === undefined) util = require('util'); - const code = util.getSystemErrorName(err); - const message = original ? - `${syscall} ${code} ${original}` : `${syscall} ${code}`; - - // eslint-disable-next-line no-restricted-syntax - const ex = new Error(message); - ex.errno = err; - ex.code = code; - ex.syscall = syscall; - ErrorCaptureStackTrace(ex, excludedStackFn || errnoException); - return ex; -} +const errnoException = hideStackFrames( + function errnoException(err, syscall, original) { + // TODO(joyeecheung): We have to use the type-checked + // getSystemErrorName(err) to guard against invalid arguments from users. + // This can be replaced with [ code ] = errmap.get(err) when this method + // is no longer exposed to user land. + if (util === undefined) util = require('util'); + const code = util.getSystemErrorName(err); + const message = original ? + `${syscall} ${code} ${original}` : `${syscall} ${code}`; + + const tmpLimit = Error.stackTraceLimit; + Error.stackTraceLimit = 0; + // eslint-disable-next-line no-restricted-syntax + const ex = new Error(message); + Error.stackTraceLimit = tmpLimit; + ex.errno = err; + ex.code = code; + ex.syscall = syscall; + + return captureLargerStackTrace(ex); + }); /** * Deprecated, new function is `uvExceptionWithHostPort()` @@ -543,41 +553,42 @@ function errnoException(err, syscall, original) { * @param {string} [additional] * @returns {Error} */ -function exceptionWithHostPort(err, syscall, address, port, additional) { - // TODO(joyeecheung): We have to use the type-checked - // getSystemErrorName(err) to guard against invalid arguments from users. - // This can be replaced with [ code ] = errmap.get(err) when this method - // is no longer exposed to user land. - if (util === undefined) util = require('util'); - const code = util.getSystemErrorName(err); - let details = ''; - if (port && port > 0) { - details = ` ${address}:${port}`; - } else if (address) { - details = ` ${address}`; - } - if (additional) { - details += ` - Local (${additional})`; - } +const exceptionWithHostPort = hideStackFrames( + function exceptionWithHostPort(err, syscall, address, port, additional) { + // TODO(joyeecheung): We have to use the type-checked + // getSystemErrorName(err) to guard against invalid arguments from users. + // This can be replaced with [ code ] = errmap.get(err) when this method + // is no longer exposed to user land. + if (util === undefined) util = require('util'); + const code = util.getSystemErrorName(err); + let details = ''; + if (port && port > 0) { + details = ` ${address}:${port}`; + } else if (address) { + details = ` ${address}`; + } + if (additional) { + details += ` - Local (${additional})`; + } - // 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; - // eslint-disable-next-line no-restricted-syntax - const ex = new Error(`${syscall} ${code}${details}`); - Error.stackTraceLimit = tmpLimit; - ex.errno = err; - ex.code = code; - ex.syscall = syscall; - ex.address = address; - if (port) { - ex.port = port; - } - ErrorCaptureStackTrace(ex, excludedStackFn || exceptionWithHostPort); - return ex; -} + // 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; + // eslint-disable-next-line no-restricted-syntax + const ex = new Error(`${syscall} ${code}${details}`); + Error.stackTraceLimit = tmpLimit; + ex.errno = err; + ex.code = code; + ex.syscall = syscall; + ex.address = address; + if (port) { + ex.port = port; + } + + return captureLargerStackTrace(ex); + }); /** * @param {number|string} code - A libuv error number or a c-ares error code @@ -585,7 +596,7 @@ function exceptionWithHostPort(err, syscall, address, port, additional) { * @param {string} [hostname] * @returns {Error} */ -function dnsException(code, syscall, hostname) { +const dnsException = hideStackFrames(function(code, syscall, hostname) { let errno; // If `code` is of type number, it is a libuv error number, else it is a // c-ares error code. @@ -619,9 +630,9 @@ function dnsException(code, syscall, hostname) { if (hostname) { ex.hostname = hostname; } - ErrorCaptureStackTrace(ex, excludedStackFn || dnsException); - return ex; -} + + return captureLargerStackTrace(ex); +}); function connResetException(msg) { // eslint-disable-next-line no-restricted-syntax diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 42ede0b9dc4e16..767195cc2ec738 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -261,7 +261,7 @@ async function writeFileHandle(filehandle, data, signal) { if (remaining === 0) return; do { if (signal?.aborted) { - throw new lazyDOMException('The operation was aborted', 'AbortError'); + throw lazyDOMException('The operation was aborted', 'AbortError'); } const { bytesWritten } = await write(filehandle, data, 0, @@ -655,7 +655,7 @@ async function writeFile(path, data, options) { return writeFileHandle(path, data, options.signal); if (options.signal?.aborted) { - throw new lazyDOMException('The operation was aborted', 'AbortError'); + throw lazyDOMException('The operation was aborted', 'AbortError'); } const fd = await open(path, flag, options.mode); diff --git a/test/message/esm_loader_not_found.out b/test/message/esm_loader_not_found.out index 26292512d9b002..61291a9bbc5ec1 100644 --- a/test/message/esm_loader_not_found.out +++ b/test/message/esm_loader_not_found.out @@ -4,6 +4,7 @@ internal/process/esm_loader.js:* internalBinding('errors').triggerUncaughtException( ^ Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'i-dont-exist' imported from * + at new NodeError (internal/errors.js:*:*) at packageResolve (internal/modules/esm/resolve.js:*:*) at moduleResolve (internal/modules/esm/resolve.js:*:*) at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:*:*) @@ -12,7 +13,6 @@ Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'i-dont-exist' imported from * at Loader.import (internal/modules/esm/loader.js:*:*) at internal/process/esm_loader.js:*:* at initializeLoader (internal/process/esm_loader.js:*:*) - at Object.loadESM (internal/process/esm_loader.js:*:*) - at runMainESM (internal/modules/run_main.js:*:*) { + at Object.loadESM (internal/process/esm_loader.js:*:*) { code: 'ERR_MODULE_NOT_FOUND' } diff --git a/test/message/esm_loader_not_found_cjs_hint_bare.out b/test/message/esm_loader_not_found_cjs_hint_bare.out index c3f758577eb4d6..c6164006300094 100644 --- a/test/message/esm_loader_not_found_cjs_hint_bare.out +++ b/test/message/esm_loader_not_found_cjs_hint_bare.out @@ -4,6 +4,7 @@ internal/process/esm_loader.js:* Error [ERR_MODULE_NOT_FOUND]: Cannot find module '*test*fixtures*node_modules*some_module*obj' imported from *test*fixtures*esm_loader_not_found_cjs_hint_bare.mjs Did you mean to import some_module/obj.js? + at new NodeError (internal/errors.js:*:*) at finalizeResolution (internal/modules/esm/resolve.js:*:*) at moduleResolve (internal/modules/esm/resolve.js:*:*) at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:*:*) diff --git a/test/message/esm_loader_not_found_cjs_hint_relative.out b/test/message/esm_loader_not_found_cjs_hint_relative.out index 1c43c0d3a2eb31..edfe27a1f3b461 100644 --- a/test/message/esm_loader_not_found_cjs_hint_relative.out +++ b/test/message/esm_loader_not_found_cjs_hint_relative.out @@ -6,6 +6,7 @@ internal/process/esm_loader.js:* Error [ERR_MODULE_NOT_FOUND]: Cannot find module '*test*common*fixtures' imported from * Did you mean to import ./test/common/fixtures.js? + at new NodeError (internal/errors.js:*:*) at finalizeResolution (internal/modules/esm/resolve.js:*:*) at moduleResolve (internal/modules/esm/resolve.js:*:*) at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:*:*) @@ -14,7 +15,6 @@ Did you mean to import ./test/common/fixtures.js? at Loader.import (internal/modules/esm/loader.js:*:*) at internal/process/esm_loader.js:*:* at initializeLoader (internal/process/esm_loader.js:*:*) - at Object.loadESM (internal/process/esm_loader.js:*:*) - at runMainESM (internal/modules/run_main.js:*:*) { + at Object.loadESM (internal/process/esm_loader.js:*:*) { code: 'ERR_MODULE_NOT_FOUND' } diff --git a/test/message/internal_assert.out b/test/message/internal_assert.out index cf09fdcb605269..9ca8350756c9ad 100644 --- a/test/message/internal_assert.out +++ b/test/message/internal_assert.out @@ -5,6 +5,7 @@ internal/assert.js:* Error [ERR_INTERNAL_ASSERTION]: This is caused by either a bug in Node.js or incorrect usage of Node.js internals. Please open an issue with this stack trace at https://github.com/nodejs/node/issues + at new NodeError (internal/errors.js:*:*) at assert (internal/assert.js:*:*) at * (*test*message*internal_assert.js:7:1) at * diff --git a/test/message/internal_assert_fail.out b/test/message/internal_assert_fail.out index 11b532b7b2af3c..11e253703170d2 100644 --- a/test/message/internal_assert_fail.out +++ b/test/message/internal_assert_fail.out @@ -6,6 +6,7 @@ Error [ERR_INTERNAL_ASSERTION]: Unreachable! This is caused by either a bug in Node.js or incorrect usage of Node.js internals. Please open an issue with this stack trace at https://github.com/nodejs/node/issues + at new NodeError (internal/errors.js:*:*) at Function.fail (internal/assert.js:*:*) at * (*test*message*internal_assert_fail.js:7:8) at *