dns: Force the DNS module to invoke callbacks asynchronously.

koichik · koichik · commit 485d5b5dff42 · 2011-08-24T03:01:41.000+09:00
Fixes #1164.
diff --git a/lib/dns_legacy.js b/lib/dns_legacy.js
@@ -91,6 +91,40 @@ var channel = new dns.Channel({SOCK_STATE_CB: function(socket, read, write) {
   updateTimer();
 }});
 
+// c-ares invokes a callback either synchronously or asynchronously,
+// but the dns API should always invoke a callback asynchronously.
+// 
+// This function makes sure that the callback is invoked asynchronously.
+// It returns a function that invokes the callback within nextTick().
+// 
+// To avoid invoking unnecessary nextTick(), `immediately` property of
+// returned function should be set to true after c-ares returned.
+// 
+// Usage:
+// 
+// function someAPI(callback) {
+//   callback = makeAsync(callback);
+//   channel.someAPI(..., callback);
+//   callback.immediately = true;
+// }
+function makeAsync(callback) {
+  if (typeof callback !== 'function') {
+    return callback;
+  }
+  return function asyncCallback() {
+    if (asyncCallback.immediately) {
+      // The API already returned, we can invoke the callback immediately.
+      callback.apply(null, arguments);
+    } else {
+      var args = arguments;
+      process.nextTick(function() {
+        callback.apply(null, args);
+      });
+    }
+  };
+}
+
+
 exports.resolve = function(domain, type_, callback_) {
   var type, callback;
   if (typeof(type_) == 'string') {
@@ -121,13 +155,17 @@ function familyToSym(family) {
 
 exports.getHostByName = function(domain, family/*=4*/, callback) {
   if (typeof family === 'function') { callback = family; family = null; }
+  callback = makeAsync(callback);
   channel.getHostByName(domain, familyToSym(family), callback);
+  callback.immediately = true;
 };
 
 
 exports.getHostByAddr = function(address, family/*=4*/, callback) {
   if (typeof family === 'function') { callback = family; family = null; }
+  callback = makeAsync(callback);
   channel.getHostByAddr(address, familyToSym(family), callback);
+  callback.immediately = true;
 };
 
 
@@ -148,6 +186,7 @@ exports.lookup = function(domain, family, callback) {
       throw new Error('invalid argument: "family" must be 4 or 6');
     }
   }
+  callback = makeAsync(callback);
 
   if (!domain) {
     callback(null, null, family === 6 ? 6 : 4);
@@ -166,6 +205,7 @@ exports.lookup = function(domain, family, callback) {
     process.binding('net').getaddrinfo(domain, 4, function(err, domains4) {
       callback(err, domains4[0], 4);
     });
+    callback.immediately = true;
     return;
   }
 
@@ -183,6 +223,7 @@ exports.lookup = function(domain, family, callback) {
         callback(err, []);
       }
     });
+    callback.immediately = true;
     return;
   }
 
@@ -200,46 +241,63 @@ exports.lookup = function(domain, family, callback) {
       });
     }
   });
+  callback.immediately = true;
 };
 
 
 exports.resolve4 = function(domain, callback) {
+  callback = makeAsync(callback);
   channel.query(domain, dns.A, callback);
+  callback.immediately = true;
 };
 
 
 exports.resolve6 = function(domain, callback) {
+  callback = makeAsync(callback);
   channel.query(domain, dns.AAAA, callback);
+  callback.immediately = true;
 };
 
 
 exports.resolveMx = function(domain, callback) {
+  callback = makeAsync(callback);
   channel.query(domain, dns.MX, callback);
+  callback.immediately = true;
 };
 
 
 exports.resolveTxt = function(domain, callback) {
+  callback = makeAsync(callback);
   channel.query(domain, dns.TXT, callback);
+  callback.immediately = true;
 };
 
 
 exports.resolveSrv = function(domain, callback) {
+  callback = makeAsync(callback);
   channel.query(domain, dns.SRV, callback);
+  callback.immediately = true;
 };
 
 
 exports.reverse = function(domain, callback) {
+  callback = makeAsync(callback);
   channel.query(domain, dns.PTR, callback);
+  callback.immediately = true;
 };
 
 
 exports.resolveNs = function(domain, callback) {
+  callback = makeAsync(callback);
   channel.query(domain, dns.NS, callback);
+  callback.immediately = true;
 };
 
 
 exports.resolveCname = function(domain, callback) {
+  callback = makeAsync(callback);
   channel.query(domain, dns.CNAME, callback);
+  callback.immediately = true;
 };
 
 var resolveMap = { A: exports.resolve4,
diff --git a/lib/dns_uv.js b/lib/dns_uv.js
@@ -52,6 +52,39 @@ function symToFamily(family) {
   }
 }
 
+// c-ares invokes a callback either synchronously or asynchronously,
+// but the dns API should always invoke a callback asynchronously.
+// 
+// This function makes sure that the callback is invoked asynchronously.
+// It returns a function that invokes the callback within nextTick().
+//  
+// To avoid invoking unnecessary nextTick(), `immediately` property of
+// returned function should be set to true after c-ares returned.
+//  
+// Usage: 
+//    
+// function someAPI(callback) {
+//   callback = makeAsync(callback);
+//   channel.someAPI(..., callback);
+//   callback.immediately = true;
+// }
+function makeAsync(callback) {
+  if (typeof callback !== 'function') {
+    return callback;
+  }
+  return function asyncCallback() {
+    if (asyncCallback.immediately) {
+      // The API already returned, we can invoke the callback immediately.
+      callback.apply(null, arguments);
+    } else {
+      var args = arguments;
+      process.nextTick(function() {
+        callback.apply(null, args);
+      });
+    }
+  };
+}
+
 
 // Easy DNS A/AAAA look up
 // lookup(domain, [family,] callback)
@@ -68,6 +101,7 @@ exports.lookup = function(domain, family, callback) {
       throw new Error('invalid argument: `family` must be 4 or 6');
     }
   }
+  callback = makeAsync(callback);
 
   if (!domain) {
     callback(null, null, family === 6 ? 6 : 4);
@@ -87,6 +121,7 @@ exports.lookup = function(domain, family, callback) {
     process.binding('net').getaddrinfo(domain, 4, function(err, domains4) {
       callback(err, domains4[0], 4);
     });
+    callback.immediately = true;
     return {};
   } */
 
@@ -103,6 +138,7 @@ exports.lookup = function(domain, family, callback) {
     throw errnoException(errno, 'getHostByName');
   }
 
+  callback.immediately = true;
   return wrap;
 };
 
@@ -119,11 +155,13 @@ function resolver(bindingName) {
       }
     }
 
+    callback = makeAsync(callback);
     var wrap = binding(name, onanswer);
     if (!wrap) {
       throw errnoException(errno, bindingName);
     }
 
+    callback.immediately = true;
     return wrap;
   }
 }
diff --git a/test/disabled/test-dns.js b/test/disabled/test-dns.js
@@ -60,9 +60,12 @@ while (i--) {
 }
 
 // CNAME should resolve
+var resolveCNAME = 'before';
 dns.resolve('labs.nrcmedia.nl', 'CNAME', function(err, result) {
   assert.deepEqual(result, ['nrcmedia.nl']);
+  assert.equal(resolveCNAME, 'beforeafter');
 });
+resolveCNAME += 'after';
 
 // CNAME should not resolve
 dns.resolve('nrcmedia.nl', 'CNAME', function(err, result) {
@@ -74,6 +77,7 @@ function checkDnsRecord(host, record) {
       myRecord = record;
   return function(err, stdout) {
     var expected = [];
+    var footprints = 'before';
     if (stdout.length)
       expected = stdout.substr(0, stdout.length - 1).split('\n');
 
@@ -94,6 +98,7 @@ function checkDnsRecord(host, record) {
 
             child_process.exec(reverseCmd, checkReverse(ip));
           }
+          assert.equal(footprints, 'beforeafter');
         });
         break;
       case 'MX':
@@ -106,6 +111,7 @@ function checkDnsRecord(host, record) {
             strResult.push(result[ll].priority + ' ' + result[ll].exchange);
           }
           cmpResults(expected, strResult, ttl, cname);
+          assert.equal(footprints, 'beforeafter');
         });
         break;
       case 'TXT':
@@ -118,6 +124,7 @@ function checkDnsRecord(host, record) {
             strResult.push('"' + result[ll] + '"');
           }
           cmpResults(expected, strResult, ttl, cname);
+          assert.equal(footprints, 'beforeafter');
         });
         break;
       case 'SRV':
@@ -133,9 +140,11 @@ function checkDnsRecord(host, record) {
                            result[ll].name);
           }
           cmpResults(expected, strResult, ttl, cname);
+          assert.equal(footprints, 'beforeafter');
         });
         break;
     }
+    footprints += 'after';
   }
 }
 
@@ -174,3 +183,46 @@ function cmpResults(expected, result, ttl, cname) {
                 ' was equal to expected ' + expected[ll]);
   }
 }
+
+// #1164
+var getHostByName = 'before';
+dns.getHostByName('localhost', function() {
+  assert.equal(getHostByName, 'beforeafter');
+});
+getHostByName += 'after';
+
+var getHostByAddr = 'before';
+dns.getHostByAddr('127.0.0.1', function() {
+  assert.equal(getHostByAddr, 'beforeafter');
+});
+getHostByAddr += 'after';
+
+var lookupEmpty = 'before';
+dns.lookup('', function() {
+  assert.equal(lookupEmpty, 'beforeafter');
+});
+lookupEmpty += 'after';
+
+var lookupIp = 'before';
+dns.lookup('127.0.0.1', function() {
+  assert.equal(lookupIp, 'beforeafter');
+});
+lookupIp += 'after';
+
+var lookupIp4 = 'before';
+dns.lookup('127.0.0.1', 4, function() {
+  assert.equal(lookupIp4, 'beforeafter');
+});
+lookupIp4 += 'after';
+
+var lookupIp6 = 'before';
+dns.lookup('ietf.org', 6, function() {
+  assert.equal(lookupIp6, 'beforeafter');
+});
+lookupIp6 += 'after';
+
+var lookupLocal = 'before';
+dns.lookup('localhost', function() {
+  assert.equal(lookupLocal, 'beforeafter');
+});
+lookupLocal += 'after';
diff --git a/test/simple/test-dgram-multicast.js b/test/simple/test-dgram-multicast.js
@@ -78,7 +78,10 @@ function mkListener() {
 
     if (receivedMessages.length == sendMessages.length) {
       listenSocket.dropMembership(LOCAL_BROADCAST_HOST);
-      listenSocket.close();
+      process.nextTick(function() { // TODO should be changed to below.
+      // listenSocket.dropMembership(LOCAL_BROADCAST_HOST, function() {
+        listenSocket.close();
+      });
     }
   });
 
