errors: fix and improve error types

1) Add missing lazy assert call 2) Remove obsolete error type 3) Name undocumented error type more appropriate 4) Consolidate error type style (rely on util.format instead of using a function) 5) Uppercase the first letter from error messages 6) Improve some internal error parameters PR-URL: #13857 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
nodejs · Jun 28, 2017 · 1698c8e · 1698c8e
1 parent 8ca9338
commit 1698c8e
Show file tree

Hide file tree

Showing 9 changed files with 53 additions and 62 deletions.
diff --git a/lib/_http_server.js b/lib/_http_server.js
@@ -225,10 +225,8 @@ function writeHead(statusCode, reason, obj) {
     headers = obj;
   }
 
-  if (common._checkInvalidHeaderChar(this.statusMessage)) {
-    throw new errors.Error('ERR_HTTP_INVALID_CHAR',
-                           'Invalid character in statusMessage.');
-  }
+  if (common._checkInvalidHeaderChar(this.statusMessage))
+    throw new errors.Error('ERR_INVALID_CHAR', 'statusMessage');
 
   var statusLine = 'HTTP/1.1 ' + statusCode + ' ' + this.statusMessage + CRLF;
 

diff --git a/lib/_stream_transform.js b/lib/_stream_transform.js
@@ -77,8 +77,7 @@ function afterTransform(er, data) {
   var cb = ts.writecb;
 
   if (!cb) {
-    return this.emit('error',
-                     new errors.Error('ERR_TRANSFORM_MULTIPLE_CALLBACK'));
+    return this.emit('error', new errors.Error('ERR_MULTIPLE_CALLBACK'));
   }
 
   ts.writechunk = null;
@@ -207,6 +206,7 @@ function done(stream, er, data) {
   if (data != null) // single equals check for both `null` and `undefined`
     stream.push(data);
 
+  // TODO(BridgeAR): Write a test for these two error cases
   // if there's nothing in the write buffer, then that means
   // that nothing more will ever be provided
   if (stream._writableState.length)

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -110,32 +110,32 @@ module.exports = exports = {
 //
 // Note: Please try to keep these in alphabetical order
 E('ERR_ARG_NOT_ITERABLE', '%s must be iterable');
-E('ERR_ASSERTION', (msg) => msg);
+E('ERR_ASSERTION', '%s');
 E('ERR_CONSOLE_WRITABLE_STREAM',
-  (name) => `Console expects a writable stream instance for ${name}`);
-E('ERR_CPU_USAGE', (errMsg) => `Unable to obtain cpu usage ${errMsg}`);
+  'Console expects a writable stream instance for %s');
+E('ERR_CPU_USAGE', 'Unable to obtain cpu usage %s');
 E('ERR_FALSY_VALUE_REJECTION', 'Promise was rejected with falsy value');
 E('ERR_HTTP_HEADERS_SENT',
   'Cannot render headers after they are sent to the client');
-E('ERR_HTTP_INVALID_CHAR', 'Invalid character in statusMessage.');
-E('ERR_HTTP_INVALID_STATUS_CODE',
-  (originalStatusCode) => `Invalid status code: ${originalStatusCode}`);
+E('ERR_HTTP_INVALID_STATUS_CODE', 'Invalid status code: %s');
+E('ERR_HTTP_TRAILER_INVALID',
+  'Trailers are invalid with this transfer encoding');
 E('ERR_INDEX_OUT_OF_RANGE', 'Index out of range');
+E('ERR_INVALID_ARG_TYPE', invalidArgType);
 E('ERR_INVALID_ARRAY_LENGTH',
   (name, length, actual) => {
-    let msg = `The "${name}" array must have a length of ${length}`;
-    if (arguments.length > 2) {
-      const len = Array.isArray(actual) ? actual.length : actual;
-      msg += `. Received length ${len}`;
-    }
-    return msg;
+    const assert = lazyAssert();
+    assert.strictEqual(typeof actual, 'number');
+    return `The "${name}" array must have a length of ${
+           length}. Received length ${actual}`;
   });
-E('ERR_INVALID_ARG_TYPE', invalidArgType);
-E('ERR_INVALID_CALLBACK', 'callback must be a function');
-E('ERR_INVALID_FD', (fd) => `"fd" must be a positive integer: ${fd}`);
+E('ERR_INVALID_CALLBACK', 'Callback must be a function');
+E('ERR_INVALID_CHAR', 'Invalid character in %s');
 E('ERR_INVALID_CURSOR_POS',
   'Cannot set cursor row without setting its column');
-E('ERR_INVALID_FILE_URL_HOST', 'File URL host %s');
+E('ERR_INVALID_FD', '"fd" must be a positive integer: %s');
+E('ERR_INVALID_FILE_URL_HOST',
+  'File URL host must be "localhost" or empty on %s');
 E('ERR_INVALID_FILE_URL_PATH', 'File URL path %s');
 E('ERR_INVALID_HANDLE_TYPE', 'This handle type cannot be sent');
 E('ERR_INVALID_OPT_VALUE',
@@ -144,47 +144,42 @@ E('ERR_INVALID_OPT_VALUE',
   });
 E('ERR_INVALID_REPL_EVAL_CONFIG',
   'Cannot specify both "breakEvalOnSigint" and "eval" for REPL');
+E('ERR_INVALID_REPL_HISTORY', 'Expected array, got %s');
 E('ERR_INVALID_SYNC_FORK_INPUT',
-  (value) => {
-    return 'Asynchronous forks do not support Buffer, Uint8Array or string' +
-           `input: ${value}`;
-  });
+  'Asynchronous forks do not support Buffer, Uint8Array or string input: %s');
 E('ERR_INVALID_THIS', 'Value of "this" must be of type %s');
 E('ERR_INVALID_TUPLE', '%s must be an iterable %s tuple');
 E('ERR_INVALID_URL', 'Invalid URL: %s');
 E('ERR_INVALID_URL_SCHEME',
-  (expected) => `The URL must be ${oneOf(expected, 'scheme')}`);
-E('ERR_INVALID_REPL_HISTORY',
-  (repl_history) => `Expected array, got ${repl_history}`);
-E('ERR_IPC_CHANNEL_CLOSED', 'channel closed');
+  (expected) => {
+    lazyAssert();
+    return `The URL must be ${oneOf(expected, 'scheme')}`;
+  });
+E('ERR_IPC_CHANNEL_CLOSED', 'Channel closed');
 E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected');
 E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe');
 E('ERR_IPC_SYNC_FORK', 'IPC cannot be used with synchronous forks');
 E('ERR_MISSING_ARGS', missingArgs);
-E('ERR_PARSE_HISTORY_DATA',
-  (oldHistoryPath) => `Could not parse history data in ${oldHistoryPath}`);
+E('ERR_MULTIPLE_CALLBACK', 'Callback called multiple times');
 E('ERR_NO_CRYPTO', 'Node.js is not compiled with OpenSSL crypto support');
+E('ERR_PARSE_HISTORY_DATA', 'Could not parse history data in %s');
+E('ERR_SOCKET_ALREADY_BOUND', 'Socket is already bound');
+E('ERR_SOCKET_BAD_TYPE',
+  'Bad socket type specified. Valid types are: udp4, udp6');
+E('ERR_SOCKET_CANNOT_SEND', 'Unable to send data');
+E('ERR_SOCKET_BAD_PORT', 'Port should be > 0 and < 65536');
+E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running');
 E('ERR_STDERR_CLOSE', 'process.stderr cannot be closed');
 E('ERR_STDOUT_CLOSE', 'process.stdout cannot be closed');
 E('ERR_STREAM_HAS_STRINGDECODER', 'Stream has StringDecoder');
 E('ERR_TRANSFORM_ALREADY_TRANSFORMING',
   'Calling transform done when still transforming');
-E('ERR_TRANSFORM_MULTIPLE_CALLBACK', 'Callback called multiple times');
 E('ERR_TRANSFORM_WITH_LENGTH_0',
-  'Calling transform done when ws.length != 0');
-E('ERR_HTTP_TRAILER_INVALID',
-  'Trailers are invalid with this transfer encoding');
-E('ERR_UNKNOWN_BUILTIN_MODULE', (id) => `No such built-in module: ${id}`);
-E('ERR_UNKNOWN_SIGNAL', (signal) => `Unknown signal: ${signal}`);
+  'Calling transform done when writableState.length != 0');
+E('ERR_UNKNOWN_SIGNAL', 'Unknown signal: %s');
 E('ERR_UNKNOWN_STDIN_TYPE', 'Unknown stdin file type');
 E('ERR_UNKNOWN_STREAM_TYPE', 'Unknown stream file type');
-E('ERR_SOCKET_ALREADY_BOUND', 'Socket is already bound');
-E('ERR_SOCKET_BAD_TYPE',
-  'Bad socket type specified. Valid types are: udp4, udp6');
-E('ERR_SOCKET_CANNOT_SEND', 'Unable to send data');
-E('ERR_SOCKET_BAD_PORT', 'Port should be > 0 and < 65536');
-E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running');
-E('ERR_V8BREAKITERATOR', 'full ICU data not installed. ' +
+E('ERR_V8BREAKITERATOR', 'Full ICU data not installed. ' +
   'See https://github.com/nodejs/node/wiki/Intl');
 // Add new errors from here...
 
@@ -200,6 +195,7 @@ function invalidArgType(name, expected, actual) {
 }
 
 function missingArgs(...args) {
+  const assert = lazyAssert();
   assert(args.length > 0, 'At least one arg needs to be specified');
   let msg = 'The ';
   const len = args.length;

diff --git a/lib/internal/process.js b/lib/internal/process.js
@@ -86,7 +86,7 @@ function setup_hrtime() {
       }
       if (time.length !== 2) {
         throw new errors.TypeError('ERR_INVALID_ARRAY_LENGTH', 'time', 2,
-                                   time);
+                                   time.length);
       }
 
       const sec = (hrValues[0] * 0x100000000 + hrValues[1]) - time[0];

diff --git a/lib/internal/url.js b/lib/internal/url.js
@@ -1352,8 +1352,7 @@ function getPathFromURLWin32(url) {
 
 function getPathFromURLPosix(url) {
   if (url.hostname !== '') {
-    return new errors.TypeError('ERR_INVALID_FILE_URL_HOST',
-                                `must be "localhost" or empty on ${platform}`);
+    return new errors.TypeError('ERR_INVALID_FILE_URL_HOST', platform);
   }
   var pathname = url.pathname;
   for (var n = 0; n < pathname.length; n++) {

diff --git a/test/parallel/test-child-process-fork-regr-gh-2847.js b/test/parallel/test-child-process-fork-regr-gh-2847.js
@@ -60,11 +60,8 @@ const server = net.createServer(function(s) {
       send(function(err) {
         // Ignore errors when sending the second handle because the worker
         // may already have exited.
-        if (err) {
-          if ((err.message !== 'channel closed') &&
-             (err.code !== 'ECONNREFUSED')) {
-            throw err;
-          }
+        if (err && err.message !== 'Channel closed') {
+          throw err;
         }
       });
     });

diff --git a/test/parallel/test-child-process-send-after-close.js b/test/parallel/test-child-process-send-after-close.js
@@ -10,9 +10,11 @@ child.on('close', common.mustCall((code, signal) => {
   assert.strictEqual(code, 0);
   assert.strictEqual(signal, null);
 
-  function testError(err) {
-    assert.strictEqual(err.message, 'channel closed');
-  }
+  const testError = common.expectsError({
+    type: Error,
+    message: 'Channel closed',
+    code: 'ERR_IPC_CHANNEL_CLOSED'
+  });
 
   child.on('error', common.mustCall(testError));
 

diff --git a/test/parallel/test-process-next-tick.js b/test/parallel/test-process-next-tick.js
@@ -43,6 +43,6 @@ process.on('exit', function() {
                 common.expectsError({
                   code: 'ERR_INVALID_CALLBACK',
                   type: TypeError,
-                  message: 'callback must be a function'
+                  message: 'Callback must be a function'
                 }));
 });
diff --git a/test/parallel/test-stream-transform-callback-twice.js b/test/parallel/test-stream-transform-callback-twice.js
@@ -1,15 +1,14 @@
 'use strict';
 const common = require('../common');
-const assert = require('assert');
 const { Transform } = require('stream');
 const stream = new Transform({
   transform(chunk, enc, cb) { cb(); cb(); }
 });
 
-stream.on('error', common.mustCall((err) => {
-  assert.strictEqual(err.toString(),
-                     'Error [ERR_TRANSFORM_MULTIPLE_CALLBACK]: ' +
-                     'Callback called multiple times');
+stream.on('error', common.expectsError({
+  type: Error,
+  message: 'Callback called multiple times',
+  code: 'ERR_MULTIPLE_CALLBACK'
 }));
 
 stream.write('foo');