From 80528cceced500b5ae49ebf6d9df242ba2ce5ea4 Mon Sep 17 00:00:00 2001 From: Haoliang Gao Date: Fri, 24 Aug 2018 14:50:37 +0800 Subject: [PATCH] refactor(dnshttpclient): use async function instead of Promise (#2774) --- config/config.default.js | 3 +- lib/core/dnscache_httpclient.js | 101 ++++++++---------- package.json | 7 +- test/app/extend/request.test.js | 1 + .../config/config.default.js | 1 + test/lib/core/dnscache_httpclient.test.js | 32 +++++- 6 files changed, 85 insertions(+), 60 deletions(-) diff --git a/config/config.default.js b/config/config.default.js index 7fd54f00f5..b24f2d6119 100644 --- a/config/config.default.js +++ b/config/config.default.js @@ -233,6 +233,7 @@ module.exports = appInfo => { * The option for httpclient * @member Config#httpclient * @property {Boolean} enableDNSCache - Enable DNS lookup from local cache or not, default is false. + * @property {Boolean} dnsCacheLookupInterval - minimum interval of DNS query on the same hostname (default 10s). * * @property {Number} request.timeout - httpclient request default timeout, default is 5000 ms. * @@ -248,8 +249,8 @@ module.exports = appInfo => { */ config.httpclient = { enableDNSCache: false, + dnsCacheLookupInterval: 10000, dnsCacheMaxLength: 1000, - dnsCacheMaxAge: 10000, request: { timeout: 5000, diff --git a/lib/core/dnscache_httpclient.js b/lib/core/dnscache_httpclient.js index d7c821e4a5..bb02d7ed0c 100644 --- a/lib/core/dnscache_httpclient.js +++ b/lib/core/dnscache_httpclient.js @@ -1,12 +1,14 @@ 'use strict'; -const dns = require('dns'); +const dns = require('mz/dns'); const LRU = require('ylru'); const urlparse = require('url').parse; const HttpClient = require('./httpclient'); const utility = require('utility'); const IP_REGEX = /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/; +const DNSLOOKUP = Symbol('DNSCacheHttpClient#dnslookup'); +const UPDATE_DNS = Symbol('DNSCacheHttpClient#updateDNS'); class DNSCacheHttpClient extends HttpClient { constructor(app) { @@ -24,20 +26,22 @@ class DNSCacheHttpClient extends HttpClient { // the callback style if (callback) { - this._dnsLookup(url, args, (err, result) => { - if (err) return callback(err); - super.request(result.url, result.args, callback); - }); + this.app.deprecate('[dnscache_httpclient] Please don\'t use callback when `request()`'); + + this[DNSLOOKUP](url, args) + .then(result => { + return super.request(result.url, result.args); + }) + .then(result => process.nextTick(() => callback(null, result.data, result.res))) + .catch(err => process.nextTick(() => callback(err))); return; } // the Promise style - return new Promise((resolve, reject) => { - this._dnsLookup(url, args, (err, result) => { - if (err) return reject(err); - resolve(result); - }); - }).then(result => super.request(result.url, result.args)); + return (async () => { + const result = await this[DNSLOOKUP](url, args); + return super.request(result.url, result.args); + })(); } curl(url, args, callback) { @@ -61,14 +65,14 @@ class DNSCacheHttpClient extends HttpClient { }; } - _dnsLookup(url, args, callback) { + async [DNSLOOKUP](url, args) { const parsed = typeof url === 'string' ? urlparse(url) : url; // hostname must exists const hostname = parsed.hostname; // don't lookup when hostname is IP if (hostname && IP_REGEX.test(hostname)) { - return callback(null, { url, args }); + return { url, args }; } args = args || {}; @@ -82,56 +86,45 @@ class DNSCacheHttpClient extends HttpClient { const record = this.dnsCache.get(hostname); const now = Date.now(); if (record) { - const needUpdate = now - record.timestamp >= this.dnsCacheLookupInterval; - if (needUpdate) { + if (now - record.timestamp >= this.dnsCacheLookupInterval) { // make sure next request don't refresh dns query record.timestamp = now; - } - callback(null, { - url: this._formatDnsLookupUrl(hostname, url, record.ip), - args, - }); - if (!needUpdate) { - // no need to refresh dns record - return; - } - // make sure not callback twice - callback = null; - } - - dns.lookup(hostname, { family: 4 }, (err, address) => { - const logger = args.ctx ? args.ctx.coreLogger : this.app.coreLogger; - if (err) { - logger.warn('[dnscache_httpclient] dns lookup error: %s(%s) => %s', hostname, url, err); - // no cache, return error - return callback && callback(err); + this.app.runInBackground(async () => { + await this[UPDATE_DNS](hostname, args); + }); } - logger.info('[dnscache_httpclient] dns lookup success: %s(%s) => %s', hostname, url, address); - this.dnsCache.set(hostname, { - timestamp: Date.now(), - ip: address, - }); + return { url: formatDnsLookupUrl(hostname, url, record.ip), args }; + } - callback && callback(null, { - url: this._formatDnsLookupUrl(hostname, url, address), - args, - }); - }); + const address = await this[UPDATE_DNS](hostname, args); + return { url: formatDnsLookupUrl(hostname, url, address), args }; } - _formatDnsLookupUrl(host, url, address) { - if (typeof url === 'string') { - url = url.replace(host, address); - } else { - url = utility.assign({}, url); - url.hostname = url.hostname.replace(host, address); - if (url.host) { - url.host = url.host.replace(host, address); - } + async [UPDATE_DNS](hostname, args) { + const logger = args.ctx ? args.ctx.coreLogger : this.app.coreLogger; + try { + const [ address ] = await dns.lookup(hostname, { family: 4 }); + logger.info('[dnscache_httpclient] dns lookup success: %s => %s', + hostname, address); + this.dnsCache.set(hostname, { timestamp: Date.now(), ip: address }); + return address; + } catch (err) { + err.message = `[dnscache_httpclient] dns lookup error: ${hostname} => ${err.message}`; + throw err; } - return url; } + } module.exports = DNSCacheHttpClient; + +function formatDnsLookupUrl(host, url, address) { + if (typeof url === 'string') return url.replace(host, address); + const urlObj = utility.assign({}, url); + urlObj.hostname = urlObj.hostname.replace(host, address); + if (urlObj.host) { + urlObj.host = urlObj.host.replace(host, address); + } + return urlObj; +} diff --git a/package.json b/package.json index df34084124..264b0befd2 100644 --- a/package.json +++ b/package.json @@ -26,7 +26,7 @@ "delegates": "^1.0.0", "egg-cluster": "^1.18.0", "egg-cookies": "^2.2.2", - "egg-core": "^4.8.0", + "egg-core": "^4.9.1", "egg-development": "^2.3.1", "egg-i18n": "^2.0.0", "egg-jsonp": "^2.0.0", @@ -48,6 +48,7 @@ "koa-is-json": "^1.0.0", "koa-override": "^3.0.0", "ms": "^2.1.1", + "mz": "^2.7.0", "on-finished": "^2.3.0", "sendmessage": "^1.1.0", "urllib": "^2.29.0", @@ -61,8 +62,8 @@ "coffee": "^4.1.0", "egg-alinode": "^1.0.3", "egg-bin": "^4.7.1", - "egg-doctools": "^2.3.1", - "egg-mock": "^3.17.2", + "egg-doctools": "^2.4.0", + "egg-mock": "^3.17.3", "egg-plugin-puml": "^2.4.0", "egg-tracer": "^1.1.0", "egg-view-nunjucks": "^2.2.0", diff --git a/test/app/extend/request.test.js b/test/app/extend/request.test.js index b528677637..884edfe18f 100644 --- a/test/app/extend/request.test.js +++ b/test/app/extend/request.test.js @@ -173,6 +173,7 @@ describe('test/app/extend/request.test.js', () => { } it('should get string value', () => { + expectQuery('=b', {}); expectQuery('a=b', { a: 'b' }); expectQuery('a=&', { a: '' }); expectQuery('a=b&', { a: 'b' }); diff --git a/test/fixtures/apps/dnscache_httpclient/config/config.default.js b/test/fixtures/apps/dnscache_httpclient/config/config.default.js index 18a819f3d3..1fd423ec38 100644 --- a/test/fixtures/apps/dnscache_httpclient/config/config.default.js +++ b/test/fixtures/apps/dnscache_httpclient/config/config.default.js @@ -2,6 +2,7 @@ exports.httpclient = { enableDNSCache: true, + dnsCacheLookupInterval: 5000, }; exports.keys = 'test key'; diff --git a/test/lib/core/dnscache_httpclient.test.js b/test/lib/core/dnscache_httpclient.test.js index 8f3a93aaae..17e1b2d9f6 100644 --- a/test/lib/core/dnscache_httpclient.test.js +++ b/test/lib/core/dnscache_httpclient.test.js @@ -2,8 +2,9 @@ const mm = require('egg-mock'); const assert = require('assert'); -const dns = require('dns'); +const dns = require('mz/dns'); const urlparse = require('url').parse; +const sleep = require('mz-modules/sleep'); const utils = require('../../utils'); describe('test/lib/core/dnscache_httpclient.test.js', () => { @@ -130,7 +131,7 @@ describe('test/lib/core/dnscache_httpclient.test.js', () => { }); it('should dnsCacheMaxLength work', async () => { - mm.data(dns, 'lookup', '127.0.0.1'); + mm(dns, 'lookup', async () => [ '127.0.0.1' ]); // reset lru cache mm(app.httpclient.dnsCache, 'max', 1); @@ -154,6 +155,33 @@ describe('test/lib/core/dnscache_httpclient.test.js', () => { assert(app.httpclient.dnsCache.get('another.com')); }); + it('should cache and update', async () => { + mm(dns, 'lookup', async () => [ '127.0.0.1' ]); + + let obj = urlparse(url + '/get_headers'); + let result = await app.curl(obj, { dataType: 'json' }); + assert(result.status === 200); + assert(result.data.host === host); + let record = app.httpclient.dnsCache.get('localhost'); + const timestamp = record.timestamp; + assert(record); + + obj = urlparse(url + '/get_headers'); + result = await app.curl(obj, { dataType: 'json' }); + assert(result.status === 200); + assert(result.data.host === host); + record = app.httpclient.dnsCache.get('localhost'); + assert(timestamp === record.timestamp); + + await sleep(5000); + obj = urlparse(url + '/get_headers'); + result = await app.curl(obj, { dataType: 'json' }); + assert(result.status === 200); + assert(result.data.host === host); + record = app.httpclient.dnsCache.get('localhost'); + assert(timestamp !== record.timestamp); + }); + it('should not cache ip', async () => { const obj = urlparse(url.replace('localhost', '127.0.0.1') + '/get_headers'); const result = await app.curl(obj, { dataType: 'json' });