From 610a0c3bd602a96e143603d44e86f88db7d78bc2 Mon Sep 17 00:00:00 2001 From: Orgad Shaneh Date: Sun, 5 Apr 2026 13:31:08 +0300 Subject: [PATCH] dns: coalesce identical concurrent lookup() requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When multiple callers issue dns.lookup() for the same (hostname, family, hints, order) concurrently, only one getaddrinfo call is now dispatched to the libuv threadpool. All callers share the result. getaddrinfo is a blocking call that runs on the libuv threadpool (capped at 4 threads by default, with a slow I/O concurrency limit of 2). When DNS resolution is slow - e.g. ~10-20 s per call due to a misbehaving resolver - identical requests queue behind each other, causing timeouts that grow linearly with the number of concurrent callers: Before: 100 parallel lookup('host') -> 50 batches × 10 s = 500+ s After: 100 parallel lookup('host') -> 1 getaddrinfo call = ~10 s This is particularly severe on WSL, where the DNS relay rewrites QNAMEs in responses (appending the search domain), causing glibc to discard them as non-matching and wait for a 5s timeout per retry. The coalescing is keyed on (hostname, family, hints, order) so lookups with different options still get separate getaddrinfo calls. Each caller independently post-processes the shared raw result (applying the 'all' flag, constructing address objects, etc.). Signed-off-by: Orgad Shaneh PR-URL: https://github.com/nodejs/node/pull/62599 Fixes: https://github.com/nodejs/node/issues/62503 --- lib/dns.js | 89 +++++--- lib/internal/dns/promises.js | 128 +++++------ test/parallel/test-dns-lookup-coalesce.js | 262 ++++++++++++++++++++++ test/parallel/test-dns-lookup-promises.js | 6 - 4 files changed, 380 insertions(+), 105 deletions(-) create mode 100644 test/parallel/test-dns-lookup-coalesce.js diff --git a/lib/dns.js b/lib/dns.js index d651f5ea0a2685..0b61586596b4ee 100644 --- a/lib/dns.js +++ b/lib/dns.js @@ -22,14 +22,18 @@ 'use strict'; const { + Array, + ArrayPrototypePush, ObjectDefineProperties, ObjectDefineProperty, + SafeMap, Symbol, } = primordials; const cares = internalBinding('cares_wrap'); const { isIP } = require('internal/net'); const { customPromisifyArgs } = require('internal/util'); +const { AsyncResource } = require('async_hooks'); const { DNSException, codes: { @@ -105,36 +109,12 @@ const { let promises = null; // Lazy loaded -function onlookup(err, addresses) { - if (err) { - return this.callback(new DNSException(err, 'getaddrinfo', this.hostname)); - } - this.callback(null, addresses[0], this.family || isIP(addresses[0])); - if (this[kPerfHooksDnsLookupContext] && hasObserver('dns')) { - stopPerf(this, kPerfHooksDnsLookupContext, { detail: { addresses } }); - } -} - - -function onlookupall(err, addresses) { - if (err) { - return this.callback(new DNSException(err, 'getaddrinfo', this.hostname)); - } - - const family = this.family; - for (let i = 0; i < addresses.length; i++) { - const addr = addresses[i]; - addresses[i] = { - address: addr, - family: family || isIP(addr), - }; - } - - this.callback(null, addresses); - if (this[kPerfHooksDnsLookupContext] && hasObserver('dns')) { - stopPerf(this, kPerfHooksDnsLookupContext, { detail: { addresses } }); - } -} +// Map of in-flight getaddrinfo requests for lookup coalescing. +// Key: `${hostname}\0${family}\0${hints}\0${order}` +// Value: Array<{ callback, all }> +// When multiple callers request the same lookup concurrently, only one +// getaddrinfo call is dispatched to the libuv threadpool. +const inflightLookups = new SafeMap(); // Easy DNS A/AAAA look up @@ -213,12 +193,6 @@ function lookup(hostname, options, callback) { return {}; } - const req = new GetAddrInfoReqWrap(); - req.callback = callback; - req.family = family; - req.hostname = hostname; - req.oncomplete = all ? onlookupall : onlookup; - let order = DNS_ORDER_VERBATIM; if (dnsOrder === 'ipv4first') { @@ -227,10 +201,53 @@ function lookup(hostname, options, callback) { order = DNS_ORDER_IPV6_FIRST; } + const key = `${hostname}\0${family}\0${hints}\0${order}`; + const waiter = { callback: AsyncResource.bind(callback), all }; + + let inflight = inflightLookups.get(key); + if (inflight) { + // Piggyback on existing in-flight request. + ArrayPrototypePush(inflight, waiter); + return {}; + } + + // First request for this key - dispatch to libuv. + inflight = [waiter]; + inflightLookups.set(key, inflight); + + const req = new GetAddrInfoReqWrap(); + req.family = family; + req.hostname = hostname; + req.oncomplete = function(errCode, addresses) { + inflightLookups.delete(key); + const waiters = inflight; + for (let i = 0; i < waiters.length; i++) { + const w = waiters[i]; + if (errCode) { + w.callback(new DNSException(errCode, 'getaddrinfo', hostname)); + } else if (w.all) { + const result = new Array(addresses.length); + for (let j = 0; j < addresses.length; j++) { + result[j] = { + address: addresses[j], + family: family || isIP(addresses[j]), + }; + } + w.callback(null, result); + } else { + w.callback(null, addresses[0], family || isIP(addresses[0])); + } + } + if (this[kPerfHooksDnsLookupContext] && hasObserver('dns')) { + stopPerf(this, kPerfHooksDnsLookupContext, { detail: { addresses } }); + } + }; + const err = cares.getaddrinfo( req, hostname, family, hints, order, ); if (err) { + inflightLookups.delete(key); process.nextTick(callback, new DNSException(err, 'getaddrinfo', hostname)); return {}; } diff --git a/lib/internal/dns/promises.js b/lib/internal/dns/promises.js index f7ee8fd25423ca..c0ff076cd780c4 100644 --- a/lib/internal/dns/promises.js +++ b/lib/internal/dns/promises.js @@ -1,9 +1,13 @@ 'use strict'; const { + Array, ArrayPrototypeMap, FunctionPrototypeCall, ObjectDefineProperty, Promise, + PromiseReject, + PromiseResolve, + SafeMap, Symbol, } = primordials; @@ -82,41 +86,14 @@ const { stopPerf, } = require('internal/perf/observe'); -function onlookup(err, addresses) { - if (err) { - this.reject(new DNSException(err, 'getaddrinfo', this.hostname)); - return; - } - - const family = this.family || isIP(addresses[0]); - this.resolve({ address: addresses[0], family }); - if (this[kPerfHooksDnsLookupContext] && hasObserver('dns')) { - stopPerf(this, kPerfHooksDnsLookupContext, { detail: { addresses } }); - } -} - -function onlookupall(err, addresses) { - if (err) { - this.reject(new DNSException(err, 'getaddrinfo', this.hostname)); - return; - } - - const family = this.family; - - for (let i = 0; i < addresses.length; i++) { - const address = addresses[i]; - - addresses[i] = { - address, - family: family || isIP(addresses[i]), - }; - } - - this.resolve(addresses); - if (this[kPerfHooksDnsLookupContext] && hasObserver('dns')) { - stopPerf(this, kPerfHooksDnsLookupContext, { detail: { addresses } }); - } -} +// Map of in-flight getaddrinfo requests for lookup coalescing. +// Key: `${hostname}\0${family}\0${hints}\0${order}` +// Value: Promise<{ err: number|null, addresses: string[]|null }> +// When multiple callers request the same lookup concurrently, only one +// getaddrinfo call is dispatched to the libuv threadpool. All callers +// share the result, avoiding threadpool starvation under sustained DNS +// failure where each getaddrinfo blocks for many seconds. +const inflightLookups = new SafeMap(); /** * Creates a promise that resolves with the IP address of the given hostname. @@ -132,41 +109,48 @@ function onlookupall(err, addresses) { * @property {0 | 4 | 6} family - The IP address type. 4 for IPv4 or 6 for IPv6, or 0 (for both). */ function createLookupPromise(family, hostname, all, hints, dnsOrder) { - return new Promise((resolve, reject) => { - if (!hostname) { - reject(new ERR_INVALID_ARG_VALUE('hostname', hostname, - 'must be a non-empty string')); - return; - } + if (!hostname) { + return PromiseReject( + new ERR_INVALID_ARG_VALUE('hostname', hostname, + 'must be a non-empty string')); + } - const matchedFamily = isIP(hostname); + const matchedFamily = isIP(hostname); + if (matchedFamily !== 0) { + const result = { address: hostname, family: matchedFamily }; + return PromiseResolve(all ? [result] : result); + } - if (matchedFamily !== 0) { - const result = { address: hostname, family: matchedFamily }; - resolve(all ? [result] : result); - return; - } + let order = DNS_ORDER_VERBATIM; + if (dnsOrder === 'ipv4first') { + order = DNS_ORDER_IPV4_FIRST; + } else if (dnsOrder === 'ipv6first') { + order = DNS_ORDER_IPV6_FIRST; + } - const req = new GetAddrInfoReqWrap(); + const key = `${hostname}\0${family}\0${hints}\0${order}`; + let rawPromise = inflightLookups.get(key); + if (!rawPromise) { + let resolveRaw; + rawPromise = new Promise((resolve) => { resolveRaw = resolve; }); + inflightLookups.set(key, rawPromise); + + const req = new GetAddrInfoReqWrap(); req.family = family; req.hostname = hostname; - req.oncomplete = all ? onlookupall : onlookup; - req.resolve = resolve; - req.reject = reject; - - let order = DNS_ORDER_VERBATIM; - - if (dnsOrder === 'ipv4first') { - order = DNS_ORDER_IPV4_FIRST; - } else if (dnsOrder === 'ipv6first') { - order = DNS_ORDER_IPV6_FIRST; - } - - const err = getaddrinfo(req, hostname, family, hints, order); + req.oncomplete = function(errCode, addresses) { + inflightLookups.delete(key); + resolveRaw({ err: errCode, addresses }); + if (this[kPerfHooksDnsLookupContext] && hasObserver('dns')) { + stopPerf(this, kPerfHooksDnsLookupContext, { detail: { addresses } }); + } + }; - if (err) { - reject(new DNSException(err, 'getaddrinfo', hostname)); + const errCode = getaddrinfo(req, hostname, family, hints, order); + if (errCode) { + inflightLookups.delete(key); + resolveRaw({ err: errCode, addresses: null }); } else if (hasObserver('dns')) { const detail = { hostname, @@ -177,6 +161,24 @@ function createLookupPromise(family, hostname, all, hints, dnsOrder) { }; startPerf(req, kPerfHooksDnsLookupContext, { type: 'dns', name: 'lookup', detail }); } + } + + // Each caller post-processes the shared raw result independently. + return rawPromise.then(({ err, addresses }) => { + if (err) { + throw new DNSException(err, 'getaddrinfo', hostname); + } + if (all) { + const result = new Array(addresses.length); + for (let i = 0; i < addresses.length; i++) { + result[i] = { + address: addresses[i], + family: family || isIP(addresses[i]), + }; + } + return result; + } + return { address: addresses[0], family: family || isIP(addresses[0]) }; }); } diff --git a/test/parallel/test-dns-lookup-coalesce.js b/test/parallel/test-dns-lookup-coalesce.js new file mode 100644 index 00000000000000..3b488e6111dfef --- /dev/null +++ b/test/parallel/test-dns-lookup-coalesce.js @@ -0,0 +1,262 @@ +// Flags: --expose-internals +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const { AsyncLocalStorage } = require('async_hooks'); +const { internalBinding } = require('internal/test/binding'); +const cares = internalBinding('cares_wrap'); + +// Track getaddrinfo dispatch count and hold oncomplete callbacks so we can +// resolve them manually, simulating async libuv threadpool behaviour. +const pending = []; +let dispatchCount = 0; +cares.getaddrinfo = (req) => { + dispatchCount++; + pending.push(req); + return 0; // Success - request is now "in flight" +}; + +const dns = require('dns'); +const dnsPromises = dns.promises; + +// ---------- promises API ---------- + +// Identical concurrent lookups should coalesce into a single getaddrinfo call. +async function testPromisesCoalesceIdentical() { + dispatchCount = 0; + pending.length = 0; + + const p1 = dnsPromises.lookup('coalesce-test.example'); + const p2 = dnsPromises.lookup('coalesce-test.example'); + const p3 = dnsPromises.lookup('coalesce-test.example'); + + // Only one getaddrinfo should have been dispatched. + assert.strictEqual(dispatchCount, 1); + assert.strictEqual(pending.length, 1); + + // Complete the single underlying request. + pending[0].oncomplete(null, ['1.2.3.4']); + + const [r1, r2, r3] = await Promise.all([p1, p2, p3]); + assert.deepStrictEqual(r1, { address: '1.2.3.4', family: 4 }); + assert.deepStrictEqual(r2, { address: '1.2.3.4', family: 4 }); + assert.deepStrictEqual(r3, { address: '1.2.3.4', family: 4 }); +} + +// Lookups with different options must NOT coalesce. +async function testPromisesNoCoalesceDifferentOptions() { + dispatchCount = 0; + pending.length = 0; + + const p1 = dnsPromises.lookup('diff-opts.example'); + const p2 = dnsPromises.lookup('diff-opts.example', { family: 4 }); + const p3 = dnsPromises.lookup('other-host.example'); + + // Lookups with different keys must not coalesce + assert.strictEqual(dispatchCount, 3); + + // Complete all three. + pending[0].oncomplete(null, ['1.1.1.1']); + pending[1].oncomplete(null, ['2.2.2.2']); + pending[2].oncomplete(null, ['3.3.3.3']); + + const [r1, r2, r3] = await Promise.all([p1, p2, p3]); + assert.deepStrictEqual(r1, { address: '1.1.1.1', family: 4 }); + assert.deepStrictEqual(r2, { address: '2.2.2.2', family: 4 }); + assert.deepStrictEqual(r3, { address: '3.3.3.3', family: 4 }); +} + +// Error should propagate to all coalesced callers. +async function testPromisesCoalesceError() { + dispatchCount = 0; + pending.length = 0; + + const p1 = dnsPromises.lookup('fail-coalesce.example'); + const p2 = dnsPromises.lookup('fail-coalesce.example'); + + assert.strictEqual(dispatchCount, 1); + + pending[0].oncomplete(internalBinding('uv').UV_EAI_AGAIN); + + const expected = { code: 'EAI_AGAIN', syscall: 'getaddrinfo' }; + await assert.rejects(p1, expected); + await assert.rejects(p2, expected); +} + +// Coalesced lookups with all:true and all:false on same key should each get +// correctly post-processed results. +async function testPromisesCoalesceMixedAll() { + dispatchCount = 0; + pending.length = 0; + + const p1 = dnsPromises.lookup('mixed.example'); + const p2 = dnsPromises.lookup('mixed.example', { all: true }); + + // all:true vs all:false share the same (hostname, family=0, hints=0, order) + // key, so they must coalesce. + assert.strictEqual(dispatchCount, 1); + + pending[0].oncomplete(null, ['10.0.0.1', '10.0.0.2']); + + const [r1, r2] = await Promise.all([p1, p2]); + // all:false → first address only + assert.deepStrictEqual(r1, { address: '10.0.0.1', family: 4 }); + // all:true → array of all addresses + assert.deepStrictEqual(r2, [ + { address: '10.0.0.1', family: 4 }, + { address: '10.0.0.2', family: 4 }, + ]); +} + +// After a coalesced batch settles, a new lookup for the same key should +// dispatch a fresh getaddrinfo (the inflight entry must be cleaned up). +async function testPromisesCoalesceCleanup() { + dispatchCount = 0; + pending.length = 0; + + const p1 = dnsPromises.lookup('cleanup.example'); + assert.strictEqual(dispatchCount, 1); + pending[0].oncomplete(null, ['5.5.5.5']); + await p1; + + // Second wave — must dispatch a new getaddrinfo. + const p2 = dnsPromises.lookup('cleanup.example'); + assert.strictEqual(dispatchCount, 2); + pending[1].oncomplete(null, ['6.6.6.6']); + const r2 = await p2; + assert.deepStrictEqual(r2, { address: '6.6.6.6', family: 4 }); +} + +// ---------- callback API ---------- + +// Identical concurrent callback-based lookups should coalesce. +async function testCallbackCoalesceIdentical() { + dispatchCount = 0; + pending.length = 0; + + const results = []; + const done = new Promise((resolve) => { + let remaining = 3; + function cb(err, address, family) { + results.push({ err, address, family }); + if (--remaining === 0) resolve(); + } + dns.lookup('cb-coalesce.example', cb); + dns.lookup('cb-coalesce.example', cb); + dns.lookup('cb-coalesce.example', cb); + }); + + // Callback lookups must coalesce + assert.strictEqual(dispatchCount, 1); + + pending[0].oncomplete(null, ['7.7.7.7']); + await done; + + for (const r of results) { + assert.strictEqual(r.err, null); + assert.strictEqual(r.address, '7.7.7.7'); + assert.strictEqual(r.family, 4); + } +} + +// Callback error propagation to all coalesced callers. +async function testCallbackCoalesceError() { + dispatchCount = 0; + pending.length = 0; + + const results = []; + const done = new Promise((resolve) => { + let remaining = 2; + function cb(err) { + results.push(err); + if (--remaining === 0) resolve(); + } + dns.lookup('cb-fail.example', cb); + dns.lookup('cb-fail.example', cb); + }); + + assert.strictEqual(dispatchCount, 1); + + pending[0].oncomplete(internalBinding('uv').UV_EAI_AGAIN); + await done; + + for (const err of results) { + assert.strictEqual(err.code, 'EAI_AGAIN'); + assert.strictEqual(err.syscall, 'getaddrinfo'); + } +} + +// ---------- async context (AsyncLocalStorage) ---------- + +// Callback API: each coalesced caller must retain its own async context. +async function testCallbackAsyncContext() { + dispatchCount = 0; + pending.length = 0; + + const als = new AsyncLocalStorage(); + const results = await new Promise((resolve) => { + const out = []; + let remaining = 3; + function done() { if (--remaining === 0) resolve(out); } + + als.run('ctx-A', () => { + dns.lookup('async-ctx.example', () => { + out.push({ id: 'A', ctx: als.getStore() }); + done(); + }); + }); + als.run('ctx-B', () => { + dns.lookup('async-ctx.example', () => { + out.push({ id: 'B', ctx: als.getStore() }); + done(); + }); + }); + als.run('ctx-C', () => { + dns.lookup('async-ctx.example', () => { + out.push({ id: 'C', ctx: als.getStore() }); + done(); + }); + }); + + assert.strictEqual(dispatchCount, 1); + pending[0].oncomplete(null, ['1.2.3.4']); + }); + + for (const r of results) { + assert.strictEqual(r.ctx, `ctx-${r.id}`, + `callback ${r.id} lost its async context`); + } +} + +// Promises API: each coalesced caller must retain its own async context. +async function testPromisesAsyncContext() { + dispatchCount = 0; + pending.length = 0; + + const als = new AsyncLocalStorage(); + const p1 = als.run('ctx-A', () => + dnsPromises.lookup('async-ctx-p.example').then( + (r) => ({ ...r, ctx: als.getStore() }))); + const p2 = als.run('ctx-B', () => + dnsPromises.lookup('async-ctx-p.example').then( + (r) => ({ ...r, ctx: als.getStore() }))); + + assert.strictEqual(dispatchCount, 1); + pending[0].oncomplete(null, ['2.3.4.5']); + + const [r1, r2] = await Promise.all([p1, p2]); + assert.strictEqual(r1.ctx, 'ctx-A'); + assert.strictEqual(r2.ctx, 'ctx-B'); +} + +(async () => { + await testPromisesCoalesceIdentical(); + await testPromisesNoCoalesceDifferentOptions(); + await testPromisesCoalesceError(); + await testPromisesCoalesceMixedAll(); + await testPromisesCoalesceCleanup(); + await testCallbackCoalesceIdentical(); + await testCallbackCoalesceError(); + await testCallbackAsyncContext(); + await testPromisesAsyncContext(); +})().then(common.mustCall()); diff --git a/test/parallel/test-dns-lookup-promises.js b/test/parallel/test-dns-lookup-promises.js index 18e148e8a4a54c..a9ddfa88eb981b 100644 --- a/test/parallel/test-dns-lookup-promises.js +++ b/test/parallel/test-dns-lookup-promises.js @@ -14,18 +14,12 @@ const dnsPromises = require('dns').promises; function getaddrinfoNegative() { return common.mustCall(function getaddrinfoNegativeHandler(req) { - const originalReject = req.reject; - req.resolve = common.mustNotCall(); - req.reject = common.mustCall(originalReject); req.oncomplete(internalBinding('uv').UV_ENOMEM); }); } function getaddrinfoPositive(addresses) { return common.mustCall(function getaddrinfo_positive(req) { - const originalResolve = req.resolve; - req.reject = common.mustNotCall(); - req.resolve = common.mustCall(originalResolve); req.oncomplete(null, addresses); }); }