Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Commit

Permalink
Force the DNS module to invoke callbacks asynchronously.
Browse files Browse the repository at this point in the history 
Fixes #1164.
  • Loading branch information
@koichik
koichik committed Aug 21, 2011
1 parent 09f753f commit 1aed45e
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 0 deletions.
44 changes: 44 additions & 0 deletions lib/dns_legacy.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,25 @@ var channel = new dns.Channel({SOCK_STATE_CB: function(socket, read, write) {
updateTimer();
}});

// Make sure that the callback is invoked asynchronously.
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') {
Expand Down Expand Up @@ -121,13 +140,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;

This comment has been minimized.

Sign in to view

Copy link
@bnoordhuis

bnoordhuis Aug 21, 2011

Member

Why is callback.immediately = true necessary and why that particular approach? Add a doc comment to makeAsync that explains the how and why.

This comment has been minimized.

Sign in to view

Copy link
@koichik

koichik Aug 22, 2011

Author

@bnoordhuis - It necessary to avoid invoking nextTick() twice.
It is commented by line 101 (not enought?).
This is the same as:

exports.getHostByName = function(domain, family/*=4*/, callback) {
  if (typeof family === 'function') { callback = family; family = null; }
  var immediately = false;
  channel.getHostByName(domain, familyToSym(family), function() {
    if (immediately) {
      // dns.getHostByName() already returned, we can invoke the callback immediately.
      callback.apply(null, arguments);
    } else {
      var args = arguments;
      process.nextTick(function() {
        callback.apply(null, args);
      });
    }
  });
  immediately = true;
};

This comment has been minimized.

Sign in to view

Copy link
@bnoordhuis

bnoordhuis Aug 22, 2011

Member

It wasn't immediately obvious (to me anyway), I had to deduce its purpose by reading the patch. A two or three line doc comment that points the reader in the right direction would help.

This comment has been minimized.

Sign in to view

Copy link
@koichik

koichik Aug 22, 2011

Author

I see. I will add the comment.

This comment has been minimized.

Sign in to view

Copy link
@koichik

koichik Aug 23, 2011

Author

@bnoordhuis - done. please review 528cd4a.
as usual, please correct my bad English :)
};


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


Expand All @@ -148,6 +171,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);
Expand All @@ -166,6 +190,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;
}

Expand All @@ -183,6 +208,7 @@ exports.lookup = function(domain, family, callback) {
callback(err, []);
}
});
callback.immediately = true;
return;
}

Expand All @@ -200,46 +226,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,
Expand All @@ -264,3 +307,4 @@ exports.DESTRUCTION = dns.DESTRUCTION;
exports.NOTIMP = dns.NOTIMP;
exports.EREFUSED = dns.EREFUSED;
exports.SERVFAIL = dns.SERVFAIL;

24 changes: 24 additions & 0 deletions lib/dns_uv.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,24 @@ function symToFamily(family) {
}
}

// Make sure that the callback is invoked asynchronously.
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)
Expand All @@ -68,6 +86,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);
Expand All @@ -87,6 +106,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 {};
} */

Expand All @@ -103,6 +123,7 @@ exports.lookup = function(domain, family, callback) {
throw errnoException(errno, 'getHostByName');
}

callback.immediately = true;
return wrap;
};

Expand All @@ -119,11 +140,13 @@ function resolver(bindingName) {
}
}

callback = makeAsync(callback);
var wrap = binding(name, onanswer);
if (!wrap) {
throw errnoException(errno, bindingName);
}

callback.immediately = true;
return wrap;
}
}
Expand Down Expand Up @@ -171,3 +194,4 @@ exports.NOTFOUND = 'ENOTFOUND';
exports.NOTIMP = 'ENOTIMP';
exports.SERVFAIL = 'ESERVFAIL';
exports.TIMEOUT = 'ETIMEOUT';

52 changes: 52 additions & 0 deletions test/disabled/test-dns.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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');

Expand All @@ -94,6 +98,7 @@ function checkDnsRecord(host, record) {

child_process.exec(reverseCmd, checkReverse(ip));
}
assert.equal(footprints, 'beforeafter');
});
break;
case 'MX':
Expand All @@ -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':
Expand All @@ -118,6 +124,7 @@ function checkDnsRecord(host, record) {
strResult.push('"' + result[ll] + '"');
}
cmpResults(expected, strResult, ttl, cname);
assert.equal(footprints, 'beforeafter');
});
break;
case 'SRV':
Expand All @@ -133,9 +140,11 @@ function checkDnsRecord(host, record) {
result[ll].name);
}
cmpResults(expected, strResult, ttl, cname);
assert.equal(footprints, 'beforeafter');
});
break;
}
footprints += 'after';
}
}

Expand Down Expand Up @@ -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';

0 comments on commit 1aed45e

Please sign in to comment.