dns: add setDefaultResolver and getDefaultResolver #14620

XadillaX · 2017-08-04T03:38:02Z

Since dns.Resolver was added, it gives a chance to manage the default
resolver by ourselves. So dns.setDefaultResolver and
dns.getDefaultResolver is added here.

Checklist

make -j4 test (UNIX)
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

dns

Since `dns.Resolver` was added, it gives a chance to manage the default resolver by ourselves. So `dns.setDefaultResolver` and `dns.getDefaultResolver` is added here.

Trott · 2017-08-04T03:47:49Z

semver-minor?

XadillaX · 2017-08-04T05:09:40Z

@Trott yep

tniessen · 2017-08-04T12:19:33Z

As far as I know, the only difference between channels is the set of DNS servers, so which advantage does this API offer, compared to modifying the default channel using dns.setServers()? Maybe I am simply missing the big picture.

Adding getDefaultResolver() kind of makes sense to expose the cancel() function for requests in the default channel, but I am not sure we want to give users the ability to do that. A lot of modules might use the default channel at the same time, and if one of them decides to cancel all outstanding requests, this will be difficult to debug and most probably not what the user wanted, so it might be a good idea to prevent people from calling cancel on resolvers they did not create themselves.

It might be a better approach to be able to pass Resolver instances to those parts of our API which need to resolve hostnames.

XadillaX · 2017-08-04T14:00:01Z

@tniessen If we have several DNS servers to be candidates and we sometime choose one of them, I think this will be much more convenient.

const resolvers = {
    google: new Resolver(), // assume we've already `setServer([ '8.8.8.8', '8.8.4.4' ])`
    other: new Resolver()
};

// some logic
function changeDefaultResolver(name) {
    dns.setDefaultResolver(resolvers[name]);
}

And if no this function, we should do this:

const servers = {
    google: [ '8.8.8.8', '8.8.4.4' ],
    other: [ ... ]
};

function changeServer(name) {
    dns.setServer(servers[name]);
}

And what's more, if we add the timeout options in Resolver's constructor on later versions, the resolver's difference will be not only name servers.

cjihrig · 2017-08-04T14:27:03Z

lib/dns.js

+
+function setDefaultResolver(resolver) {
+  if (!(resolver instanceof Resolver)) {
+    throw new errors.TypeError('ERR_INVALID_DNS_RESOLVER');


Why not just reuse the existing ERR_INVALID_ARG_TYPE?

jasnell · 2017-08-04T14:41:23Z

doc/api/dns.md

+-->
+
+Get the default resolver which may used by Node.js modules such as `http` and
+provides functions like `dns.resolve()`, `dns.setServers()`, etc.


Perhaps just...

Returns the default `dns.Resolver` used by Node.js.

jasnell · 2017-08-04T14:41:53Z

doc/api/dns.md

+-->
+
+Set the default resolver which may used by Node.js modules such as `http` and
+provides functions like `dns.resolve()`, `dns.setServers()`, etc.


Perhaps just...

Changes the default `Resolver` used by Node.js.

jasnell · 2017-08-04T14:42:10Z

doc/api/dns.md

+Set the default resolver which may used by Node.js modules such as `http` and
+provides functions like `dns.resolve()`, `dns.setServers()`, etc.
+
+The only parameter must be a `dns.Resolver` instance that to replace the default


I'd omit this sentence.

jasnell · 2017-08-04T14:42:50Z

doc/api/dns.md

+Get the default resolver which may used by Node.js modules such as `http` and
+provides functions like `dns.resolve()`, `dns.setServers()`, etc.
+
+## dns.setDefaultResolver()


### dns.setDefaultResolver(resolver)  * `resolver` {dns.Resolver}

jasnell · 2017-08-04T14:47:33Z

lib/dns.js

+  defaultResolver = resolver;
+  DEFAULT_RESOLVER_EXPORTED_FUNCTIONS.forEach((key) => {
+    module.exports[key] = defaultResolver[key].bind(defaultResolver);
+  });


This feels... hackish. Perhaps changing the module.exports properties into getters that return the equivalent method from defaultResolver would be a cleaner approach?

e.g.

module.exports = { /** ... **/ get getServers() { return defaultResolver.getServers.bind(defaultResolver); } /** ... **/ }

That way the module's exported properties never change.

tniessen · 2017-08-04T15:10:31Z

IMO we should minimize the side effects of our API, and certainly not encourage using APIs with side effects. Having a default channel is unavoidable both for convenience and backward compatibility, but promoting its use (and primarily reconfiguration) does not seem like a good idea to me.

If multiple modules try to configure the default channel or even attempt to replace it, just to enforce specific options for their DNS requests, they will interfere with each other. Instead, if a module requires the resolver to have specific properties, it should create a new resolver and either use it directly or pass it to the respective API (which we would need to adapt accordingly, e.g. by adding a resolver option, or figuring out how to glue resolve to lookup).

cc @addaleax for designing the Resolver API

addaleax · 2017-08-04T15:13:02Z

@tniessen I’m already 👍ing your comments because I really have nothing to add, I just very plainly agree with you. :)

jasnell · 2017-08-04T20:33:32Z

Thinking about it further, I agree. Rather than allowing the default to be overridden, I'd prefer to make it easier / possible to use specific Resolver instances where necessary.

XadillaX · 2017-08-06T05:35:57Z

So does this PR still need opening?

jasnell · 2017-08-08T22:58:43Z

Let's leave it open for the time being. We should investigate how feasible it would be to support the other option of passing in Resolver instances where appropriate.

BridgeAR · 2017-09-19T21:32:08Z

@jasnell what should be done here? I am not certain how we will progress with it and is just keeping it open the right thing?

jasnell · 2017-09-20T08:09:28Z

I'm assigning myselfto this. I'll be able to look at it in detail in the next week or so.

BridgeAR · 2017-11-22T18:23:40Z

Closing due to long inactivity. If anyone thinks this should be reopened, please feel free to do so.

jasnell · 2017-11-22T18:29:42Z

ping ... perhaps @addaleax has some thoughts?

addaleax · 2017-11-22T18:55:57Z

Yeah, like I said above, I don’t think we want this. @XadillaX also opened #14891, which seems like a better way of basically achieving the same thing

dns: add setDefaultResolver and getDefaultResolver

d2ed111

Since `dns.Resolver` was added, it gives a chance to manage the default resolver by ourselves. So `dns.setDefaultResolver` and `dns.getDefaultResolver` is added here.

nodejs-github-bot added dns Issues and PRs related to the dns subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Aug 4, 2017

add test case of error

b57d85c

mscdex added semver-minor PRs that contain new features and should be released in the next minor version. and removed errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Aug 4, 2017

cjihrig reviewed Aug 4, 2017

View reviewed changes

jasnell reviewed Aug 4, 2017

View reviewed changes

jasnell added the blocked PRs that are blocked by other issues or PRs. label Aug 8, 2017

jasnell self-assigned this Sep 20, 2017

BridgeAR closed this Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: add setDefaultResolver and getDefaultResolver #14620

dns: add setDefaultResolver and getDefaultResolver #14620

XadillaX commented Aug 4, 2017 •

edited

Loading

Trott commented Aug 4, 2017

XadillaX commented Aug 4, 2017

tniessen commented Aug 4, 2017

XadillaX commented Aug 4, 2017

cjihrig Aug 4, 2017

jasnell Aug 4, 2017

jasnell Aug 4, 2017

jasnell Aug 4, 2017

jasnell Aug 4, 2017

jasnell Aug 4, 2017

tniessen commented Aug 4, 2017

addaleax commented Aug 4, 2017

jasnell commented Aug 4, 2017

XadillaX commented Aug 6, 2017

jasnell commented Aug 8, 2017

BridgeAR commented Sep 19, 2017

jasnell commented Sep 20, 2017

BridgeAR commented Nov 22, 2017

jasnell commented Nov 22, 2017

addaleax commented Nov 22, 2017

dns: add setDefaultResolver and getDefaultResolver #14620

dns: add setDefaultResolver and getDefaultResolver #14620

Conversation

XadillaX commented Aug 4, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Trott commented Aug 4, 2017

XadillaX commented Aug 4, 2017

tniessen commented Aug 4, 2017

XadillaX commented Aug 4, 2017

cjihrig Aug 4, 2017

Choose a reason for hiding this comment

jasnell Aug 4, 2017

Choose a reason for hiding this comment

jasnell Aug 4, 2017

Choose a reason for hiding this comment

jasnell Aug 4, 2017

Choose a reason for hiding this comment

jasnell Aug 4, 2017

Choose a reason for hiding this comment

jasnell Aug 4, 2017

Choose a reason for hiding this comment

tniessen commented Aug 4, 2017

addaleax commented Aug 4, 2017

jasnell commented Aug 4, 2017

XadillaX commented Aug 6, 2017

jasnell commented Aug 8, 2017

BridgeAR commented Sep 19, 2017

jasnell commented Sep 20, 2017

BridgeAR commented Nov 22, 2017

jasnell commented Nov 22, 2017

addaleax commented Nov 22, 2017

XadillaX commented Aug 4, 2017 •

edited

Loading