refactor(dnshttpclient): use async function instead of Promise (#2774)

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,

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;
+}

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",

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' });

test/fixtures/apps/dnscache_httpclient/config/config.default.js

@@ -2,6 +2,7 @@
 
 exports.httpclient = {
  enableDNSCache: true,
+ dnsCacheLookupInterval: 5000,
 };
 
 exports.keys = 'test key';

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' });