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 · 19 comments

Comments

Projects
None yet
@robertschultz
Copy link

commented Mar 24, 2016

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

This comment has been minimized.

Copy link
Contributor

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.

@robertschultz

This comment has been minimized.

Copy link
Author

commented Mar 24, 2016

@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

This comment has been minimized.

Copy link

commented Mar 25, 2016

+1

@justsml

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Author

commented Mar 25, 2016

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

@benjamingr

This comment has been minimized.

Copy link
Member

commented Mar 27, 2016

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

This comment has been minimized.

Copy link
Member

commented Mar 27, 2016

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

@jasnell

This comment has been minimized.

Copy link
Member

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 May 13, 2016

@reallistic

This comment has been minimized.

Copy link

commented Oct 12, 2016

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

This comment has been minimized.

Copy link
Member

commented Oct 12, 2016

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

@reallistic

This comment has been minimized.

Copy link

commented Oct 12, 2016

@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

This comment has been minimized.

Copy link

commented Oct 26, 2016

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

This comment has been minimized.

Copy link
Member

commented Oct 26, 2016

@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

This comment has been minimized.

Copy link

commented Oct 26, 2016

@bnoordhuis thanks!
It sounds great.

@reallistic

This comment has been minimized.

Copy link

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

dns: implement {ttl: true} for dns.resolve4()
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

dns: implement {ttl: true} for dns.resolve6()
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 added a commit that referenced this issue Nov 22, 2016

dns: implement {ttl: true} for dns.resolve4()
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 added a commit that referenced this issue Nov 22, 2016

dns: implement {ttl: true} for dns.resolve6()
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 added a commit that referenced this issue May 16, 2017

dns: implement {ttl: true} for dns.resolve4()
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 added a commit that referenced this issue May 16, 2017

dns: implement {ttl: true} for dns.resolve6()
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 added a commit that referenced this issue May 18, 2017

dns: implement {ttl: true} for dns.resolve4()
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 added a commit that referenced this issue May 18, 2017

dns: implement {ttl: true} for dns.resolve6()
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 added a commit to michielbaird/node that referenced this issue Jul 19, 2017

dns: implement {ttl: true} for dns.resolve4()
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 added a commit to michielbaird/node that referenced this issue Jul 19, 2017

dns: implement {ttl: true} for dns.resolve6()
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>
@eduardbcom

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

@benjamingr

This comment has been minimized.

Copy link
Member

commented Jan 21, 2018

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

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Jan 21, 2018

@benjamingr It's closed already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.