dns.setServers doesn't play well with async code

@indutny

nodejs/node-v0.x-archive#9243 (comment) led me to realizing that the way dns.setServers is implemented doesn't play too well with async code.

The current target server is stored in a per-app global state, which would probably be fine with sync code, but complicates things if you want to query multiple servers with our async resolve.

I'd propose adding an options object to resolve containing servers, while deprecating dns.setServers in a major version. I think having an option object on resolve methods could prove useful later.

Note: I haven't checked if c-ares enables per-query target servers easily, maybe @indutny can comment on that part.

Hm... this is not a bad suggestion at all, but I this is not the way c-ares works :) The thing that you propose will require creating and keeping alive separate channels in a hashmap.

Ah, so there is a reason it's implemented in this way.

Maybe something that should be kept in mind for #1013.

Another solution would be to allow multiple "resolver" instances, e.g. support the following pattern:

var resolver = dns.createResolver(server);
resolver.lookup(...);
resolver.close();

The default lookup method would just delegate to a default resolver.

Please don't close this, I didn't mean that it wasn't feasible :) Just wanted to stress out how it will be implemented.

"resolver" instances

This is what node-dns basically does. It's an option, but I think I'm leaning more towards the option object for simplicity.

Atm, dns.setServers asserts when there are running dns queries or callbacks of dns queries.

This crashes:

var dns = require('dns');

var servers = dns.getServers();
dns.setServers(['208.67.222.222', '208.67.220.220']);
dns.resolve4('myip.opendns.com', function(err, addresses, family) {
    dns.setServers(servers);
    console.log('addresses:', addresses);
});

This works:

var dns = require('dns');

var servers = dns.getServers();
dns.setServers(['208.67.222.222', '208.67.220.220']);
dns.resolve4('myip.opendns.com', function(err, addresses, family) {
    setTimeout(function() { dns.setServers(servers); }, 0);
    console.log('addresses:', addresses);
});

@ChALkeR

@ChALkeR this is known, see #894.

I hope this idea hasn't died. I'd really like to see this functionality. The node-dns alternative doesn't appear to be functioning at all in 2.2.1.

I have a change for this i mind after the new DNS implementation lands (#1843 (comment))

I'm going to add a stalled label until #1843 comes together. Feel free to remove if that's an abuse of that particular label.

@mscdex

It looks like #1843 might be stalling out (@mscdex). Thinking about this a bit, I'm wondering if a possible stop-gap "solution" would be to modify setServers() such that it will throw early if there are any pending resolves that have not yet returned. Ultimately I think the right solution is to allow multiple resolvers as @piscisaureus has suggested but that's a larger change. If we did go that route, setServers() would be deprecated anyway. Adding the throw would seem to be better than allowing the current existing failure.

@jasnell

@jasnell I'm all for it if it avoids the crash. It would give the user a chance to workaround.

Regarding alternatives, @mafintosh's dns-socket is a pretty flexible JS DNS solution I've been using recently. Maybe worth investigating how its perf compares to @mscdex's implementation.

@silverwind

Hey @silverwind, does #13050 fix this?

Yes, I believe #13050 actually does fix this. Good catch. There are still a number of ways that the dns module can be improved, but the change in #13050 addresses the immediate issue. Closing. In any of the collaborators feel that it's necessary to do so, we can reopen the issue.

Navigation Menu

Repository navigation

`dns.setServers` doesn't play well with async code #1071

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

dns.setServers doesn't play well with async code #1071

Description

Activity

indutny commented on Mar 5, 2015

silverwind commented on Mar 5, 2015

piscisaureus commented on Mar 5, 2015

indutny commented on Mar 5, 2015

silverwind commented on Mar 5, 2015

ChALkeR commented on Mar 5, 2015

silverwind commented on Mar 6, 2015

snsparrish commented on Jun 7, 2015

silverwind commented on Jun 7, 2015

Trott commented on Mar 11, 2016

jasnell commented on Apr 19, 2016

silverwind commented on Apr 19, 2016

XadillaX commented on May 17, 2017

jasnell commented on May 30, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`dns.setServers` doesn't play well with async code #1071