lib: move extra properties into error creation

This encapsulates the Node.js errors more by adding extra properties to an error inside of the function to create the error message instead of adding the properties at the call site. That simplifies the usage of our errors and makes sure the expected properties are always set. PR-URL: #26752 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Mar 27, 2019 · bff5d30 · bff5d30
1 parent 1458711
commit bff5d30
Show file tree

Hide file tree

Showing 9 changed files with 78 additions and 47 deletions.
diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js
@@ -309,15 +309,13 @@ function addChunk(stream, state, chunk, addToFront) {
 }
 
 function chunkInvalid(state, chunk) {
-  var er;
   if (!Stream._isUint8Array(chunk) &&
       typeof chunk !== 'string' &&
       chunk !== undefined &&
       !state.objectMode) {
-    er = new ERR_INVALID_ARG_TYPE(
+    return new ERR_INVALID_ARG_TYPE(
       'chunk', ['string', 'Buffer', 'Uint8Array'], chunk);
   }
-  return er;
 }
 
 

diff --git a/lib/internal/encoding.js b/lib/internal/encoding.js
@@ -394,9 +394,7 @@ function makeTextDecoderICU() {
 
       const ret = _decode(this[kHandle], input, flags);
       if (typeof ret === 'number') {
-        const err = new ERR_ENCODING_INVALID_ENCODED_DATA(this.encoding);
-        err.errno = ret;
-        throw err;
+        throw new ERR_ENCODING_INVALID_ENCODED_DATA(this.encoding, ret);
       }
       return ret.toString('ucs2');
     }

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -47,7 +47,8 @@ function lazyBuffer() {
 // and may have .path and .dest.
 class SystemError extends Error {
   constructor(key, context) {
-    const prefix = getMessage(key, []);
+    super();
+    const prefix = getMessage(key, [], this);
     let message = `${prefix}: ${context.syscall} returned ` +
                   `${context.code} (${context.message})`;
 
@@ -56,7 +57,12 @@ class SystemError extends Error {
     if (context.dest !== undefined)
       message += ` => ${context.dest}`;
 
-    super(message);
+    Object.defineProperty(this, 'message', {
+      value: message,
+      enumerable: false,
+      writable: true,
+      configurable: true
+    });
     Object.defineProperty(this, kInfo, {
       configurable: false,
       enumerable: false,
@@ -150,7 +156,14 @@ let useOriginalName = false;
 function makeNodeErrorWithCode(Base, key) {
   return class NodeError extends Base {
     constructor(...args) {
-      super(getMessage(key, args));
+      super();
+      const message = getMessage(key, args, this);
+      Object.defineProperty(this, 'message', {
+        value: message,
+        enumerable: false,
+        writable: true,
+        configurable: true
+      });
     }
 
     get name() {
@@ -204,7 +217,7 @@ function E(sym, val, def, ...otherClasses) {
   codes[sym] = def;
 }
 
-function getMessage(key, args) {
+function getMessage(key, args, self) {
   const msg = messages.get(key);
 
   if (util === undefined) util = require('util');
@@ -216,7 +229,7 @@ function getMessage(key, args) {
       `Code: ${key}; The provided arguments length (${args.length}) does not ` +
         `match the required ones (${msg.length}).`
     );
-    return msg.apply(null, args);
+    return msg.apply(self, args);
   }
 
   const expectedLength = (msg.match(/%[dfijoOs]/g) || []).length;
@@ -608,8 +621,10 @@ E('ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE',
   'The `domain` module is in use, which is mutually exclusive with calling ' +
      'process.setUncaughtExceptionCaptureCallback()',
   Error);
-E('ERR_ENCODING_INVALID_ENCODED_DATA',
-  'The encoded data was not valid for encoding %s', TypeError);
+E('ERR_ENCODING_INVALID_ENCODED_DATA', function(encoding, ret) {
+  this.errno = ret;
+  return `The encoded data was not valid for encoding ${encoding}`;
+}, 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);
@@ -652,7 +667,16 @@ E('ERR_HTTP2_INVALID_PSEUDOHEADER',
   '"%s" is an invalid pseudoheader or is used incorrectly', TypeError);
 E('ERR_HTTP2_INVALID_SESSION', 'The session has been destroyed', Error);
 E('ERR_HTTP2_INVALID_SETTING_VALUE',
-  'Invalid value for setting "%s": %s', TypeError, RangeError);
+  // Using default arguments here is important so the arguments are not counted
+  // towards `Function#length`.
+  function(name, actual, min = undefined, max = undefined) {
+    this.actual = actual;
+    if (min !== undefined) {
+      this.min = min;
+      this.max = max;
+    }
+    return `Invalid value for setting "${name}": ${actual}`;
+  }, TypeError, RangeError);
 E('ERR_HTTP2_INVALID_STREAM', 'The stream has been destroyed', Error);
 E('ERR_HTTP2_MAX_PENDING_SETTINGS_ACK',
   'Maximum number of pending settings acknowledgements', Error);
@@ -685,7 +709,15 @@ E('ERR_HTTP2_SOCKET_UNBOUND',
 E('ERR_HTTP2_STATUS_101',
   'HTTP status code 101 (Switching Protocols) is forbidden in HTTP/2', Error);
 E('ERR_HTTP2_STATUS_INVALID', 'Invalid status code: %s', RangeError);
-E('ERR_HTTP2_STREAM_CANCEL', 'The pending stream has been canceled', Error);
+E('ERR_HTTP2_STREAM_CANCEL', function(error) {
+  let msg = 'The pending stream has been canceled';
+  if (error) {
+    this.cause = error;
+    if (typeof error.message === 'string')
+      msg += ` (caused by: ${error.message})`;
+  }
+  return msg;
+}, Error);
 E('ERR_HTTP2_STREAM_ERROR', 'Stream closed with error code %s', Error);
 E('ERR_HTTP2_STREAM_SELF_DEPENDENCY',
   'A stream cannot depend on itself', Error);
@@ -709,7 +741,11 @@ E('ERR_INSPECTOR_CLOSED', 'Session was closed', Error);
 E('ERR_INSPECTOR_COMMAND', 'Inspector error %d: %s', Error);
 E('ERR_INSPECTOR_NOT_AVAILABLE', 'Inspector is not available', Error);
 E('ERR_INSPECTOR_NOT_CONNECTED', 'Session is not connected', Error);
-E('ERR_INVALID_ADDRESS_FAMILY', 'Invalid address family: %s', RangeError);
+E('ERR_INVALID_ADDRESS_FAMILY', function(addressType, host, port) {
+  this.host = host;
+  this.port = port;
+  return `Invalid address family: ${addressType} ${host}:${port}`;
+}, RangeError);
 E('ERR_INVALID_ARG_TYPE',
   (name, expected, actual) => {
     assert(typeof name === 'string', "'name' must be a string");
@@ -812,7 +848,10 @@ E('ERR_INVALID_SYNC_FORK_INPUT',
 E('ERR_INVALID_THIS', 'Value of "this" must be of type %s', TypeError);
 E('ERR_INVALID_TUPLE', '%s must be an iterable %s tuple', TypeError);
 E('ERR_INVALID_URI', 'URI malformed', URIError);
-E('ERR_INVALID_URL', 'Invalid URL: %s', TypeError);
+E('ERR_INVALID_URL', function(input) {
+  this.input = input;
+  return `Invalid URL: ${input}`;
+}, TypeError);
 E('ERR_INVALID_URL_SCHEME',
   (expected) => `The URL must be ${oneOf(expected, 'scheme')}`, TypeError);
 E('ERR_IPC_CHANNEL_CLOSED', 'Channel closed', Error);
@@ -926,8 +965,12 @@ E('ERR_STREAM_WRAP', 'Stream has StringDecoder set or is in objectMode', Error);
 E('ERR_STREAM_WRITE_AFTER_END', 'write after end', Error);
 E('ERR_SYNTHETIC', 'JavaScript Callstack', Error);
 E('ERR_SYSTEM_ERROR', 'A system error occurred', SystemError);
-E('ERR_TLS_CERT_ALTNAME_INVALID',
-  'Hostname/IP does not match certificate\'s altnames: %s', Error);
+E('ERR_TLS_CERT_ALTNAME_INVALID', function(reason, host, cert) {
+  this.reason = reason;
+  this.host = host;
+  this.cert = cert;
+  return `Hostname/IP does not match certificate's altnames: ${reason}`;
+}, Error);
 E('ERR_TLS_DH_PARAM_SIZE', 'DH parameter size %s is less than 2048', Error);
 E('ERR_TLS_HANDSHAKE_TIMEOUT', 'TLS handshake timeout', Error);
 E('ERR_TLS_INVALID_PROTOCOL_VERSION',

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -812,7 +812,6 @@ function validateSettings(settings) {
       typeof settings.enablePush !== 'boolean') {
     const err = new ERR_HTTP2_INVALID_SETTING_VALUE('enablePush',
                                                     settings.enablePush);
-    err.actual = settings.enablePush;
     Error.captureStackTrace(err, 'validateSettings');
     throw err;
   }
@@ -1219,12 +1218,7 @@ class Http2Session extends EventEmitter {
     this.removeAllListeners('timeout');
 
     // Destroy any pending and open streams
-    const cancel = new ERR_HTTP2_STREAM_CANCEL();
-    if (error) {
-      cancel.cause = error;
-      if (typeof error.message === 'string')
-        cancel.message += ` (caused by: ${error.message})`;
-    }
+    const cancel = new ERR_HTTP2_STREAM_CANCEL(error);
     state.pendingStreams.forEach((stream) => stream.destroy(cancel));
     state.streams.forEach((stream) => stream.destroy(error));
 

diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js
@@ -515,10 +515,8 @@ function assertIsObject(value, name, types) {
 function assertWithinRange(name, value, min = 0, max = Infinity) {
   if (value !== undefined &&
       (typeof value !== 'number' || value < min || value > max)) {
-    const err = new ERR_HTTP2_INVALID_SETTING_VALUE.RangeError(name, value);
-    err.min = min;
-    err.max = max;
-    err.actual = value;
+    const err = new ERR_HTTP2_INVALID_SETTING_VALUE.RangeError(
+      name, value, min, max);
     Error.captureStackTrace(err, assertWithinRange);
     throw err;
   }

diff --git a/lib/internal/url.js b/lib/internal/url.js
@@ -238,9 +238,7 @@ function onParseComplete(flags, protocol, username, password,
 }
 
 function onParseError(flags, input) {
-  const error = new ERR_INVALID_URL(input);
-  error.input = input;
-  throw error;
+  throw new ERR_INVALID_URL(input);
 }
 
 function onParseProtocolComplete(flags, protocol, username, password,

diff --git a/lib/net.js b/lib/net.js
@@ -989,19 +989,20 @@ function lookupAndConnect(self, options) {
       if (!self.connecting) return;
 
       if (err) {
-        // net.createConnection() creates a net.Socket object and
-        // immediately calls net.Socket.connect() on it (that's us).
-        // There are no event listeners registered yet so defer the
-        // error event to the next tick.
+        // net.createConnection() creates a net.Socket object and immediately
+        // calls net.Socket.connect() on it (that's us). There are no event
+        // listeners registered yet so defer the error event to the next tick.
+        // TODO(BridgeAR): The error could either originate from user code or
+        // by the C++ layer. The port is never the cause for the error as it is
+        // not used in the lookup. We should probably just remove this.
         err.host = options.host;
         err.port = options.port;
         err.message = err.message + ' ' + options.host + ':' + options.port;
         process.nextTick(connectErrorNT, self, err);
       } else if (addressType !== 4 && addressType !== 6) {
-        err = new ERR_INVALID_ADDRESS_FAMILY(addressType);
-        err.host = options.host;
-        err.port = options.port;
-        err.message = err.message + ' ' + options.host + ':' + options.port;
+        err = new ERR_INVALID_ADDRESS_FAMILY(addressType,
+                                             options.host,
+                                             options.port);
         process.nextTick(connectErrorNT, self, err);
       } else {
         self._unrefTimer();

diff --git a/lib/tls.js b/lib/tls.js
@@ -242,11 +242,7 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
   }
 
   if (!valid) {
-    const err = new ERR_TLS_CERT_ALTNAME_INVALID(reason);
-    err.reason = reason;
-    err.host = hostname;
-    err.cert = cert;
-    return err;
+    return new ERR_TLS_CERT_ALTNAME_INVALID(reason, hostname, cert);
   }
 };
 

diff --git a/test/parallel/test-net-options-lookup.js b/test/parallel/test-net-options-lookup.js
@@ -38,5 +38,10 @@ function connectDoesNotThrow(input) {
     cb(null, '127.0.0.1', 100);
   });
 
-  s.on('error', common.expectsError({ code: 'ERR_INVALID_ADDRESS_FAMILY' }));
+  s.on('error', common.expectsError({
+    code: 'ERR_INVALID_ADDRESS_FAMILY',
+    host: 'localhost',
+    port: 0,
+    message: 'Invalid address family: 100 localhost:0'
+  }));
 }