From 1698c8e1655adaf19e13cc6a5e1a4a570735e16f Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 21 Jun 2017 20:16:43 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/13857 Reviewed-By: Refael Ackermann Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Michael Dawson --- lib/_http_server.js | 6 +- lib/_stream_transform.js | 4 +- lib/internal/errors.js | 74 +++++++++---------- lib/internal/process.js | 2 +- lib/internal/url.js | 3 +- .../test-child-process-fork-regr-gh-2847.js | 7 +- .../test-child-process-send-after-close.js | 8 +- test/parallel/test-process-next-tick.js | 2 +- .../test-stream-transform-callback-twice.js | 9 +-- 9 files changed, 53 insertions(+), 62 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index c3a10112ff2887..f77b8f6a7e7867 100644 --- 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 index 0f607a6da38593..34a83f76322807 100644 --- 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 index f1835516a70e95..109892f831247d 100644 --- 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 index 1636e73b7e009d..062b16e09039a6 100644 --- 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 index 7d7c79149a5c4f..64ebf383e6eb31 100644 --- 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 index 08f7977459bae4..9e4412d1f73738 100644 --- 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 index 78cf3b6697ea1f..5039767aeb7660 100644 --- 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 index 9cbdfc6d3b6a2d..5c078870806cc5 100644 --- 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 index afb55faa1a27c1..83c799b92fba25 100644 --- 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');