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); }); }