From 485d5b5dff42d6d597e4e3107d7eb55edbf0a1b3 Mon Sep 17 00:00:00 2001 From: koichik Date: Sat, 13 Aug 2011 03:09:34 +0900 Subject: [PATCH] dns: Force the DNS module to invoke callbacks asynchronously. Fixes #1164. --- lib/dns_legacy.js | 58 +++++++++++++++++++++++++++++ lib/dns_uv.js | 38 +++++++++++++++++++ test/disabled/test-dns.js | 52 ++++++++++++++++++++++++++ test/simple/test-dgram-multicast.js | 5 ++- 4 files changed, 152 insertions(+), 1 deletion(-) diff --git a/lib/dns_legacy.js b/lib/dns_legacy.js index 441b488d29..b4d7e008cb 100644 --- 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 index 60562f8032..eccbfd5d2a 100644 --- 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 index a7d6117830..6c17868fe6 100644 --- 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 index 50c957d47c..1a610f6176 100644 --- 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(); + }); } });