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.

[jasnell]

@indutny creating the new channel on setServers sounds like a good approach. Once the old channel drains it falls off and get's de-ref'd. This allows us to keep from having to cancel inflight requests. Even so, as @tjfontaine indicates, it's an antipattern that folks should avoid. I'll see if I can get to this later this week but please cc me if you get to it first.

[gchudnov]

It looks setServers sets the global state. Is there a way to issue a resolve request with some specific server in node | io.js?

Something like: dig @172.17.42.1 "db-mysql.production.docker"

We have an server app in a docker environment with the custom DNS server deployed. Some DNS-requests should be issued against our private DNS server. And it looks the global state makes to issue this request difficult...

[silverwind]

@gchudnov indeed, that's not easily achieved with setServers, and I think it shows a bit of a shortcoming on how setServers was implemented. I think an option on resolve for a target server might've been the better approach. Maybe it's not too late to add that.

[misterdjules]

Just to track some of the progress that has been made on this, @emotional-engineering has worked on some changes and their integration into io.js plus some tests.

EDIT: I have tested these changes in node v0.12 and they fix the crash mentioned in the original comment of this issue.

I'm not familiar enough to review the changes themselves, but they seem to be heading in the right direction to solve the original issue (dns.setServers asserting), even if they still seem to be incomplete.

@emotional-engineering You mentioned that the changes have been sent to the c-ares mailing list but looking at the c-ares mailing list archives, I don't see any message related to that. Did I miss something?

Thank you!

[emotional-engineering]

@misterdjules Yes, I missed to complete verification my email in mailing list, thank you.

[misterdjules]

@emotional-engineering Thank you!

Moving to 0.12.4, as I don't think we'll have time to come up with a robust solution before that.

[whitlockjc]

What is remaining for this issue? I'd love to help if there is remaining work but that is a little unclear based on the discussion above.

[silverwind]

I'm not sure we'll ever see an upstream fix for this in c-ares in a reasonable time frame. The best bet on this whole DNS business is nodejs/node#1843, which has stalled a bit unfortunately, but if you have feedback or can review the code, that'd help!

[ChALkeR]

This is reported to the current repo as nodejs/node#894, no reason to keep it open in two places.