feat: add dns cache #2440 #2552

antoinegomez · 2023-12-28T11:00:31Z

This relates to #2440

Rationale

Implement a dns cache lookup feature as requested.

Copy code from https://github.com/szmarczak/cacheable-lookup/blob/master/source/index.mjs

Changes

add lookup in core/connect.js

Features

dns cache lookup

Bug Fixes

N/A

Breaking Changes and Deprecations

N/A

Status

antoinegomez · 2023-12-28T11:03:35Z

Note that this is just an initial draft, anyone can jump in and use this as a base or if in wrong direction simply discard it.

if ipv6 is available on host and returned by lookup use this
this is a quick copy of the mentioned cacheable-lookup module and it's clearly not finished
the difficulty is to know how to set the options for this module and pass them down to the core/connect method

mcollina

I would create a new class DNSResolver and add a dnsResolver option to pool and Agent. An Agent or pool would allocate one if it's not passed in as an option. The Agent would pass down its cache to all created pools.

antoinegomez · 2023-12-28T16:05:30Z

Benchmarks:

Linux KDE Neon
AMD Ryzen 5 5600X 6-Core
4 x 8GB RAM

main branch 9b9ca60

│ Tests               │ Samples │          Result │ Tolerance │ Difference with slowest │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ http - no keepalive │       1 │ 1962.85 req/sec │  ± 0.00 % │                       - │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ http - keepalive    │       1 │ 2387.90 req/sec │  ± 0.00 % │               + 21.65 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - pipeline   │       1 │ 3035.14 req/sec │  ± 0.00 % │               + 54.63 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - request    │       1 │ 3160.77 req/sec │  ± 0.00 % │               + 61.03 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - stream     │       1 │ 3807.23 req/sec │  ± 0.00 % │               + 93.96 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - dispatch   │       1 │ 4833.26 req/sec │  ± 0.00 % │              + 146.24 % │

Feature branch:

│ Tests               │ Samples │          Result │ Tolerance │ Difference with slowest │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ http - no keepalive │       1 │ 1824.48 req/sec │  ± 0.00 % │                       - │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ http - keepalive    │       1 │ 2483.32 req/sec │  ± 0.00 % │               + 36.11 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - request    │       1 │ 3087.63 req/sec │  ± 0.00 % │               + 69.23 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - pipeline   │       1 │ 3289.09 req/sec │  ± 0.00 % │               + 80.28 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - stream     │       1 │ 3935.02 req/sec │  ± 0.00 % │              + 115.68 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - dispatch   │       1 │ 5294.26 req/sec │  ± 0.00 % │              + 190.18 % │

KhafraDev · 2023-12-29T03:59:25Z

lib/core/dns-resolver.js

+    if (entries[0].status === 'rejected' && entries[1].status === 'rejected') {
+      throw entries[0].reason
+    }


Why only throw the first error? Wouldn't it be better to throw an AggregateError that contains both?

doing that, using AggregateError and modifying tests

mcollina · 2023-12-29T07:49:11Z

lib/core/dns-resolver.js

@@ -0,0 +1,443 @@
+// source: https://raw.githubusercontent.com/szmarczak/cacheable-lookup/9e60c9f6e74a003692aec68f3ddad93afe613b8f/source/index.mjs


please include the license at the top of the file

yes no probs

test/dns-resolver.js

mcollina · 2023-12-29T07:52:26Z

lib/core/dns-resolver.js

+          )
+          cacheError.cause = error
+
+          throw cacheError


I don't think this kind of error handling is correct. Error situations should auto heal when possible

Here it is because it's possible to specify a custom cache object, if the set method throws it is considered as a fatal and unrecoverable error.

Using the default cache which is a simple Map() this would not happen.

Also happy to remove any form of custom caching, that's was part of the copied module.

mcollina · 2023-12-29T07:54:03Z

lib/core/dns-resolver.js

+      const { _iface } = this
+      cached = cached.filter((entry) =>
+        entry.family === 6 ? _iface.has6 : _iface.has4
+      )


Please avoid the use of filter, map etc, they are an order of magnitude (or more) slower than using normal iteration.

I translated the code with loops, no optimizations yet. To have the code passing all tests. Made the function less readable. Also note that here it will be common to have one or two items returns and I am yet to see anyone using more than 4-5 items per record.
Agreeing to keep an eye on the perf side.

antoinegomez · 2023-12-29T23:14:53Z

Update:

As mentioned above, happy to refactor a bit more and to remove some functionalities like the custom caching if we want to slim down the code and responsibilities. in the end people could easily use cacheable-lookup module in Undici if they want
Added a file global-dns-resolver to get/set the global instance of the resolver, as my guess is that the default behavior we want is to cache across all requests
There is no option/method to disable the DNS cache globally easily
- for now you have to create and set a new global Agent with the option dnsResolver.disable: true
By default and for the first release should we not enable it by default to test? This kind of feature could break people production if not tested more carefully. DNS sometimes can behaves in unexpected ways.

mcollina · 2023-12-30T23:24:52Z

I don't think there should be a global dns resolver. We already have a global agent, which is enough to handle this use case.

codecov-commenter · 2023-12-30T23:26:47Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (8ee39e8) 85.56%.
Report is 311 commits behind head on main.

Files	Patch %	Lines
lib/dns-resolver.js	96.71%	7 Missing ⚠️
lib/agent.js	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2552      +/-   ##
==========================================
+ Coverage   85.54%   85.56%   +0.02%     
==========================================
  Files          76       85       +9     
  Lines        6858     7810     +952     
==========================================
+ Hits         5867     6683     +816     
- Misses        991     1127     +136

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/agent.js

lib/dns-resolver.js

metcoder95

Can we add tests that covers the usage within the Agent/Pool?

lib/agent.js

KhafraDev

I forgot with my other review but this does need types as well.

KhafraDev · 2024-01-09T23:19:40Z

lib/dns-resolver.js

+  get _cache () {
+    return this.#cache
+  }


from the tests, it looks like this is only needed to check how many entries are in the cache. Would it make sense to only expose the number of cached entries, rather than the cache itself? If not, changing these to a symbol property to deter users from using this would be better.

KhafraDev · 2024-01-09T23:20:41Z

lib/dns-resolver.js

+  get _hostnamesToFallback () {
+    return this.#hostnamesToFallback
+  }


lib/agent.js

lib/pool.js

lib/dns-resolver.js

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

antoinegomez · 2024-01-10T09:58:20Z

Can we add tests that covers the usage within the Agent/Pool?

I did add two tests, one that will check that the cache is incrementing when using the DNS resolver and one that check that no cache is being used if disabled.

Since Agent is not exposing the DNSResolver it is not possible to get it and check the cache, but I will have a look for a solution to see if I can intercept/mock it.

mcollina · 2024-01-10T15:40:18Z

linting is failing

mcollina

We would need to implement some kind of solution of "Happy Eyeballs" similar to what Node Core did.

The more I look at this, the more I'm thinking this should live in Node.js core and not here, but let's chat about it.

mcollina · 2024-01-10T15:42:32Z

README.md

+  * **hints** [`getaddrinfo flags`](https://nodejs.org/api/dns.html#supported-getaddrinfo-flags)
+  * **all** `Boolean` - Default: `false`
+
+* **roundRobinStrategy** `'first' | 'random'` - Default: `'first'`


this should be called scheduling. What does first vs random do?

mcollina · 2024-01-10T15:43:16Z

lib/dns-resolver.js

+    }
+
+    if (this.roundRobinStrategy === 'first') {
+      return cached[0]


if we do not rotate the cache, then it's not round robin.

yes true, I will think of a way to rotate the cache

antoinegomez · 2024-01-10T22:54:24Z

I forgot with my other review but this does need types as well.

first time I add types in non typescript project, is that ok?

https://github.com/nodejs/undici/pull/2552/files#diff-6704654b8ba0b45cd2cc752d380e7c9f70c6c8aac678524044da44446db6c976

mcollina · 2024-01-11T14:29:01Z

index.js

@@ -165,3 +165,6 @@ module.exports.MockClient = MockClient
 module.exports.MockPool = MockPool
 module.exports.MockAgent = MockAgent
 module.exports.mockErrors = mockErrors
+
+const DNSResolver = require('./lib/dns-resolver')


Move this to the top

mcollina · 2024-01-11T14:30:00Z

cc @ShogunPanda

metcoder95 · 2024-01-12T09:51:53Z

lib/dns-resolver.js

+  get [kDnsCacheSize] () {
+    return this.#cache.size ?? 0
+  }


@KhafraDev can you verify its what you were seeking for?

metcoder95 · 2024-01-12T10:00:51Z

lib/dns-resolver.js

+      return cached
+    }
+
+    if (this.#scheduling === 'first') {


Wondering, what are the intentions with the scheduling option?
Its not added within cacheable-lookup as it already caches the records

So with first it is the same behavior as cacheable-lookup and returns the first cached entry from cache.

With random it will return a random item from the cache.

This is a way to achieve some sort of round robin without to much complexity.

ShogunPanda

Minor suggestions, but not blockers.
LGTM!

ShogunPanda · 2024-01-12T13:06:59Z

lib/agent.js

@@ -22,6 +23,18 @@ function defaultFactory (origin, opts) {
    : new Pool(origin, opts)
 }

+function getDNSResolver (options) {
+  if (options?.dnsResolverOptions?.disable) {


I wonder if it would be a better DX to have { dnsResolverOptions: false } instead to completely disable it.
Not a blocker, just a thought.

ShogunPanda · 2024-01-12T13:08:48Z

types/agent.d.ts

@@ -22,6 +23,8 @@ declare namespace Agent {
    maxRedirections?: number;

    interceptors?: { Agent?: readonly Dispatcher.DispatchInterceptor[] } & Pool.Options["interceptors"]
+
+    dnsResolverOptions: DNSResolver.Options;


Similarly, what about having just dnsResolver here?

It was asked before to rename the options to dnsResolverOptions and have a separate one called dnsResolver to specify a custom DNSResolver class.

I changed the disable check on dnsResolver instead of putting it in the options.

So to disable it will be:

new Agent({ dnsResolver: false, })

ShogunPanda · 2024-01-12T15:33:15Z

In regard of Happy Eyeballs, let me clarify our options here.

From what I see now, with this new feature undici will attempt the records in the order node (and thus the DNS server) resolves them). This is a good thing, at least as default.

The next step would be to implement Happy Eyeballs. I would add a specific implementation in undici since we can be explicit and control the sockets we open and thus we can implement the full Happy Eyeballs specification. In other words, in Node I was forced to implement a serialized version of the algorithm (not to break too many things), here we don't have to.

If we do this, we have two options:

Implement Happy Eyeballs on the Dispatcher class.
Implement Happy Eyeballs on the DNSResolver class. Maybe this is too much for it, but it would open the possibility to return to the caller an IP which we already verified via Happy Eyeballs. And/or, potentially, to return the socket we were already able to open.

mcollina · 2024-01-12T16:16:02Z

I think we would need happy eyeballs to enable this by default.

metcoder95 · 2024-01-14T14:55:36Z

The next step would be to implement Happy Eyeballs. I would add a specific implementation in undici since we can be explicit and control the sockets we open and thus we can implement the full Happy Eyeballs specification. In other words, in Node I was forced to implement a serialized version of the algorithm (not to break too many things), here we don't have to.

We should also not forget to add an option to disable it if required

Implement Happy Eyeballs on the Dispatcher class.

I'd like to see if we can keep the Dispatcher class lean, and leverage all this logic to the DNSResolver class, so we can also enable users to customize the behaviour as they need.

Implement Happy Eyeballs on the DNSResolver class. Maybe this is too much for it, but it would open the possibility to return to the caller an IP which we already verified via Happy Eyeballs. And/or, potentially, to return the socket we were already able to open.

Like this pretty much, do you think this can enable features such as #2515?

ShogunPanda · 2024-01-16T16:37:32Z

The next step would be to implement Happy Eyeballs. I would add a specific implementation in undici since we can be explicit and control the sockets we open and thus we can implement the full Happy Eyeballs specification. In other words, in Node I was forced to implement a serialized version of the algorithm (not to break too many things), here we don't have to.

We should also not forget to add an option to disable it if required

Absolutely yes!

Implement Happy Eyeballs on the Dispatcher class.

I'd like to see if we can keep the Dispatcher class lean, and leverage all this logic to the DNSResolver class, so we can also enable users to customize the behaviour as they need.

I'm fine with it. Maybe a separate class used by the the DNSResolver.

Implement Happy Eyeballs on the DNSResolver class. Maybe this is too much for it, but it would open the possibility to return to the caller an IP which we already verified via Happy Eyeballs. And/or, potentially, to return the socket we were already able to open.

Like this pretty much, do you think this can enable features such as #2515?

Probably yes, and I would love it!

mcollina · 2024-01-17T09:49:55Z

types/pool.d.ts

-    interceptors?: { Pool?: readonly Dispatcher.DispatchInterceptor[] } & Client.Options["interceptors"]
+    interceptors?: { Pool?: readonly Dispatcher.DispatchInterceptor[] } & Client.Options["interceptors"];
+
+    dnsResolver?: typeof DNSResolver;


please add tests for this types

antoinegomez · 2024-01-22T23:07:01Z

The next step would be to implement Happy Eyeballs. I would add a specific implementation in undici since we can be explicit and control the sockets we open and thus we can implement the full Happy Eyeballs specification. In other words, in Node I was forced to implement a serialized version of the algorithm (not to break too many things), here we don't have to.

We should also not forget to add an option to disable it if required

Absolutely yes!

Implement Happy Eyeballs on the Dispatcher class.

I'd like to see if we can keep the Dispatcher class lean, and leverage all this logic to the DNSResolver class, so we can also enable users to customize the behaviour as they need.

I'm fine with it. Maybe a separate class used by the the DNSResolver.

Implement Happy Eyeballs on the DNSResolver class. Maybe this is too much for it, but it would open the possibility to return to the caller an IP which we already verified via Happy Eyeballs. And/or, potentially, to return the socket we were already able to open.

Like this pretty much, do you think this can enable features such as #2515?

Probably yes, and I would love it!

I added some tests for types, tried to resolve your comments.

For the Happy Eyeballs I would be glad to do it too if that's ok. Any source or link to existing material?

Since I changed the behavior for this to be disabled by default it can be done in another PR.

metcoder95 · 2024-01-23T09:05:49Z

Yeah, another PR will be good. So far I believe this is the implementation: https://github.com/ShogunPanda/node/blob/ffb326c583c6a23bc94ff4989fe8a06edb92b011/lib/net.js#L1401

and this is the standard that is based on: https://datatracker.ietf.org/doc/html/rfc8305

Are those resources all right @ShogunPanda?

As pointed out, we should seek to stick to the spec as much as we can as we have better ergonomics here to do so.

ShogunPanda · 2024-01-29T15:30:05Z

@metcoder95 Yes, all good links sir. :)

antoinegomez · 2024-02-07T23:52:13Z

Hello @ShogunPanda, I have read the RFC and also the code linked.

Some thoughts.

For now this PR is creating a custom lookup function to cache dns queries.
So in my understanding if happy eye balls is already implemented in node/net module then it seems redundant to implement it again here.

By default the options for lookup are { family: undefined, hints: 32 }
And in node/net the happy eye balls happens when: if (dnsopts.family !== 4 && dnsopts.family !== 6 && !localAddress && autoSelectFamily).
I can see that autoSelectFamily is undefined by default in undici.

In the RFC it suggest that we should not wait for all DNS queries to resolve to start connecting. Starting to query ipv6 then ipv4. Would be a bit more complex to make it work.

I also understand that we could implement the happy eye balls in undici using a similar approach than node/net module but since we are relying on it I am wondering if it is worth it.

ShogunPanda · 2024-02-15T14:12:14Z

In Node.js it is going to use Happy Eyeballs only if using a hostname when doing net.connect, which is not the case in undici since we're using a DNS cache.
We don't have to implement Happy Eyeballs exactly as the RFC mandates, for instance not even Node is doing it.

osuka · 2024-03-25T19:07:23Z

Great to see work on this, we are very interested in the possibility of DNS caching as in kubernetes in particular the DNS lookups become a big issue at scale (even with local caching)

feat: add dns cache nodejs#2440

9413807

mcollina reviewed Dec 28, 2023

View reviewed changes

antoinegomez added 4 commits December 28, 2023 13:13

refactor

5170763

use whole cacheable-dns module instead, use it in Agent and Pool

65cc4fc

use Promise.allSettled

dffb510

Merge branch 'main' into feat-add-dns-cache

d28dc26

antoinegomez added 3 commits December 28, 2023 16:55

fix ipv6

0b0ca32

slight refactor

9a8ea6e

add tests

14e986b

KhafraDev reviewed Dec 29, 2023

View reviewed changes

mcollina requested changes Dec 29, 2023

View reviewed changes

antoinegomez added 2 commits December 29, 2023 21:11

apply comments

007872f

add documentation and global

7940b9a

remove global dns resolver

3db9550

KhafraDev requested changes Jan 8, 2024

View reviewed changes

lib/agent.js Outdated Show resolved Hide resolved

lib/agent.js Outdated Show resolved Hide resolved

lib/dns-resolver.js Outdated Show resolved Hide resolved

lib/dns-resolver.js Show resolved Hide resolved

lib/dns-resolver.js Outdated Show resolved Hide resolved

metcoder95 reviewed Jan 9, 2024

View reviewed changes

lib/agent.js Outdated Show resolved Hide resolved

antoinegomez added 2 commits January 9, 2024 22:17

resolve comments

afd92c8

test agent DNSResolver disabled

30ef32b

KhafraDev requested changes Jan 9, 2024

View reviewed changes

metcoder95 reviewed Jan 10, 2024

View reviewed changes

lib/agent.js Outdated Show resolved Hide resolved

lib/pool.js Outdated Show resolved Hide resolved

lib/dns-resolver.js Outdated Show resolved Hide resolved

antoinegomez and others added 3 commits January 10, 2024 09:54

Update lib/agent.js

67c132f

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

Update lib/dns-resolver.js

4f603a6

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

Update lib/pool.js

3c4487b

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

mcollina requested changes Jan 10, 2024

View reviewed changes

antoinegomez added 2 commits January 10, 2024 22:41

update comments

b192a0e

add types

bbff8e2

mcollina reviewed Jan 11, 2024

View reviewed changes

metcoder95 reviewed Jan 12, 2024

View reviewed changes

ShogunPanda reviewed Jan 12, 2024

View reviewed changes

fix comments

13616d3

mcollina reviewed Jan 17, 2024

View reviewed changes

add types

76e34bb

Merge branch 'main' into feat-add-dns-cache

8ee39e8

antoinegomez requested a review from KhafraDev February 1, 2024 20:25

hjr3 mentioned this pull request Apr 19, 2024

feat: add lookup option to Client, Pool and Agent (#2440) #3139

Closed

7 tasks

		@@ -0,0 +1,443 @@
		// source: https://raw.githubusercontent.com/szmarczak/cacheable-lookup/9e60c9f6e74a003692aec68f3ddad93afe613b8f/source/index.mjs

feat: add dns cache #2440 #2552

Are you sure you want to change the base?

feat: add dns cache #2440 #2552

Conversation

antoinegomez commented Dec 28, 2023 • edited

This relates to #2440

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

antoinegomez commented Dec 28, 2023

mcollina left a comment

Choose a reason for hiding this comment

antoinegomez commented Dec 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoinegomez commented Dec 29, 2023

mcollina commented Dec 30, 2023

codecov-commenter commented Dec 30, 2023 • edited

Codecov Report

metcoder95 left a comment

Choose a reason for hiding this comment

KhafraDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoinegomez commented Jan 10, 2024

mcollina commented Jan 10, 2024

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoinegomez commented Jan 10, 2024

Choose a reason for hiding this comment

mcollina commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShogunPanda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShogunPanda commented Jan 12, 2024

mcollina commented Jan 12, 2024

metcoder95 commented Jan 14, 2024

ShogunPanda commented Jan 16, 2024

Choose a reason for hiding this comment

antoinegomez commented Jan 22, 2024 • edited

metcoder95 commented Jan 23, 2024

ShogunPanda commented Jan 29, 2024

antoinegomez commented Feb 7, 2024 • edited

ShogunPanda commented Feb 15, 2024

osuka commented Mar 25, 2024

antoinegomez commented Dec 28, 2023 •

edited

codecov-commenter commented Dec 30, 2023 •

edited

antoinegomez commented Jan 22, 2024 •

edited

antoinegomez commented Feb 7, 2024 •

edited