dns: gate setServers to avoid async cares conflicts

lib/dns.js

@@ -2,6 +2,7 @@
 
 const net = require('net');
 const util = require('util');
+const EventEmitter = require('events').EventEmitter;
 
 const cares = process.binding('cares_wrap');
 const uv = process.binding('uv');
@@ -11,6 +12,12 @@ const GetNameInfoReqWrap = cares.GetNameInfoReqWrap;
 
 const isIp = net.isIP;
 
+// `resolving` serves as a gate to `setServers()` which is not async
+// friendly, we have to wait for resolutions to finish before changing
+// the servers in c-ares
+const resolving = new EventEmitter();
+resolving.count = 0;
+resolving.paused = false;
 
 function errnoException(err, syscall, hostname) {
   // FIXME(bnoordhuis) Remove this backwards compatibility shite and pass
@@ -56,7 +63,10 @@ function makeAsync(callback) {
   if (typeof callback !== 'function') {
     return callback;
   }
+  resolving.count++;
   return function asyncCallback() {
+    if (--resolving.count === 0)
+      resolving.emit('empty');
     if (asyncCallback.immediately) {
       // The API already returned, we can invoke the callback immediately.
       callback.apply(null, arguments);
@@ -164,13 +174,20 @@ exports.lookup = function lookup(hostname, options, callback) {
   req.hostname = hostname;
   req.oncomplete = all ? onlookupall : onlookup;
 
-  var err = cares.getaddrinfo(req, hostname, family, hints);
-  if (err) {
-    callback(errnoException(err, 'getaddrinfo', hostname));
-    return {};
+  function enqueue() {
+    var err = cares.getaddrinfo(req, hostname, family, hints);
+    if (err) {
+      callback(errnoException(err, 'getaddrinfo', hostname));
+      return {};
+    }
+    callback.immediately = true;
+    return req;
   }
 
-  callback.immediately = true;
+  if (!resolving.paused)
+    return enqueue();
+
+  resolving.once('ready', enqueue);
   return req;
 };
 
@@ -199,10 +216,18 @@ exports.lookupService = function(host, port, callback) {
   req.port = port;
   req.oncomplete = onlookupservice;
 
-  var err = cares.getnameinfo(req, host, port);
-  if (err) throw errnoException(err, 'getnameinfo', host);
+  function enqueue() {
+    var err = cares.getnameinfo(req, host, port);
+    if (err) throw errnoException(err, 'getnameinfo', host);
+  }
 
   callback.immediately = true;
+
+  if (resolving.paused)
+    resolving.once('ready', enqueue);
+  else
+    return enqueue();
+
   return req;
 };
 
@@ -232,9 +257,19 @@ function resolver(bindingName) {
       hostname: name,
       oncomplete: onresolve
     };
-    var err = binding(req, name);
-    if (err) throw errnoException(err, bindingName);
+
+    function enqueue() {
+      var err = binding(req, name);
+      if (err) throw errnoException(err, bindingName);
+    }
+
     callback.immediately = true;
+
+    if (resolving.paused)
+      resolving.once('ready', enqueue);
+    else
+      return enqueue();
+
     return req;
   }
 }
@@ -278,7 +313,31 @@ exports.getServers = function() {
 };
 
 
-exports.setServers = function(servers) {
+exports.setServers = function setServers(servers) {
+  if (resolving.paused) {
+    return resolving.once('ready', function() {
+      setServers(servers);
+    });
+  }
+
+  if (resolving.count === 0) {
+    return _setServers(servers);
+  }
+
+  resolving.paused = true
+  resolving.once('empty', function() {
+    setImmediate(function() {
+      try {
+        _setServers(servers);
+      } finally {
+        resolving.paused = false;
+        resolving.emit('ready');
+      }
+    });
+  });
+}
+
+function _setServers(servers) {
   // cache the original servers because in the event of an error setting the
   // servers cares won't have any servers available for resolution
   var orig = cares.getServers();
@@ -319,7 +378,7 @@ exports.setServers = function(servers) {
     throw new Error('c-ares failed to set servers: "' + err +
                     '" [' + servers + ']');
   }
-};
+}
 
 // uv_getaddrinfo flags
 exports.ADDRCONFIG = cares.AI_ADDRCONFIG;

test/parallel/test-dns-setserver-async.js

@@ -0,0 +1,13 @@
+// should not crash
+// see https://github.com/iojs/io.js/issues/894 for what this is trying
+// to test
+
+var common = require('../common');
+var assert = require('assert');
+var dns = require('dns');
+
+dns.resolve4('www.microsoft.com', function (err, result) {
+  dns.setServers([ '8.8.8.9' ]);
+  dns.resolve4('test.com', function (err, result) {});
+});
+dns.setServers([ '8.8.8.8' ]);