dns: implement light dns caching #49560

kshitjj · 2023-09-08T16:37:26Z

Cache DNS results very lightly in node. Strawman: Cache for 1 second, refresh in the background, and delete cache after 1 second.

Fixes: #49394

nodejs-github-bot · 2023-09-08T16:37:30Z

Review requested:

@nodejs/net

mcollina

I think we need a better algorithm for this that avoids concurrent cache misses to trigger multiple dns requests.

lib/dns.js

a lookup for, say, "constructor" or "toString" results in a cache hit Co-authored-by: Ben Noordhuis <[email protected]>

kshitjj · 2023-09-09T11:36:04Z

@mcollina

I think I can implement a locking mechanism that would solve the concurrent cache missing problem.

mcollina · 2023-09-09T11:50:19Z

I think it should cache the lookup operation.

mcollina · 2023-09-09T11:50:26Z

cc @jasnell

lib/dns.js

ShogunPanda · 2023-09-11T08:22:31Z

I think we need a better algorithm for this that avoids concurrent cache misses to trigger multiple dns requests.

I would go for a separate object that tracks all pending requests by hostname (and all other options, to be accurate). This way every time a lookup finishes, all pending request for that host can benefit from the single resolve.

I was thinking to something like:

pending[`${hostname}:${family}:${options]`].push(callback);

// ...

for(const cb of pending[`${hostname}:${family}:${options]`]) {
  process.nextTick(cb, null, result);
}

My only concern here is to have an option to disable such feature. If some hosts are configured with DNS Round Robin, caching results (or return a single lookup response for all concurrent lookups request) might interfere with it.

kshitjj · 2023-09-11T12:54:50Z

@mcollina @ShogunPanda I tested the new algorithm against multiple concurrent DNS requests with different websites, and it works just fine.

I have written the test script now I am tweaking it so it's by the node guideline.

Result for concurrent dns requests:
https://pastebin.com/3AxwzG5g
there were no misses.

ShogunPanda · 2023-09-11T14:09:23Z

Can you also upload the test script?
Also, I think now in the callbacks you're not handling the all case.

kshitjj · 2023-09-11T16:46:28Z

@ShogunPanda I don't think there's an all condition in req.callback, as we are not passing all in callback in any way.

Working on the test script right now

ShogunPanda · 2023-09-11T17:01:04Z

No, I'm talking about having all: true in the options, which changes the callback signature.

See: https://nodejs.org/dist/latest-v20.x/docs/api/dns.html#dnslookuphostname-options-callback

Linkgoron · 2023-09-11T22:02:36Z

lib/dns.js

 function lookup(hostname, options, callback) {
  let hints = 0;
  let family = 0;
  let all = false;
  let verbatim = getDefaultVerbatim();

+  const cacheKey = `${hostname}_${family || 'default'}`;


IMO this (and the code added before the if(!hostname) should move to before const req= new GetAddrInfoReqWrap(). The hostname variable should be used only after it passes validation.

The cache key should probably include if options.all is true or not

I thought if there's a dns key for that domain that means it passed the validation earlier so there was no need for validating the hostname again. I could be wrong though

Also having the all in dns key makes a lot of sense I will add that.

lib/dns.js

mcollina · 2023-09-12T07:06:05Z

Good work! I think this would need some additional tests.

kshitjj · 2023-09-12T10:33:44Z

No, I'm talking about having all: true in the options, which changes the callback signature.

See: https://nodejs.org/dist/latest-v20.x/docs/api/dns.html#dnslookuphostname-options-callback

Oh I am sorry I didn't get you earlier, I will make callbacks for all condition.

lib/dns.js

jasnell · 2023-09-16T16:40:37Z

lib/dns.js

@@ -212,8 +245,22 @@ function lookup(hostname, options, callback) {
    return {};
  }

+  const cacheKey = `${hostname}_${family || 'default'}`;
+  const cachedResult = dnsCache[cacheKey];


A read-through cache approach would be a bit simpler here. essentially something like

cache.get(cacheKey, callback, () => { // If the cache lookup is a miss, this function is called to do the lookup and cache the result of the lookup. });

This commit replaces the timer based approach for purging the expired dns cache with lazy cleanup approach. The DNS cache is now cleaned before adding a new DNS entry and before performing a lookup. This change simplifies the code and eliminates the need for a separate cleanup timer.

H4ad · 2023-09-18T00:33:46Z

lib/dns.js

+  const currentTime = DateNow();
+  for (const key in dnsCache) {
+    if (currentTime - dnsCache[key].timestamp >= 1000) {
+      delete dnsCache[key];


Changing dnsCache to a Map could be a better/faster structure to use in this case, delete is usually slow.

Refs:

https://stackoverflow.com/a/44008788

https://github.com/RafaelGSS/nodejs-bench-operations/blob/9689895d6cc8e91b4598f1995c9e11eedf5d3a72/v20/RESULTS-v20_0_0.md#deleting-properties

If we want to keep the object rather than a Map, just do a dnsCache[key] = undefined.

andymac4182 · 2023-09-30T03:25:46Z

Any chance of this landing before V21?

We have had to build our own version of DNS caching to solve this. It heavily impacts the scaleability of our system.

kshitjj · 2023-10-02T17:57:40Z

@andymac4182

I am sorry for the inconvenience, I will try my best to close this issue within this week.

ShogunPanda · 2023-10-03T13:46:40Z

No worries, if this can't land in Node v21.0.0 as semver-major, we can modify it to be under a flag and therefore include it in v21.x.0 as semver-minor.

andymac4182 · 2023-10-04T05:41:41Z

That would be great to be under a flag and shipped in V20 👍

@ShogunPanda Not sure if you got your versions miss written but V20.9 would be awesome to land this in 😉

kshitjj · 2023-10-06T17:10:17Z

test/parallel/test-dns-set-default-order, test-dns-default-verbatim-false, test-dns-default-verbatim-true is failing because getaddrinfo is being called twice instead of three as DNS cache is being used, so it isn't being looked up through getaddrinfo

The only way to pass the test is by changing the test

Rest of the tests are passing.

dns: implement light dns caching

db4dde6

nodejs-github-bot added dns Issues and PRs related to the dns subsystem. needs-ci PRs that need a full CI run. labels Sep 8, 2023

mcollina reviewed Sep 8, 2023

View reviewed changes

bnoordhuis reviewed Sep 8, 2023

View reviewed changes

lib/dns.js Outdated Show resolved Hide resolved

dns: add default value to dnsCache

b5a7580

a lookup for, say, "constructor" or "toString" results in a cache hit Co-authored-by: Ben Noordhuis <[email protected]>

ShogunPanda reviewed Sep 11, 2023

View reviewed changes

lib/dns.js Outdated Show resolved Hide resolved

kshitjj added 3 commits September 11, 2023 18:10

Merge branch 'main' into async-dns-resolution

5d33b3f

fix: dns cache algorithm

3038e59

Merge branch 'nodejs:main' into async-dns-resolution

71fc2cc

kshitjj added 2 commits September 11, 2023 19:05

fix: removed hasOwnProperty method

b4f1025

fix: removed hasOwnProperty method

c32745b

kshitjj force-pushed the async-dns-resolution branch from 5b28d66 to c32745b Compare September 11, 2023 17:05

Linkgoron reviewed Sep 11, 2023

View reviewed changes

lib/dns.js Outdated Show resolved Hide resolved

kshitjj added 4 commits September 15, 2023 23:25

Merge branch 'main' into async-dns-resolution

9fff8f9

fix: linter

f731c16

fix: unref timer

80c60eb

Refactor: DNS cache cleanup scheduling

83586ef

jasnell reviewed Sep 16, 2023

View reviewed changes

lib/dns.js Outdated Show resolved Hide resolved

jasnell reviewed Sep 16, 2023

View reviewed changes

kshitjj force-pushed the async-dns-resolution branch from 49c4df3 to 090a1a0 Compare September 16, 2023 17:59

H4ad reviewed Sep 18, 2023

View reviewed changes

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 19, 2023

performance: changed deleting method

514f8e2

kshitjj added 2 commits October 7, 2023 16:12

dns: fix test/test-tls-connect-address-family.js

1c6a9f6

Merge branch 'nodejs:main' into async-dns-resolution

d217c14

kshitjj closed this Jul 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: implement light dns caching #49560

dns: implement light dns caching #49560

kshitjj commented Sep 8, 2023 •

edited

Loading

nodejs-github-bot commented Sep 8, 2023

mcollina left a comment

kshitjj commented Sep 9, 2023

mcollina commented Sep 9, 2023

mcollina commented Sep 9, 2023

ShogunPanda commented Sep 11, 2023 •

edited

Loading

kshitjj commented Sep 11, 2023 •

edited

Loading

ShogunPanda commented Sep 11, 2023

kshitjj commented Sep 11, 2023 •

edited

Loading

ShogunPanda commented Sep 11, 2023

Linkgoron Sep 11, 2023 •

edited

Loading

Linkgoron Sep 11, 2023

kshitjj Sep 12, 2023

mcollina commented Sep 12, 2023

kshitjj commented Sep 12, 2023

jasnell Sep 16, 2023

H4ad Sep 18, 2023

ShogunPanda Sep 19, 2023

andymac4182 commented Sep 30, 2023

kshitjj commented Oct 2, 2023

ShogunPanda commented Oct 3, 2023

andymac4182 commented Oct 4, 2023 •

edited

Loading

kshitjj commented Oct 6, 2023 •

edited

Loading

dns: implement light dns caching #49560

dns: implement light dns caching #49560

Conversation

kshitjj commented Sep 8, 2023 • edited Loading

nodejs-github-bot commented Sep 8, 2023

mcollina left a comment

Choose a reason for hiding this comment

kshitjj commented Sep 9, 2023

mcollina commented Sep 9, 2023

mcollina commented Sep 9, 2023

ShogunPanda commented Sep 11, 2023 • edited Loading

kshitjj commented Sep 11, 2023 • edited Loading

ShogunPanda commented Sep 11, 2023

kshitjj commented Sep 11, 2023 • edited Loading

ShogunPanda commented Sep 11, 2023

Linkgoron Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Linkgoron Sep 11, 2023

Choose a reason for hiding this comment

kshitjj Sep 12, 2023

Choose a reason for hiding this comment

mcollina commented Sep 12, 2023

kshitjj commented Sep 12, 2023

jasnell Sep 16, 2023

Choose a reason for hiding this comment

H4ad Sep 18, 2023

Choose a reason for hiding this comment

ShogunPanda Sep 19, 2023

Choose a reason for hiding this comment

andymac4182 commented Sep 30, 2023

kshitjj commented Oct 2, 2023

ShogunPanda commented Oct 3, 2023

andymac4182 commented Oct 4, 2023 • edited Loading

kshitjj commented Oct 6, 2023 • edited Loading

kshitjj commented Sep 8, 2023 •

edited

Loading

ShogunPanda commented Sep 11, 2023 •

edited

Loading

kshitjj commented Sep 11, 2023 •

edited

Loading

kshitjj commented Sep 11, 2023 •

edited

Loading

Linkgoron Sep 11, 2023 •

edited

Loading

andymac4182 commented Oct 4, 2023 •

edited

Loading

kshitjj commented Oct 6, 2023 •

edited

Loading