errors: remove eager stack generation for node errors

PR-URL: #39182 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Jul 11, 2021 · 33cad27 · 33cad27
1 parent a7cd40e
commit 33cad27
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 80 deletions.
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -33,6 +33,7 @@ const {
   Number,
   NumberIsInteger,
   ObjectDefineProperty,
+  ObjectDefineProperties,
   ObjectIsExtensible,
   ObjectGetOwnPropertyDescriptor,
   ObjectKeys,
@@ -58,6 +59,8 @@ const {
   URIError,
 } = primordials;
 
+const kIsNodeError = Symbol('kIsNodeError');
+
 const isWindows = process.platform === 'win32';
 
 const messages = new SafeMap();
@@ -116,7 +119,12 @@ const prepareStackTrace = (globalThis, error, trace) => {
   // Error: Message
   //     at function (file)
   //     at file
-  const errorString = ErrorPrototypeToString(error);
+  let errorString;
+  if (kIsNodeError in error) {
+    errorString = `${error.name} [${error.code}]: ${error.message}`;
+  } else {
+    errorString = ErrorPrototypeToString(error);
+  }
   if (trace.length === 0) {
     return errorString;
   }
@@ -186,27 +194,6 @@ 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;
-  }
-});
-
 function isErrorStackTraceLimitWritable() {
   const desc = ObjectGetOwnPropertyDescriptor(Error, 'stackTraceLimit');
   if (desc === undefined) {
@@ -242,43 +229,55 @@ class SystemError extends Error {
     if (context.dest !== undefined)
       message += ` => ${context.dest}`;
 
-    ObjectDefineProperty(this, 'message', {
-      value: message,
-      enumerable: false,
-      writable: true,
-      configurable: true
-    });
-    addCodeToName(this, 'SystemError', key);
+    captureLargerStackTrace(this);
 
     this.code = key;
 
-    ObjectDefineProperty(this, 'info', {
-      value: context,
-      enumerable: true,
-      configurable: true,
-      writable: false
-    });
-
-    ObjectDefineProperty(this, 'errno', {
-      get() {
-        return context.errno;
+    ObjectDefineProperties(this, {
+      [kIsNodeError]: {
+        value: true,
+        enumerable: false,
+        writable: false,
+        configurable: true,
       },
-      set: (value) => {
-        context.errno = value;
+      name: {
+        value: 'SystemError',
+        enumerable: false,
+        writable: true,
+        configurable: true,
       },
-      enumerable: true,
-      configurable: true
-    });
-
-    ObjectDefineProperty(this, 'syscall', {
-      get() {
-        return context.syscall;
+      message: {
+        value: message,
+        enumerable: false,
+        writable: true,
+        configurable: true,
+      },
+      info: {
+        value: context,
+        enumerable: true,
+        configurable: true,
+        writable: false,
       },
-      set: (value) => {
-        context.syscall = value;
+      errno: {
+        get() {
+          return context.errno;
+        },
+        set: (value) => {
+          context.errno = value;
+        },
+        enumerable: true,
+        configurable: true,
+      },
+      syscall: {
+        get() {
+          return context.syscall;
+        },
+        set: (value) => {
+          context.syscall = value;
+        },
+        enumerable: true,
+        configurable: true,
       },
-      enumerable: true,
-      configurable: true
     });
 
     if (context.path !== undefined) {
@@ -346,21 +345,29 @@ function makeNodeErrorWithCode(Base, key) {
     // Reset the limit and setting the name property.
     if (isErrorStackTraceLimitWritable()) Error.stackTraceLimit = limit;
     const message = getMessage(key, args, error);
-    ObjectDefineProperty(error, 'message', {
-      value: message,
-      enumerable: false,
-      writable: true,
-      configurable: true,
-    });
-    ObjectDefineProperty(error, 'toString', {
-      value() {
-        return `${this.name} [${key}]: ${this.message}`;
+    ObjectDefineProperties(error, {
+      [kIsNodeError]: {
+        value: true,
+        enumerable: false,
+        writable: false,
+        configurable: true,
+      },
+      message: {
+        value: message,
+        enumerable: false,
+        writable: true,
+        configurable: true,
+      },
+      toString: {
+        value() {
+          return `${this.name} [${key}]: ${this.message}`;
+        },
+        enumerable: false,
+        writable: true,
+        configurable: true,
       },
-      enumerable: false,
-      writable: true,
-      configurable: true,
     });
-    addCodeToName(error, Base.name, key);
+    captureLargerStackTrace(error);
     error.code = key;
     return error;
   };
@@ -792,7 +799,6 @@ class AbortError extends Error {
   }
 }
 module.exports = {
-  addCodeToName, // Exported for NghttpError
   aggregateTwoErrors,
   codes,
   dnsException,
@@ -815,7 +821,9 @@ module.exports = {
   maybeOverridePrepareStackTrace,
   overrideStackTrace,
   kEnhanceStackBeforeInspector,
-  fatalExceptionStackEnhancers
+  fatalExceptionStackEnhancers,
+  kIsNodeError,
+  captureLargerStackTrace,
 };
 
 // To declare an error message, use the E(sym, val, def) function above. The sym

diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js
@@ -9,6 +9,7 @@ const {
   MathMax,
   Number,
   ObjectCreate,
+  ObjectDefineProperty,
   ObjectKeys,
   SafeSet,
   String,
@@ -28,9 +29,10 @@ const {
     ERR_INVALID_ARG_TYPE,
     ERR_INVALID_HTTP_TOKEN
   },
-  addCodeToName,
+  captureLargerStackTrace,
   getMessage,
-  hideStackFrames
+  hideStackFrames,
+  kIsNodeError,
 } = require('internal/errors');
 
 const kSensitiveHeaders = Symbol('nodejs.http2.sensitiveHeaders');
@@ -550,7 +552,13 @@ class NghttpError extends Error {
       binding.nghttp2ErrorString(integerCode));
     this.code = customErrorCode || 'ERR_HTTP2_ERROR';
     this.errno = integerCode;
-    addCodeToName(this, super.name, this.code);
+    captureLargerStackTrace(this);
+    ObjectDefineProperty(this, kIsNodeError, {
+      value: true,
+      enumerable: false,
+      writable: false,
+      configurable: true,
+    });
   }
 
   toString() {

diff --git a/lib/internal/source_map/prepare_stack_trace.js b/lib/internal/source_map/prepare_stack_trace.js
@@ -21,7 +21,8 @@ const { findSourceMap } = require('internal/source_map/source_map_cache');
 const {
   kNoOverride,
   overrideStackTrace,
-  maybeOverridePrepareStackTrace
+  maybeOverridePrepareStackTrace,
+  kIsNodeError,
 } = require('internal/errors');
 const { fileURLToPath } = require('internal/url');
 
@@ -41,7 +42,12 @@ const prepareStackTrace = (globalThis, error, trace) => {
     maybeOverridePrepareStackTrace(globalThis, error, trace);
   if (globalOverride !== kNoOverride) return globalOverride;
 
-  const errorString = ErrorPrototypeToString(error);
+  let errorString;
+  if (kIsNodeError in error) {
+    errorString = `${error.name} [${error.code}]: ${error.message}`;
+  } else {
+    errorString = ErrorPrototypeToString(error);
+  }
 
   if (trace.length === 0) {
     return errorString;

diff --git a/test/message/esm_loader_not_found.out b/test/message/esm_loader_not_found.out
@@ -1,8 +1,8 @@
 (node:*) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
 (Use `* --trace-warnings ...` to show where the warning was created)
-node:internal/process/esm_loader:*
-    internalBinding('errors').triggerUncaughtException(
-                              ^
+node:internal/errors:*
+    ErrorCaptureStackTrace(err);
+    ^
 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'i-dont-exist' imported from *
     at new NodeError (node:internal/errors:*:*)
     at packageResolve (node:internal/modules/esm/resolve:*:*)

diff --git a/test/parallel/test-repl-top-level-await.js b/test/parallel/test-repl-top-level-await.js
@@ -175,15 +175,17 @@ async function ordinaryTests() {
 
 async function ctrlCTest() {
   console.log('Testing Ctrl+C');
-  assert.deepStrictEqual(await runAndWait([
+  const output = await runAndWait([
     'await new Promise(() => {})',
     { ctrl: true, name: 'c' },
-  ]), [
+  ]);
+  assert.deepStrictEqual(output.slice(0, 3), [
     'await new Promise(() => {})\r',
     'Uncaught:',
-    '[Error [ERR_SCRIPT_EXECUTION_INTERRUPTED]: ' +
-      'Script execution was interrupted by `SIGINT`] {',
-    "  code: 'ERR_SCRIPT_EXECUTION_INTERRUPTED'",
+    'Error [ERR_SCRIPT_EXECUTION_INTERRUPTED]: ' +
+      'Script execution was interrupted by `SIGINT`',
+  ]);
+  assert.deepStrictEqual(output.slice(-2), [
     '}',
     PROMPT,
   ]);