From d8b637fa3753eff4698059d43aed99cd4c3563f5 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 | 43 +++---------- lib/fs.js | 2 +- lib/internal/child_process.js | 2 +- lib/internal/errors.js | 116 ++++++++++++++++++++++++++++++++++ lib/internal/http2/core.js | 4 +- lib/internal/process.js | 5 +- lib/net.js | 4 +- lib/tty.js | 8 +-- lib/util.js | 39 +----------- 10 files changed, 141 insertions(+), 86 deletions(-) diff --git a/lib/dgram.js b/lib/dgram.js index af1fd986fa5cdd..55d498d67c372f 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 4c7fbe147497f4..9d853cd6d8d758 100644 --- a/lib/dns.js +++ b/lib/dns.js @@ -21,12 +21,10 @@ 'use strict'; -const util = require('util'); - const cares = process.binding('cares_wrap'); -const uv = process.binding('uv'); const { isLegalPort } = require('internal/net'); const { customPromisifyArgs } = require('internal/util'); +const errors = require('internal/errors'); const { GetAddrInfoReqWrap, @@ -36,30 +34,6 @@ const { isIP } = 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.UV_EAI_MEMORY || - err === uv.UV_EAI_NODATA || - err === uv.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 digits = [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 0-15 @@ -86,10 +60,11 @@ function isIPv4(str) { return (str.length > 3 && str.charCodeAt(3) === 46/*'.'*/); } +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); @@ -101,7 +76,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; @@ -181,7 +156,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; @@ -193,7 +168,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); } @@ -222,7 +197,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; } @@ -235,7 +210,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); } @@ -272,7 +247,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 b5565201e2abdd..b8ae3aa31ea911 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -59,7 +59,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; function getOptions(options, defaultOptions) { if (options === null || options === undefined || diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index 726a83c0734473..a0409f72c29de0 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -18,7 +18,7 @@ const SocketList = require('internal/socket_list'); const { convertToValidSignal } = require('internal/util'); const { isUint8Array } = require('internal/util/types'); -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 1027ef53ebdde6..93418006a7ac95 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -11,6 +11,11 @@ const kCode = Symbol('code'); const messages = new Map(); +const { + UV_EAI_MEMORY, + UV_EAI_NODATA, + UV_EAI_NONAME +} = process.binding('uv'); const { defineProperty } = Object; // Lazily loaded @@ -22,6 +27,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) { @@ -125,7 +138,110 @@ function E(sym, val) { messages.set(sym, typeof val === 'function' ? val : String(val)); } +/** + * 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, message, Error: makeNodeError(Error), TypeError: makeNodeError(TypeError), diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 68bfbb043b0b63..2e420d14cecd83 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1647,7 +1647,7 @@ class Http2Stream extends Duplex { req.async = false; const err = createWriteReq(req, handle, data, encoding); if (err) - return this.destroy(util._errnoException(err, 'write', req.error), cb); + return this.destroy(errors.errnoException(err, 'write', req.error), cb); trackWriteState(this, req.bytes); } @@ -1690,7 +1690,7 @@ class Http2Stream extends Duplex { } const err = handle.writev(req, chunks); if (err) - return this.destroy(util._errnoException(err, 'write', req.error), cb); + return this.destroy(errors.errnoException(err, 'write', req.error), cb); trackWriteState(this, req.bytes); } diff --git a/lib/internal/process.js b/lib/internal/process.js index 2c7c87c401fe11..1613ae6bfddd66 100644 --- a/lib/internal/process.js +++ b/lib/internal/process.js @@ -1,5 +1,6 @@ 'use strict'; +const errors = require('internal/errors'); const util = require('util'); const constants = process.binding('constants').os.signals; @@ -180,7 +181,7 @@ function setupKillAndExit() { } if (err) - throw util._errnoException(err, 'kill'); + throw errors.errnoException(err, 'kill'); return true; }; @@ -211,7 +212,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 53c91a640f5c1d..dae596eec0780c 100644 --- a/lib/net.js +++ b/lib/net.js @@ -48,8 +48,8 @@ const dns = require('dns'); // reasons it's lazy loaded. var cluster = null; -const errnoException = util._errnoException; -const exceptionWithHostPort = util._exceptionWithHostPort; +const errnoException = errors.errnoException; +const exceptionWithHostPort = errors.exceptionWithHostPort; function noop() {} diff --git a/lib/tty.js b/lib/tty.js index 9595c79db39a33..29440d6d96e3d5 100644 --- a/lib/tty.js +++ b/lib/tty.js @@ -21,11 +21,9 @@ 'use strict'; -const util = require('util'); +const { inherits, _extend } = require('util'); const net = require('net'); const { TTY, isTTY } = process.binding('tty_wrap'); -const { inherits } = util; -const errnoException = util._errnoException; const errors = require('internal/errors'); const readline = require('readline'); @@ -40,7 +38,7 @@ function ReadStream(fd, options) { if (fd >> 0 !== fd || fd < 0) throw new errors.RangeError('ERR_INVALID_FD', fd); - options = util._extend({ + options = _extend({ highWaterMark: 0, readable: true, writable: false, @@ -99,7 +97,7 @@ WriteStream.prototype._refreshSize = function() { var winSize = new Array(2); var err = this._handle.getWindowSize(winSize); if (err) { - this.emit('error', errnoException(err, 'getWindowSize')); + this.emit('error', errors.errnoException(err, 'getWindowSize')); return; } var newCols = winSize[0]; diff --git a/lib/util.js b/lib/util.js index 1379eb3f31a9d2..14c236cbb2c064 100644 --- a/lib/util.js +++ b/lib/util.js @@ -983,41 +983,6 @@ function error(...args) { } } -function _errnoException(err, syscall, original) { - const name = getSystemErrorName(err); - var message = `${syscall} ${name}`; - if (original) - message += ` ${original}`; - var e = new Error(message); - e.code = name; - 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 @@ -1075,8 +1040,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,