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

Author: popomore

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