dns: refactor dns.resolve

doc/api/dns.markdown

@@ -168,6 +168,23 @@ corresponding lookup methods.
 On error, `err` is an [`Error`][] object, where `err.code` is
 one of the error codes listed [here](#dns_error_codes).
 
+Returns an `EventEmitter` object that will emit a single `'complete'` event
+on the next next tick of the Node.js event loop (using `setImmediate()`) after
+the resolve operation completes, or an `'error'` event if an error occurs
+while *creating* the resolve request.
+
+For instance, the `dns.setServers()` method will throw an error if called
+while an outstanding `dns.resolve()` operation is still being processed. The
+`'complete'` event, then, can be used to determine when the resolve is
+completely finished so that the `dns.setServers()` call can be invoked:
+
+```js
+dns.resolve('example.com', 'A', (err, addr) => { /** ... **/ })
+   .on('complete', () => {
+     dns.setServers(newServerArray);
+   });
+```
+
 ## dns.resolve4(hostname, callback)
 
 Uses the DNS protocol to resolve a IPv4 addresses (`A` records) for the
@@ -283,8 +300,8 @@ If a port specified on the address it will be removed.
 
 An error will be thrown if an invalid address is provided.
 
-The `dns.setServers()` method must not be called while a DNS query is in
-progress.
+An error will be thrown if the `dns.setServers()` method is called while a
+DNS query is in progress.
 
 ## Error codes
 

lib/dns.js

@@ -5,6 +5,7 @@ const util = require('util');
 const cares = process.binding('cares_wrap');
 const uv = process.binding('uv');
 const internalNet = require('internal/net');
+const EventEmitter = require('events');
 
 const GetAddrInfoReqWrap = cares.GetAddrInfoReqWrap;
 const GetNameInfoReqWrap = cares.GetNameInfoReqWrap;
@@ -13,6 +14,7 @@ const QueryReqWrap = cares.QueryReqWrap;
 const isIP = cares.isIP;
 const isLegalPort = internalNet.isLegalPort;
 
+var _resolvesOutstanding = 0;
 
 function errnoException(err, syscall, hostname) {
   // FIXME(bnoordhuis) Remove this backwards compatibility nonsense and pass
@@ -212,33 +214,61 @@ exports.lookupService = function(host, port, callback) {
 
 
 function onresolve(err, result) {
-  if (err)
-    this.callback(errnoException(err, this.bindingName, this.hostname));
-  else
-    this.callback(null, result);
+  const emitter = this.emitter;
+  process.nextTick(() => {
+    if (err) {
+      emitter.emit('error',
+                   errnoException(err, this.bindingName, this.hostname));
+    } else {
+      emitter.emit('resolve', result);
+    }
+  });
+  // This has to be done on setImmediate in order to give c-ares
+  // time to cleanup after the resolve completes.
+  setImmediate(() => {
+    _resolvesOutstanding--;
+    emitter.emit('complete');
+  });
 }
 
+class Resolver extends EventEmitter {
+  constructor(binding, bindingName, name, callback) {
+    super();
+    _resolvesOutstanding++;
+    if (typeof name !== 'string') {
+      throw new Error('"name" argument must be a string');
+    }
+    // Do this on nextTick in order to give users time to
+    // attach the listeners.
+    process.nextTick(() => {
+      var req = new QueryReqWrap();
+      req.bindingName = bindingName;
+      req.hostname = name;
+      req.oncomplete = onresolve;
+      req.emitter = this;
+      // For backwards compatibility reasons, if the callback is
+      // passed, set it as both the resolve and error listener,
+      // so that errors are handled by the callback.
+      if (typeof callback === 'function') {
+        req.emitter.on('resolve', (res) => {
+          callback(null, res);
+        });
+        req.emitter.on('error', callback);
+      }
+      var err = binding(req, name);
+      if (err) {
+        req.emitter.emit('error', errnoException(err, bindingName));
+        _resolvesOutstanding--;
+      }
+    });
+  }
+}
 
 function resolver(bindingName) {
   var binding = cares[bindingName];
 
   return function query(name, callback) {
-    if (typeof name !== 'string') {
-      throw new Error('"name" argument must be a string');
-    } else if (typeof callback !== 'function') {
-      throw new Error('"callback" argument must be a function');
-    }
-
-    callback = makeAsync(callback);
-    var req = new QueryReqWrap();
-    req.bindingName = bindingName;
-    req.callback = callback;
-    req.hostname = name;
-    req.oncomplete = onresolve;
-    var err = binding(req, name);
-    if (err) throw errnoException(err, bindingName);
-    callback.immediately = true;
-    return req;
+    return new Resolver(binding, bindingName, name, callback);
   };
 }
 
@@ -283,6 +313,8 @@ exports.getServers = function() {
 
 
 exports.setServers = function(servers) {
+  if (_resolvesOutstanding > 0)
+    throw new Error('Cannot call setServers() while resolving.');
   // cache the original servers because in the event of an error setting the
   // servers cares won't have any servers available for resolution
   const orig = cares.getServers();

test/internet/test-dns-setservers-during-resolve.js

@@ -0,0 +1,15 @@
+'use strict';
+
+const common = require('../common');
+const dns = require('dns');
+const assert = require('assert');
+
+var m = dns.resolve('www.example.com', 'A', (err, addresses) => {
+  assert.throws(() => {
+    dns.setServers([]);
+  }, /Cannot call setServers\(\) while resolving./);
+});
+
+m.on('complete', common.mustCall(() => {
+  dns.setServers([]);
+}));

test/internet/test-dns.js

@@ -44,20 +44,13 @@ function checkWrap(req) {
 
 
 TEST(function test_reverse_bogus(done) {
-  var error;
 
-  try {
-    dns.reverse('bogus ip', function() {
-      assert.ok(false);
-    });
-  } catch (e) {
-    error = e;
-  }
-
-  assert.ok(error instanceof Error);
-  assert.strictEqual(error.errno, 'EINVAL');
+  dns.reverse('bogus ip', (err) => {
+    assert(err);
+    assert.strictEqual(err.errno, 'EINVAL');
+    done();
+  });
 
-  done();
 });
 
 

test/parallel/test-dns-regress-7070.js

@@ -5,4 +5,6 @@ var dns = require('dns');
 
 // Should not raise assertion error. Issue #7070
 assert.throws(function() { dns.resolveNs([]); }); // bad name
-assert.throws(function() { dns.resolveNs(''); }); // bad callback
+// The following is no longer valid since dns.resolve returns an
+// EventEmitter. The results can be handled by setting event listeners.
+//assert.throws(function() { dns.resolveNs(''); }); // bad callback