dns.setServers() crash

[BLeQuerrec]

Hi,

In a Vagrant Ubuntu environment, node crashes with this code:

$ cat dns.js
var dns = require('dns');

dns.resolveSoa('example.org', function(err, soa){
    console.log(soa.nsname);
    dns.resolve4(soa.nsname, function (err, nameServers) {
        console.log(dns.getServers());
        dns.setServers(nameServers);
        console.log(dns.getServers());
    });
});

Expected result:

$ node dns.js
sns.dns.icann.org
[ '10.0.2.3' ]
[ '199.4.28.26' ]

Actual result:

$ node dns.js
sns.dns.icann.org
[ '10.0.2.3' ]
node: ../deps/cares/src/ares_destroy.c:102: ares__destroy_servers_state: Assertion `ares__is_list_empty(&server->queries_to_server)' failed.
Aborted (core dumped)

Other informations:

$ cat /etc/os-release
NAME="Ubuntu"
VERSION="14.04.1 LTS, Trusty Tahr"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 14.04.1 LTS"
VERSION_ID="14.04"
HOME_URL="http://www.ubuntu.com/"
SUPPORT_URL="http://help.ubuntu.com/"
BUG_REPORT_URL="http://bugs.launchpad.net/ubuntu/"
$ node -v
v0.12.0

DNS test (in test/simple/test-dns.js) ends successfully.

[amir-s]

I have the same problem with latest versions of node and iojs.

[tjfontaine]

Indeed I've confirmed this, we'll need to get this fixed.

[BLeQuerrec]

Why do you need the line 102 in deps/cares/src/ares_destroy.c? I can fix the bug by removing it.

[silverwind]

@BastienLQ assertions are usually put there for a reason, and while just removing it might fix the crash, the whole library could be in an undefined state, which is very bad.

[jasnell]

Looking it over, I'm not sure it's an actual bug. In cares, ares-send.c invokes the callback before cleaning up. That assert appears to be designed to catch the fact that things might still be in flight when the attempt to change the servers is called.

Test this:

var dns = require('dns');
dns.resolveSoa('jasnell.me', function(err, soa){});
dns.setServers(['0.0.0.0']);

Then this:

var dns = require('dns');
dns.resolveSoa('jasnell.me', function(err, soa){});
setTimeout(
  function() {
    dns.setServers(['0.0.0.0']);
  }, 5 * 1000
);

In other words, if we delay the dns.setServers call to give cares time to complete it's cleanup, things work. Otherwise, assertion failure.

[BLeQuerrec]

It is a bug because dns.setServers() (the function itself, not in the JS script as you did) should wait cares ends the cleanup. This is actually how io.js devs will fix this bug : nodejs/node#894 (comment).

[jasnell]

@BastienLQ +1... I'll monitor @indutny's progress but will look at this myself again once I'm not in conference hell. (and when I say it's not a bug, I meant it in the sense that the code is working as it was written... it just wasn't written with this particular case in mind ;-) ... isn't async programming fun?)

[indutny]

@jasnell the idea that I have in mind is to create new cares channel on servers change. Just didn't get time to actually implement it.

[tjfontaine]

The bug is whether or not we should be tracking outstanding requests to c-ares, and then letting them drain before calling setServers or if we should be creating a new channel like @indunty suggests.

Draining requests before setting the server will probably come with too many latency problems. The question I would have is if we destroy the existing channel and any of its outstanding requests, or if we maintain a list of active channels.

Destroying the existing channel should have the effect of canceling outstanding requests, do those outstanding requests receive something of an ECANCELED then from c-ares (looks like ARES_EDESTRUCTION)? or will we need to track and deliver those?

Maintaining a list of active channels seems ideal, in that environment making sure we're doing appropriate reference counting will be crucial.

We should probably document the anti-pattern of:

function oneOff(server, query) {
  oldServers = dns.getServers();
  dns.setServers(server);
  dns.resolve(query, function cb() {
    dns.setServers(oldServers);
  });
}

Is probably not what people want, and instead should be looking to leverage an API that allows for queries to be associated with a specific request. Something like what I've done for the native-dns.