From d3671aeb8a3162113efb09151fd86951dd2e197a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 3 Feb 2018 17:30:48 +0800 Subject: [PATCH 1/3] util: skip type checks in internal getSystemErrorName --- lib/internal/util.js | 8 -------- lib/util.js | 13 ++++++++++++- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/internal/util.js b/lib/internal/util.js index 1e48eedfd59b48..3de8090cfa2618 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -221,14 +221,6 @@ function getConstructorOf(obj) { } function getSystemErrorName(err) { - if (typeof err !== 'number') { - throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'err', 'number', err); - } - if (err >= 0 || !Number.isSafeInteger(err)) { - throw new errors.RangeError('ERR_OUT_OF_RANGE', 'err', - 'a negative integer', err); - } - const entry = errmap.get(err); return entry ? entry[0] : `Unknown system error ${err}`; } diff --git a/lib/util.js b/lib/util.js index 4525792b2ec4c4..14c99bd454c8b0 100644 --- a/lib/util.js +++ b/lib/util.js @@ -55,7 +55,7 @@ const { const { customInspectSymbol, deprecate, - getSystemErrorName, + getSystemErrorName: internalErrorName, getIdentificationOf, isError, promisify, @@ -1139,6 +1139,17 @@ function callbackify(original) { return callbackified; } +function getSystemErrorName(err) { + if (typeof err !== 'number') { + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'err', 'number', err); + } + if (err >= 0 || !Number.isSafeInteger(err)) { + throw new errors.RangeError('ERR_OUT_OF_RANGE', 'err', + 'a negative integer', err); + } + return internalErrorName(err); +} + // Keep the `exports =` so that various functions can still be monkeypatched module.exports = exports = { _errnoException, From 7cb2e97db2b52329e9c3f706f5cc4166fedbace4 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 3 Feb 2018 17:35:04 +0800 Subject: [PATCH 2/3] fs: do not call new when creating uvException We cannot make uvException a proper class due to compatibility reasons for now, so there is no need to call new since it only returns a newly-created Error. --- lib/fs.js | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 0b9cf0cc9b8190..d74d7226e7db67 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -406,7 +406,7 @@ fs.accessSync = function(path, mode) { binding.access(pathModule.toNamespacedPath(path), mode, undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } }; @@ -637,7 +637,7 @@ function tryStatSync(fd, isUserFd) { binding.fstat(fd, undefined, ctx); if (ctx.errno !== undefined && !isUserFd) { fs.closeSync(fd); - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } } @@ -735,7 +735,7 @@ fs.closeSync = function(fd) { const ctx = {}; binding.close(fd, undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } }; @@ -928,7 +928,7 @@ fs.renameSync = function(oldPath, newPath) { binding.rename(pathModule.toNamespacedPath(oldPath), pathModule.toNamespacedPath(newPath), undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } }; @@ -998,7 +998,7 @@ fs.ftruncateSync = function(fd, len = 0) { const ctx = {}; binding.ftruncate(fd, len, undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } }; @@ -1032,7 +1032,7 @@ fs.fdatasyncSync = function(fd) { const ctx = {}; binding.fdatasync(fd, undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } }; @@ -1048,7 +1048,7 @@ fs.fsyncSync = function(fd) { const ctx = {}; binding.fsync(fd, undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } }; @@ -1133,7 +1133,7 @@ fs.fstatSync = function(fd) { const ctx = { fd }; binding.fstat(fd, undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } return statsFromValues(); }; @@ -1145,7 +1145,7 @@ fs.lstatSync = function(path) { const ctx = { path }; binding.lstat(pathModule.toNamespacedPath(path), undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } return statsFromValues(); }; @@ -1157,7 +1157,7 @@ fs.statSync = function(path) { const ctx = { path }; binding.stat(pathModule.toNamespacedPath(path), undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } return statsFromValues(); }; @@ -1183,7 +1183,7 @@ fs.readlinkSync = function(path, options) { const result = binding.readlink(pathModule.toNamespacedPath(path), options.encoding, undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } return result; }; @@ -1263,7 +1263,7 @@ fs.symlinkSync = function(target, path, type) { pathModule.toNamespacedPath(path), flags, undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } else if (ctx.error) { // TODO(joyeecheung): this is an encoding error usually caused by memory // problems. We need to figure out proper error code(s) for this. @@ -1308,7 +1308,7 @@ fs.linkSync = function(existingPath, newPath) { pathModule.toNamespacedPath(newPath), undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } return result; }; @@ -1331,7 +1331,7 @@ fs.unlinkSync = function(path) { const ctx = { path }; binding.unlink(pathModule.toNamespacedPath(path), undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } }; @@ -1937,7 +1937,7 @@ fs.realpathSync = function realpathSync(p, options) { const ctx = { path: base }; binding.lstat(pathModule.toNamespacedPath(base), undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } knownHard[base] = true; } @@ -1981,7 +1981,7 @@ fs.realpathSync = function realpathSync(p, options) { const ctx = { path: base }; binding.lstat(baseLong, undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } if ((statValues[1/*mode*/] & S_IFMT) !== S_IFLNK) { @@ -2006,11 +2006,11 @@ fs.realpathSync = function realpathSync(p, options) { const ctx = { path: base }; binding.stat(baseLong, undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } linkTarget = binding.readlink(baseLong, undefined, undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } } resolvedLink = pathModule.resolve(previous, linkTarget); @@ -2031,7 +2031,7 @@ fs.realpathSync = function realpathSync(p, options) { const ctx = { path: base }; binding.lstat(pathModule.toNamespacedPath(base), undefined, ctx); if (ctx.errno !== undefined) { - throw new errors.uvException(ctx); + throw errors.uvException(ctx); } knownHard[base] = true; } From 895a2d64314008d06c8210fe925f98917f6eae90 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 3 Feb 2018 17:09:15 +0800 Subject: [PATCH 3/3] 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. --- 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 d74d7226e7db67..03df5016da5cc5 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; 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 3530d63710cf77..39ad4c041bd82d 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 595030a13a49f5..4f395fcb8f0a60 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1628,7 +1628,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); } @@ -1671,7 +1671,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 cda613f2e23ba7..c2f3a189973ea9 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,