From c0762c2f54f80b92f2f5e0f9af51f4c048c53e4f Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 3 Feb 2018 17:09:15 +0800 Subject: [PATCH] errors: move error creation helpers to errors.js This commit moves error creation helpers scattered around under lib/ into lib/internal/errors.js in the hope of being clearer about the differences of errors that we throw into the user land. - Move util._errnoException and util._exceptionWithHostPort into internal/errors.js and simplify their logic so it's clearer what the properties these helpers create. - Move the errnoException helper in dns.js to internal/errors.js into internal/errors.js and rename it to dnsException. Simplify it's logic so it no longer calls errnoException and skips the unnecessary argument checks. PR-URL: https://github.com/nodejs/node/pull/18546 Reviewed-By: James M Snell --- lib/dgram.js | 4 +- lib/dns.js | 47 +++--------- lib/fs.js | 2 +- lib/internal/child_process.js | 2 +- lib/internal/errors.js | 131 ++++++++++++++++++++++++++++++++-- lib/internal/http2/core.js | 4 +- lib/internal/process.js | 4 +- lib/net.js | 4 +- lib/tty.js | 4 +- lib/util.js | 38 +--------- 10 files changed, 148 insertions(+), 92 deletions(-) diff --git a/lib/dgram.js b/lib/dgram.js index 0c4990cac5d315..bed6129b47be11 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -45,8 +45,8 @@ const SEND_BUFFER = false; // Lazily loaded var cluster = null; -const errnoException = util._errnoException; -const exceptionWithHostPort = util._exceptionWithHostPort; +const errnoException = errors.errnoException; +const exceptionWithHostPort = errors.exceptionWithHostPort; function lookup4(lookup, address, callback) { diff --git a/lib/dns.js b/lib/dns.js index 979d724c3e4aa5..51c144f4e394eb 100644 --- a/lib/dns.js +++ b/lib/dns.js @@ -21,17 +21,10 @@ 'use strict'; -const util = require('util'); - const cares = process.binding('cares_wrap'); const { isIP, isIPv4, isLegalPort } = require('internal/net'); const { customPromisifyArgs } = require('internal/util'); const errors = require('internal/errors'); -const { - UV_EAI_MEMORY, - UV_EAI_NODATA, - UV_EAI_NONAME -} = process.binding('uv'); const { GetAddrInfoReqWrap, @@ -40,36 +33,12 @@ const { ChannelWrap, } = cares; -function errnoException(err, syscall, hostname) { - // FIXME(bnoordhuis) Remove this backwards compatibility nonsense and pass - // the true error to the user. ENOTFOUND is not even a proper POSIX error! - if (err === UV_EAI_MEMORY || - err === UV_EAI_NODATA || - err === UV_EAI_NONAME) { - err = 'ENOTFOUND'; - } - var ex = null; - if (typeof err === 'string') { // c-ares error code. - const errHost = hostname ? ` ${hostname}` : ''; - ex = new Error(`${syscall} ${err}${errHost}`); - ex.code = err; - ex.errno = err; - ex.syscall = syscall; - } else { - ex = util._errnoException(err, syscall); - } - if (hostname) { - ex.hostname = hostname; - } - return ex; -} - const IANA_DNS_PORT = 53; - +const dnsException = errors.dnsException; function onlookup(err, addresses) { if (err) { - return this.callback(errnoException(err, 'getaddrinfo', this.hostname)); + return this.callback(dnsException(err, 'getaddrinfo', this.hostname)); } if (this.family) { this.callback(null, addresses[0], this.family); @@ -81,7 +50,7 @@ function onlookup(err, addresses) { function onlookupall(err, addresses) { if (err) { - return this.callback(errnoException(err, 'getaddrinfo', this.hostname)); + return this.callback(dnsException(err, 'getaddrinfo', this.hostname)); } var family = this.family; @@ -161,7 +130,7 @@ function lookup(hostname, options, callback) { var err = cares.getaddrinfo(req, hostname, family, hints, verbatim); if (err) { - process.nextTick(callback, errnoException(err, 'getaddrinfo', hostname)); + process.nextTick(callback, dnsException(err, 'getaddrinfo', hostname)); return {}; } return req; @@ -173,7 +142,7 @@ Object.defineProperty(lookup, customPromisifyArgs, function onlookupservice(err, host, service) { if (err) - return this.callback(errnoException(err, 'getnameinfo', this.host)); + return this.callback(dnsException(err, 'getnameinfo', this.host)); this.callback(null, host, service); } @@ -202,7 +171,7 @@ function lookupService(host, port, callback) { req.oncomplete = onlookupservice; var err = cares.getnameinfo(req, host, port); - if (err) throw errnoException(err, 'getnameinfo', host); + if (err) throw dnsException(err, 'getnameinfo', host); return req; } @@ -215,7 +184,7 @@ function onresolve(err, result, ttls) { result = result.map((address, index) => ({ address, ttl: ttls[index] })); if (err) - this.callback(errnoException(err, this.bindingName, this.hostname)); + this.callback(dnsException(err, this.bindingName, this.hostname)); else this.callback(null, result); } @@ -253,7 +222,7 @@ function resolver(bindingName) { req.oncomplete = onresolve; req.ttl = !!(options && options.ttl); var err = this._handle[bindingName](req, name); - if (err) throw errnoException(err, bindingName); + if (err) throw dnsException(err, bindingName); return req; } Object.defineProperty(query, 'name', { value: bindingName }); diff --git a/lib/fs.js b/lib/fs.js index 7136911e60eb23..04d3d0a5bd4357 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -62,7 +62,7 @@ const { kMaxLength } = require('buffer'); const isWindows = process.platform === 'win32'; const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG); -const errnoException = util._errnoException; +const errnoException = errors.errnoException; let truncateWarn = true; diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index 2bade01f95f209..ac2b75f24575b8 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -29,7 +29,7 @@ const { UV_ESRCH } = process.binding('uv'); -const errnoException = util._errnoException; +const errnoException = errors.errnoException; const { SocketListSend, SocketListReceive } = SocketList; const MAX_HANDLE_RETRANSMISSIONS = 3; diff --git a/lib/internal/errors.js b/lib/internal/errors.js index a41df135407341..c07920ae5d01ed 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -18,7 +18,12 @@ var green = ''; var red = ''; var white = ''; -const { errmap } = process.binding('uv'); +const { + errmap, + UV_EAI_MEMORY, + UV_EAI_NODATA, + UV_EAI_NONAME +} = process.binding('uv'); const { kMaxLength } = process.binding('buffer'); const { defineProperty } = Object; @@ -33,6 +38,14 @@ function lazyUtil() { return util_; } +var internalUtil = null; +function lazyInternalUtil() { + if (!internalUtil) { + internalUtil = require('internal/util'); + } + return internalUtil; +} + function makeNodeError(Base) { return class NodeError extends Base { constructor(key, ...args) { @@ -356,10 +369,15 @@ function E(sym, val) { messages.set(sym, typeof val === 'function' ? val : String(val)); } -// This creates an error compatible with errors produced in UVException -// using the context collected in CollectUVExceptionInfo -// The goal is to migrate them to ERR_* errors later when -// compatibility is not a concern +/** + * This creates an error compatible with errors produced in the C++ + * function UVException using a context object with data assembled in C++. + * The goal is to migrate them to ERR_* errors later when compatibility is + * not a concern. + * + * @param {Object} ctx + * @returns {Error} + */ function uvException(ctx) { const err = new Error(); @@ -389,7 +407,110 @@ function uvException(ctx) { return err; } +/** + * This used to be util._errnoException(). + * + * @param {number} err - A libuv error number + * @param {string} syscall + * @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. + const code = lazyUtil().getSystemErrorName(err); + const message = original ? + `${syscall} ${code} ${original}` : `${syscall} ${code}`; + + const ex = new Error(message); + // TODO(joyeecheung): errno is supposed to err, like in uvException + ex.code = ex.errno = code; + ex.syscall = syscall; + + Error.captureStackTrace(ex, errnoException); + return ex; +} + +/** + * This used to be util._exceptionWithHostPort(). + * + * @param {number} err - A libuv error number + * @param {string} syscall + * @param {string} address + * @param {number} [port] + * @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. + const code = lazyUtil().getSystemErrorName(err); + let details = ''; + if (port && port > 0) { + details = ` ${address}:${port}`; + } else if (address) { + details = ` ${address}`; + } + if (additional) { + details += ` - Local (${additional})`; + } + + const ex = new Error(`${syscall} ${code}${details}`); + // TODO(joyeecheung): errno is supposed to err, like in uvException + ex.code = ex.errno = code; + ex.syscall = syscall; + ex.address = address; + if (port) { + ex.port = port; + } + + Error.captureStackTrace(ex, exceptionWithHostPort); + return ex; +} + +/** + * @param {number|string} err - A libuv error number or a c-ares error code + * @param {string} syscall + * @param {string} [hostname] + * @returns {Error} + */ +function dnsException(err, syscall, hostname) { + const ex = new Error(); + // FIXME(bnoordhuis) Remove this backwards compatibility nonsense and pass + // the true error to the user. ENOTFOUND is not even a proper POSIX error! + if (err === UV_EAI_MEMORY || + err === UV_EAI_NODATA || + err === UV_EAI_NONAME) { + err = 'ENOTFOUND'; // Fabricated error name. + } + if (typeof err === 'string') { // c-ares error code. + const errHost = hostname ? ` ${hostname}` : ''; + ex.message = `${syscall} ${err}${errHost}`; + // TODO(joyeecheung): errno is supposed to be a number, like in uvException + ex.code = ex.errno = err; + ex.syscall = syscall; + } else { // libuv error number + const code = lazyInternalUtil().getSystemErrorName(err); + ex.message = `${syscall} ${code}`; + // TODO(joyeecheung): errno is supposed to be err, like in uvException + ex.code = ex.errno = code; + ex.syscall = syscall; + } + if (hostname) { + ex.hostname = hostname; + } + Error.captureStackTrace(ex, dnsException); + return ex; +} + module.exports = exports = { + dnsException, + errnoException, + exceptionWithHostPort, uvException, message, Error: makeNodeError(Error), diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index f735e2fcc92f62..5f811ca8af403a 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1631,7 +1631,7 @@ class Http2Stream extends Duplex { req.async = false; const err = createWriteReq(req, handle, data, encoding); if (err) - throw util._errnoException(err, 'write', req.error); + throw errors.errnoException(err, 'write', req.error); trackWriteState(this, req.bytes); } @@ -1674,7 +1674,7 @@ class Http2Stream extends Duplex { } const err = handle.writev(req, chunks); if (err) - throw util._errnoException(err, 'write', req.error); + throw errors.errnoException(err, 'write', req.error); trackWriteState(this, req.bytes); } diff --git a/lib/internal/process.js b/lib/internal/process.js index 757c8de8e685f1..776129a140fe68 100644 --- a/lib/internal/process.js +++ b/lib/internal/process.js @@ -170,7 +170,7 @@ function setupKillAndExit() { } if (err) - throw util._errnoException(err, 'kill'); + throw errors.errnoException(err, 'kill'); return true; }; @@ -200,7 +200,7 @@ function setupSignalHandlers() { const err = wrap.start(signum); if (err) { wrap.close(); - throw util._errnoException(err, 'uv_signal_start'); + throw errors.errnoException(err, 'uv_signal_start'); } signalWraps[type] = wrap; diff --git a/lib/net.js b/lib/net.js index 0ba7457b7f8026..5d99b00662d49d 100644 --- a/lib/net.js +++ b/lib/net.js @@ -59,8 +59,8 @@ const kLastWriteQueueSize = Symbol('lastWriteQueueSize'); // reasons it's lazy loaded. var cluster = null; -const errnoException = util._errnoException; -const exceptionWithHostPort = util._exceptionWithHostPort; +const errnoException = errors.errnoException; +const exceptionWithHostPort = errors.exceptionWithHostPort; const { kTimeout, diff --git a/lib/tty.js b/lib/tty.js index 584ff9d87093c4..79e61e981b5a07 100644 --- a/lib/tty.js +++ b/lib/tty.js @@ -21,7 +21,7 @@ 'use strict'; -const { inherits, _errnoException, _extend } = require('util'); +const { inherits, _extend } = require('util'); const net = require('net'); const { TTY, isTTY } = process.binding('tty_wrap'); const errors = require('internal/errors'); @@ -178,7 +178,7 @@ WriteStream.prototype._refreshSize = function() { const winSize = new Array(2); const err = this._handle.getWindowSize(winSize); if (err) { - this.emit('error', _errnoException(err, 'getWindowSize')); + this.emit('error', errors.errnoException(err, 'getWindowSize')); return; } const [newCols, newRows] = winSize; diff --git a/lib/util.js b/lib/util.js index 14c99bd454c8b0..e13f5b8b80009e 100644 --- a/lib/util.js +++ b/lib/util.js @@ -1058,40 +1058,6 @@ function error(...args) { } } -function _errnoException(err, syscall, original) { - const name = getSystemErrorName(err); - var message = `${syscall} ${name}`; - if (original) - message += ` ${original}`; - const e = new Error(message); - e.code = e.errno = name; - e.syscall = syscall; - return e; -} - -function _exceptionWithHostPort(err, - syscall, - address, - port, - additional) { - var details; - if (port && port > 0) { - details = `${address}:${port}`; - } else { - details = address; - } - - if (additional) { - details += ` - Local (${additional})`; - } - var ex = exports._errnoException(err, syscall, details); - ex.address = address; - if (port) { - ex.port = port; - } - return ex; -} - function callbackifyOnRejected(reason, cb) { // `!reason` guard inspired by bluebird (Ref: https://goo.gl/t5IS6M). // Because `null` is a special error value in callbacks which means "no error @@ -1152,8 +1118,8 @@ function getSystemErrorName(err) { // Keep the `exports =` so that various functions can still be monkeypatched module.exports = exports = { - _errnoException, - _exceptionWithHostPort, + _errnoException: errors.errnoException, + _exceptionWithHostPort: errors.exceptionWithHostPort, _extend, callbackify, debuglog,