Skip to content

Commit

Permalink
dns: gate setServers to avoid async cares conflicts
Browse files Browse the repository at this point in the history
Use an EE with some state data to gate async resolution operations in
c-ares so that setServers() can run independently.

Likely a temporary fix for nodejs#894
with a better solution being to fix c-ares to do this without barfing.
  • Loading branch information
rvagg committed Mar 12, 2015
1 parent 41c9daa commit ee5056f
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 11 deletions.
81 changes: 70 additions & 11 deletions lib/dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
};

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

Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
Expand Down
13 changes: 13 additions & 0 deletions test/parallel/test-dns-setserver-async.js
Original file line number Diff line number Diff line change
@@ -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' ]);

0 comments on commit ee5056f

Please sign in to comment.