Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Native DNS caching for lookups #5893

Closed
robertschultz opened this issue Mar 24, 2016 · 20 comments
Closed

Native DNS caching for lookups #5893

robertschultz opened this issue Mar 24, 2016 · 20 comments
Labels
dns Issues and PRs related to the dns subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@robertschultz
Copy link

Will we ever see caching in the native dns module? We have noticed a decent amount of overhead when making many parallel calls to the same dns and the dns module is doing lookups.

I understand the idea around this module is to be unopinionated but it might be a feature typical users might want to enable w/o using something like bind.

@robertschultz robertschultz changed the title dns caching DNS caching? Mar 24, 2016
@robertschultz robertschultz changed the title DNS caching? Native DNS caching for lookups Mar 24, 2016
@mscdex
Copy link
Contributor

mscdex commented Mar 24, 2016

What about using dnsmasq? Also for net.Sockets for example, you can specify your own lookup() function when connecting which could utilize a custom cache.

@mscdex mscdex added dns Issues and PRs related to the dns subsystem. feature request Issues that request new features to be added to Node.js. labels Mar 24, 2016
@robertschultz
Copy link
Author

@mscdex In regards to custom lookup() that's true. My request really is for the general masses, and the overhead 99.99% of people would have using the stock dns module. My main argument here is that the request module is using things like url.resolve so I'd have to monkey patch this or something.

@bompus
Copy link

bompus commented Mar 25, 2016

+1

@justsml
Copy link

justsml commented Mar 25, 2016

While this sounds nice, and i'd love to have an option to just 'turn on fast DNS'.
This would really burden Node's core. DNS is a well established tech, with really significant thought put into cache, expiration, failure handling, etc. Making this builtin would fight the way it's supposed to be utilized: as @mscdex mentioned, run a local dnsmasq cache - or any other DNS cache - again this should be a system wide design decision.
For example, It would be VERY frustrating if your nodejs process could use a different IP when debugging side-by-side using a curl cmd.
The reasons against this are numerous, and could open the system up to security issues, race conditions, inconsistent data, etc.

Just set up a local resolver, let it do what it does best. As a bonus your whole system will operate faster.

@mscdex
Copy link
Contributor

mscdex commented Mar 25, 2016

@justsml You wouldn't have to use it system-wide unless you wanted to. node's dns module supports setting explicit nameservers (at least for dns.resolve()).

@robertschultz
Copy link
Author

@justsml I'm in favor of a local resolver as well, just calling this out if it was worth bringing it up. Thanks.

@benjamingr
Copy link
Member

I'm -1 on this, this is something that userland modules should solve and caching bugs are worse than performance bugs.

I think the caching should be done outside the dns module anywy - as in - the DNS module always works "as advertised" and performs a clean lookup to the nameservers.

@benjamingr
Copy link
Member

Here is a userland solution that works https://www.npmjs.com/package/dnscache

@jasnell
Copy link
Member

jasnell commented May 13, 2016

Given the responses it's unlikely that we'll end up doing this. Going to close. Feel free to reopen if necessary.

@jasnell jasnell closed this as completed May 13, 2016
@reallistic
Copy link

Just to add my two cents in, it is very dangerous to use the dnscache package because it does not respect the ttl set by the dns server. This means that you could potentially have a bad dns cache for some time. By implementing a OS level solution, you'll get all the benefits mentioned above in addition to the reassurance that your dns entries will always be correct.

@jbergstroem
Copy link
Member

@reallistic couldn't dnscache be patched then? Reading above it seems unlikely to land in core.

@reallistic
Copy link

@jbergstroem what I mean is that I agree with the comments above that this shouldn't fall on Node.js' hands and also warning that the dnscache module suggested could cause more problems.

@PaquitoSoft
Copy link

Based on the comments exposed here about not implementing DNS cache on nodejs core and the comment from @reallistic about potential problems using dnscache module, I would like to know how can we get the TTL set from the DNS server so we can correctly implement DNS caching in our applications.
The dns.lookup function will only return and address (and a family) but not that value. So, if we want to honor the TTL the DNS server sets, what would be the best approach?

Thanks!

@bnoordhuis
Copy link
Member

@PaquitoSoft dns.lookup() uses getaddrinfo(3) which doesn't report the TTL. For the A and AAAA flavors of dns.resolve() we can retrieve it though. I've opened #9296 for that.

@PaquitoSoft
Copy link

@bnoordhuis thanks!
It sounds great.

@reallistic
Copy link

reallistic commented Oct 30, 2016

@PaquitoSoft I highly recommend fixing this at the OS level. Using bind, dnsmasq, or something similar.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Nov 18, 2016
Add an option to retrieve the Time-To-Live of the A record.

PR-URL: nodejs#9296
Refs: nodejs#5893
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Nov 18, 2016
Add an option to retrieve the Time-To-Live of the AAAA record.

PR-URL: nodejs#9296
Refs: nodejs#5893
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Fishrock123 pushed a commit that referenced this issue Nov 22, 2016
Add an option to retrieve the Time-To-Live of the A record.

PR-URL: #9296
Refs: #5893
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Fishrock123 pushed a commit that referenced this issue Nov 22, 2016
Add an option to retrieve the Time-To-Live of the AAAA record.

PR-URL: #9296
Refs: #5893
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this issue May 16, 2017
Add an option to retrieve the Time-To-Live of the A record.

PR-URL: #9296
Refs: #5893
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this issue May 16, 2017
Add an option to retrieve the Time-To-Live of the AAAA record.

PR-URL: #9296
Refs: #5893
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this issue May 18, 2017
Add an option to retrieve the Time-To-Live of the A record.

PR-URL: #9296
Refs: #5893
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this issue May 18, 2017
Add an option to retrieve the Time-To-Live of the AAAA record.

PR-URL: #9296
Refs: #5893
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Add an option to retrieve the Time-To-Live of the A record.

PR-URL: nodejs/node#9296
Refs: nodejs/node#5893
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Add an option to retrieve the Time-To-Live of the AAAA record.

PR-URL: nodejs/node#9296
Refs: nodejs/node#5893
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@eduardbme
Copy link
Contributor

Take a look at https://www.npmjs.com/package/lookup-dns-cache

@benjamingr
Copy link
Member

@bnoordhuis can this be closed given that #9296 landed?

@bnoordhuis
Copy link
Member

@benjamingr It's closed already.

@titanism
Copy link

We released 🍊 Tangerine today, which is a userland package tangerine that solves DNS caching for Node.js. It's a ⚡ faster ⚡ 1:1 drop-in replacement for dns.promises.Resolver using DNS over HTTPS ("DoH") via undici with built-in retries, timeouts, smart server rotation, AbortControllers, and caching support for multiple backends via Keyv.

npm install tangerine
-import dns from 'dns';
+import Tangerine from 'tangerine';

- const resolver = new dns.promises.Resolver();
+const resolver = new Tangerine();

Documentation, API, options, source code, tests, benchmarks, and more available at https://github.com/forwardemail/tangerine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests