From d7f3facce4b857f374de5daa2f2e425cb63103ab Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 18 Aug 2026 18:44:54 +0000 Subject: [PATCH 1/4] perf(fetch): skip empty RequestInit work and drop idle-socket timer floor The Request constructor treated every init as non-empty because undici's priority member defaults to "auto". That forced the header clone/clear path on every fetch(url) / new Request(url). Idle keep-alive reuse was deferred with setTimeout(0), adding ~1ms of latency per sequential fetch. Use a ref'd setImmediate so validation still runs after poll without the timer floor or the unref'd-Immediate poll stall. Assisted by Cursor Signed-off-by: Yagiz Nizipli --- benchmarks/fetch/sequential-keepalive.mjs | 48 +++++++++++++++++++++++ lib/dispatcher/client-h1.js | 16 ++++++-- lib/web/fetch/request.js | 37 ++++++++++++++++- test/fetch/request.js | 37 +++++++++++++++++ test/node-test/keep-alive-reuse.js | 48 ++++++++++++++++++++++- 5 files changed, 179 insertions(+), 7 deletions(-) create mode 100644 benchmarks/fetch/sequential-keepalive.mjs diff --git a/benchmarks/fetch/sequential-keepalive.mjs b/benchmarks/fetch/sequential-keepalive.mjs new file mode 100644 index 00000000000..1a21ad36535 --- /dev/null +++ b/benchmarks/fetch/sequential-keepalive.mjs @@ -0,0 +1,48 @@ +'use strict' + +import { createServer } from 'node:http' +import { Agent, fetch } from '../../index.js' + +const ITERATIONS = Number(process.env.SAMPLES ?? 2000) +const WARMUP = 300 + +const server = createServer((req, res) => { + res.writeHead(200, { 'content-type': 'application/json' }) + res.end('{"ok":1}') +}) + +server.keepAliveTimeout = 65_000 + +await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + +const { port } = server.address() +const url = `http://127.0.0.1:${port}/` +const agent = new Agent({ + keepAliveTimeout: 60_000, + connections: 1, + pipelining: 1 +}) + +for (let i = 0; i < WARMUP; i++) { + await (await fetch(url, { dispatcher: agent })).text() +} + +const times = new Array(ITERATIONS) +for (let i = 0; i < ITERATIONS; i++) { + const t0 = process.hrtime.bigint() + await (await fetch(url, { dispatcher: agent })).text() + times[i] = Number(process.hrtime.bigint() - t0) +} + +times.sort((a, b) => a - b) +const pct = (p) => times[Math.min(ITERATIONS - 1, Math.floor(ITERATIONS * p))] / 1e6 + +console.log(JSON.stringify({ + iterations: ITERATIONS, + p50_ms: Number(pct(0.5).toFixed(3)), + p90_ms: Number(pct(0.9).toFixed(3)), + p99_ms: Number(pct(0.99).toFixed(3)) +})) + +await agent.close() +server.close() diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js index 9f6f17c1579..f06ca74bfe5 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -1052,7 +1052,7 @@ function onSocketClose () { function clearIdleSocketValidation (socket) { if (socket[kIdleSocketValidationTimeout]) { - clearTimeout(socket[kIdleSocketValidationTimeout]) + clearImmediate(socket[kIdleSocketValidationTimeout]) socket[kIdleSocketValidationTimeout] = null } @@ -1061,15 +1061,23 @@ function clearIdleSocketValidation (socket) { function scheduleIdleSocketValidation (client, socket) { socket[kIdleSocketValidation] = 1 - socket[kIdleSocketValidationTimeout] = setTimeout(() => { + // Yield to the check phase (after poll) so unsolicited bytes / FIN / RST + // already pending on this idle keep-alive socket are processed before the + // next request is written (GHSA-35p6-xmwp-9g52). + // + // setTimeout(0) pays Node's ~1ms timer floor on every sequential reuse + // (#5493). setImmediate avoids that, but an *unref'd* Immediate lets poll + // block for ~500ms when the event loop is otherwise idle (#5600 / #5606). + // A ref'd Immediate both keeps the pending request alive and makes poll + // return immediately — the hybrid those issues asked for. + socket[kIdleSocketValidationTimeout] = setImmediate(() => { socket[kIdleSocketValidationTimeout] = null socket[kIdleSocketValidation] = 2 if (client[kSocket] === socket && !socket.destroyed) { client[kResume]() } - }, 0) - socket[kIdleSocketValidationTimeout].unref?.() + }) } /** diff --git a/lib/web/fetch/request.js b/lib/web/fetch/request.js index dbe809c289c..bd12987891c 100644 --- a/lib/web/fetch/request.js +++ b/lib/web/fetch/request.js @@ -30,6 +30,39 @@ const { getMaxListeners, setMaxListeners, defaultMaxListeners } = require('node: const kAbortController = Symbol('abortController') +/** + * Fetch's "If init is not empty" check is based on user-specified + * RequestInit members, not dictionary defaults. undici's `priority` + * member defaults to `"auto"`, so `Object.keys(convertedInit)` is + * never empty and would otherwise force the expensive header + * clone/clear/re-append path on every `fetch(url)` / `new Request(url)`. + * + * Matches the spec / Servo / Chromium "any members present" test. + * + * @param {object | null | undefined} init + * @returns {boolean} + */ +function requestInitHasUserMembers (init) { + return init != null && ( + init.method !== undefined || + init.headers !== undefined || + init.body !== undefined || + init.referrer !== undefined || + init.referrerPolicy !== undefined || + init.mode !== undefined || + init.credentials !== undefined || + init.cache !== undefined || + init.redirect !== undefined || + init.integrity !== undefined || + init.keepalive !== undefined || + init.signal !== undefined || + 'window' in init || + init.duplex !== undefined || + init.dispatcher !== undefined || + init.priority !== undefined + ) +} + const requestFinalizer = new FinalizationRegistry(({ signal, abort }) => { signal.removeEventListener('abort', abort) }) @@ -116,6 +149,8 @@ class Request { webidl.argumentLengthCheck(arguments, 1, prefix) input = webidl.converters.RequestInfo(input) + // Capture this before WebIDL conversion fills dictionary defaults. + const initHasKey = requestInitHasUserMembers(init) init = webidl.converters.RequestInit(init) // 1. Let request be null. @@ -241,8 +276,6 @@ class Request { urlList: [...request.urlList] }) - const initHasKey = Object.keys(init).length !== 0 - // 13. If init is not empty, then: if (initHasKey) { // 1. If request’s mode is "navigate", then set it to "same-origin". diff --git a/test/fetch/request.js b/test/fetch/request.js index 51e92b5c061..964e1808dfa 100644 --- a/test/fetch/request.js +++ b/test/fetch/request.js @@ -420,6 +420,43 @@ test('request.referrer', (t) => { } }) +// Dictionary defaults (undici's priority: "auto") must not make an omitted +// RequestInit count as non-empty. That used to force a header clone/clear +// on every fetch(url) / new Request(url) and reset copied request state. +test('omitted RequestInit is empty even though priority defaults to auto', (t) => { + const parent = new Request('http://localhost/a', { + method: 'POST', + body: 'hi', + referrerPolicy: 'unsafe-url', + headers: { 'x-a': '1' } + }) + + const copied = new Request(parent) + t.assert.strictEqual(copied.method, 'POST') + t.assert.strictEqual(copied.referrerPolicy, 'unsafe-url') + t.assert.deepStrictEqual([...copied.headers], [ + ['content-type', 'text/plain;charset=UTF-8'], + ['x-a', '1'] + ]) + + copied.headers.append('x-b', '2') + t.assert.deepStrictEqual([...parent.headers], [ + ['content-type', 'text/plain;charset=UTF-8'], + ['x-a', '1'] + ]) +}) + +test('explicit RequestInit members still count as non-empty', (t) => { + const parent = new Request('http://localhost/a', { + referrerPolicy: 'unsafe-url', + headers: { 'x-a': '1' } + }) + + const copied = new Request(parent, { priority: 'high' }) + t.assert.strictEqual(copied.referrerPolicy, '') + t.assert.deepStrictEqual([...copied.headers], [['x-a', '1']]) +}) + // https://github.com/nodejs/undici/issues/2445 test('Clone the set-cookie header when Request is passed as the first parameter and no header is passed.', (t) => { const request = new Request('http://localhost', { headers: { 'set-cookie': 'A' } }) diff --git a/test/node-test/keep-alive-reuse.js b/test/node-test/keep-alive-reuse.js index 5a92f6003c0..4158a43993f 100644 --- a/test/node-test/keep-alive-reuse.js +++ b/test/node-test/keep-alive-reuse.js @@ -4,7 +4,7 @@ const { test } = require('node:test') const assert = require('node:assert') const { createServer } = require('node:http') const { once } = require('node:events') -const { Pool } = require('../..') +const { Agent, Pool, fetch } = require('../..') // Regression for #5600 / #5606: // Reusing an idle keep-alive socket must not stall behind the poll phase. @@ -66,3 +66,49 @@ test('reusing an idle keep-alive socket must not stall', { timeout: 1000 }, asyn assert.strictEqual(connections, 1, 'keep-alive socket must be reused') } }) + +test('fetch reusing an idle keep-alive socket must not stall', { timeout: 1000 }, async (t) => { + let connections = 0 + + const server = createServer((req, res) => { + res.writeHead(200, { 'content-length': 2 }) + res.end('ok') + }) + + server.on('connection', () => { + connections++ + }) + + server.listen(0) + await once(server, 'listening') + + const url = `http://127.0.0.1:${server.address().port}` + const agent = new Agent({ + connections: 1, + pipelining: 1, + keepAliveTimeout: 60_000 + }) + + t.after(async () => { + await agent.close() + server.close() + }) + + { + const res = await fetch(`${url}/0`, { dispatcher: agent }) + assert.strictEqual(await res.text(), 'ok') + } + assert.strictEqual(connections, 1) + + for (let i = 1; i <= REUSES; i++) { + const requested = once(server, 'request') + const resPromise = fetch(`${url}/${i}`, { dispatcher: agent }) + resPromise.catch(() => {}) + + await requested + + const res = await resPromise + assert.strictEqual(await res.text(), 'ok') + assert.strictEqual(connections, 1, 'keep-alive socket must be reused') + } +}) From af145577230d1910043fd8a19c2147b68a1574f0 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Fri, 21 Aug 2026 01:28:39 +0000 Subject: [PATCH 2/4] fix(fetch): default Request priority on the inner request Move the "auto" default from the RequestInit WebIDL converter to makeRequest so an omitted init stays empty after conversion. Restores the Object.keys empty-init check. Assisted by Cursor Signed-off-by: Yagiz Nizipli --- lib/web/fetch/request.js | 42 ++++------------------------------------ test/fetch/request.js | 9 +++++---- 2 files changed, 9 insertions(+), 42 deletions(-) diff --git a/lib/web/fetch/request.js b/lib/web/fetch/request.js index bd12987891c..56945ec02ee 100644 --- a/lib/web/fetch/request.js +++ b/lib/web/fetch/request.js @@ -30,39 +30,6 @@ const { getMaxListeners, setMaxListeners, defaultMaxListeners } = require('node: const kAbortController = Symbol('abortController') -/** - * Fetch's "If init is not empty" check is based on user-specified - * RequestInit members, not dictionary defaults. undici's `priority` - * member defaults to `"auto"`, so `Object.keys(convertedInit)` is - * never empty and would otherwise force the expensive header - * clone/clear/re-append path on every `fetch(url)` / `new Request(url)`. - * - * Matches the spec / Servo / Chromium "any members present" test. - * - * @param {object | null | undefined} init - * @returns {boolean} - */ -function requestInitHasUserMembers (init) { - return init != null && ( - init.method !== undefined || - init.headers !== undefined || - init.body !== undefined || - init.referrer !== undefined || - init.referrerPolicy !== undefined || - init.mode !== undefined || - init.credentials !== undefined || - init.cache !== undefined || - init.redirect !== undefined || - init.integrity !== undefined || - init.keepalive !== undefined || - init.signal !== undefined || - 'window' in init || - init.duplex !== undefined || - init.dispatcher !== undefined || - init.priority !== undefined - ) -} - const requestFinalizer = new FinalizationRegistry(({ signal, abort }) => { signal.removeEventListener('abort', abort) }) @@ -149,8 +116,6 @@ class Request { webidl.argumentLengthCheck(arguments, 1, prefix) input = webidl.converters.RequestInfo(input) - // Capture this before WebIDL conversion fills dictionary defaults. - const initHasKey = requestInitHasUserMembers(init) init = webidl.converters.RequestInit(init) // 1. Let request be null. @@ -276,6 +241,8 @@ class Request { urlList: [...request.urlList] }) + const initHasKey = Object.keys(init).length !== 0 + // 13. If init is not empty, then: if (initHasKey) { // 1. If request’s mode is "navigate", then set it to "same-origin". @@ -956,7 +923,7 @@ function makeRequest (init) { serviceWorkers: init.serviceWorkers ?? 'all', initiator: init.initiator ?? '', destination: init.destination ?? '', - priority: init.priority ?? null, + priority: init.priority ?? 'auto', origin: init.origin ?? 'client', policyContainer: init.policyContainer ?? 'client', referrer: init.referrer ?? 'client', @@ -1162,8 +1129,7 @@ webidl.converters.RequestInit = webidl.dictionaryConverter([ { key: 'priority', converter: webidl.converters.DOMString, - allowedValues: ['high', 'low', 'auto'], - defaultValue: () => 'auto' + allowedValues: ['high', 'low', 'auto'] } ]) diff --git a/test/fetch/request.js b/test/fetch/request.js index 964e1808dfa..85ecbb6029f 100644 --- a/test/fetch/request.js +++ b/test/fetch/request.js @@ -420,10 +420,11 @@ test('request.referrer', (t) => { } }) -// Dictionary defaults (undici's priority: "auto") must not make an omitted -// RequestInit count as non-empty. That used to force a header clone/clear -// on every fetch(url) / new Request(url) and reset copied request state. -test('omitted RequestInit is empty even though priority defaults to auto', (t) => { +// RequestInit.priority must not have a WebIDL default. A converter default +// of "auto" made every omitted init non-empty (Object.keys never empty), +// which cloned/cleared headers on fetch(url) / new Request(url) and reset +// copied request state. +test('omitted RequestInit is empty', (t) => { const parent = new Request('http://localhost/a', { method: 'POST', body: 'hi', From c16b846a438e9a54b021723f1c7752bbedde589d Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Fri, 21 Aug 2026 01:34:06 +0000 Subject: [PATCH 3/4] test(fetch): drop extra RequestInit empty-init tests Review feedback: the converter default move is sufficient; these Request-copy cases are not needed. Assisted by Cursor Signed-off-by: Yagiz Nizipli --- test/fetch/request.js | 38 -------------------------------------- 1 file changed, 38 deletions(-) diff --git a/test/fetch/request.js b/test/fetch/request.js index 85ecbb6029f..51e92b5c061 100644 --- a/test/fetch/request.js +++ b/test/fetch/request.js @@ -420,44 +420,6 @@ test('request.referrer', (t) => { } }) -// RequestInit.priority must not have a WebIDL default. A converter default -// of "auto" made every omitted init non-empty (Object.keys never empty), -// which cloned/cleared headers on fetch(url) / new Request(url) and reset -// copied request state. -test('omitted RequestInit is empty', (t) => { - const parent = new Request('http://localhost/a', { - method: 'POST', - body: 'hi', - referrerPolicy: 'unsafe-url', - headers: { 'x-a': '1' } - }) - - const copied = new Request(parent) - t.assert.strictEqual(copied.method, 'POST') - t.assert.strictEqual(copied.referrerPolicy, 'unsafe-url') - t.assert.deepStrictEqual([...copied.headers], [ - ['content-type', 'text/plain;charset=UTF-8'], - ['x-a', '1'] - ]) - - copied.headers.append('x-b', '2') - t.assert.deepStrictEqual([...parent.headers], [ - ['content-type', 'text/plain;charset=UTF-8'], - ['x-a', '1'] - ]) -}) - -test('explicit RequestInit members still count as non-empty', (t) => { - const parent = new Request('http://localhost/a', { - referrerPolicy: 'unsafe-url', - headers: { 'x-a': '1' } - }) - - const copied = new Request(parent, { priority: 'high' }) - t.assert.strictEqual(copied.referrerPolicy, '') - t.assert.deepStrictEqual([...copied.headers], [['x-a', '1']]) -}) - // https://github.com/nodejs/undici/issues/2445 test('Clone the set-cookie header when Request is passed as the first parameter and no header is passed.', (t) => { const request = new Request('http://localhost', { headers: { 'set-cookie': 'A' } }) From 4b22133769322fbcc464840785b3c46ad1974783 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Fri, 21 Aug 2026 01:49:56 +0000 Subject: [PATCH 4/4] revert: move idle-socket setImmediate change to a follow-up PR Keep this PR scoped to the RequestInit priority default fix. Assisted by Cursor Signed-off-by: Yagiz Nizipli --- benchmarks/fetch/sequential-keepalive.mjs | 48 ----------------------- lib/dispatcher/client-h1.js | 16 ++------ test/node-test/keep-alive-reuse.js | 48 +---------------------- 3 files changed, 5 insertions(+), 107 deletions(-) delete mode 100644 benchmarks/fetch/sequential-keepalive.mjs diff --git a/benchmarks/fetch/sequential-keepalive.mjs b/benchmarks/fetch/sequential-keepalive.mjs deleted file mode 100644 index 1a21ad36535..00000000000 --- a/benchmarks/fetch/sequential-keepalive.mjs +++ /dev/null @@ -1,48 +0,0 @@ -'use strict' - -import { createServer } from 'node:http' -import { Agent, fetch } from '../../index.js' - -const ITERATIONS = Number(process.env.SAMPLES ?? 2000) -const WARMUP = 300 - -const server = createServer((req, res) => { - res.writeHead(200, { 'content-type': 'application/json' }) - res.end('{"ok":1}') -}) - -server.keepAliveTimeout = 65_000 - -await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) - -const { port } = server.address() -const url = `http://127.0.0.1:${port}/` -const agent = new Agent({ - keepAliveTimeout: 60_000, - connections: 1, - pipelining: 1 -}) - -for (let i = 0; i < WARMUP; i++) { - await (await fetch(url, { dispatcher: agent })).text() -} - -const times = new Array(ITERATIONS) -for (let i = 0; i < ITERATIONS; i++) { - const t0 = process.hrtime.bigint() - await (await fetch(url, { dispatcher: agent })).text() - times[i] = Number(process.hrtime.bigint() - t0) -} - -times.sort((a, b) => a - b) -const pct = (p) => times[Math.min(ITERATIONS - 1, Math.floor(ITERATIONS * p))] / 1e6 - -console.log(JSON.stringify({ - iterations: ITERATIONS, - p50_ms: Number(pct(0.5).toFixed(3)), - p90_ms: Number(pct(0.9).toFixed(3)), - p99_ms: Number(pct(0.99).toFixed(3)) -})) - -await agent.close() -server.close() diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js index f06ca74bfe5..9f6f17c1579 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -1052,7 +1052,7 @@ function onSocketClose () { function clearIdleSocketValidation (socket) { if (socket[kIdleSocketValidationTimeout]) { - clearImmediate(socket[kIdleSocketValidationTimeout]) + clearTimeout(socket[kIdleSocketValidationTimeout]) socket[kIdleSocketValidationTimeout] = null } @@ -1061,23 +1061,15 @@ function clearIdleSocketValidation (socket) { function scheduleIdleSocketValidation (client, socket) { socket[kIdleSocketValidation] = 1 - // Yield to the check phase (after poll) so unsolicited bytes / FIN / RST - // already pending on this idle keep-alive socket are processed before the - // next request is written (GHSA-35p6-xmwp-9g52). - // - // setTimeout(0) pays Node's ~1ms timer floor on every sequential reuse - // (#5493). setImmediate avoids that, but an *unref'd* Immediate lets poll - // block for ~500ms when the event loop is otherwise idle (#5600 / #5606). - // A ref'd Immediate both keeps the pending request alive and makes poll - // return immediately — the hybrid those issues asked for. - socket[kIdleSocketValidationTimeout] = setImmediate(() => { + socket[kIdleSocketValidationTimeout] = setTimeout(() => { socket[kIdleSocketValidationTimeout] = null socket[kIdleSocketValidation] = 2 if (client[kSocket] === socket && !socket.destroyed) { client[kResume]() } - }) + }, 0) + socket[kIdleSocketValidationTimeout].unref?.() } /** diff --git a/test/node-test/keep-alive-reuse.js b/test/node-test/keep-alive-reuse.js index 4158a43993f..5a92f6003c0 100644 --- a/test/node-test/keep-alive-reuse.js +++ b/test/node-test/keep-alive-reuse.js @@ -4,7 +4,7 @@ const { test } = require('node:test') const assert = require('node:assert') const { createServer } = require('node:http') const { once } = require('node:events') -const { Agent, Pool, fetch } = require('../..') +const { Pool } = require('../..') // Regression for #5600 / #5606: // Reusing an idle keep-alive socket must not stall behind the poll phase. @@ -66,49 +66,3 @@ test('reusing an idle keep-alive socket must not stall', { timeout: 1000 }, asyn assert.strictEqual(connections, 1, 'keep-alive socket must be reused') } }) - -test('fetch reusing an idle keep-alive socket must not stall', { timeout: 1000 }, async (t) => { - let connections = 0 - - const server = createServer((req, res) => { - res.writeHead(200, { 'content-length': 2 }) - res.end('ok') - }) - - server.on('connection', () => { - connections++ - }) - - server.listen(0) - await once(server, 'listening') - - const url = `http://127.0.0.1:${server.address().port}` - const agent = new Agent({ - connections: 1, - pipelining: 1, - keepAliveTimeout: 60_000 - }) - - t.after(async () => { - await agent.close() - server.close() - }) - - { - const res = await fetch(`${url}/0`, { dispatcher: agent }) - assert.strictEqual(await res.text(), 'ok') - } - assert.strictEqual(connections, 1) - - for (let i = 1; i <= REUSES; i++) { - const requested = once(server, 'request') - const resPromise = fetch(`${url}/${i}`, { dispatcher: agent }) - resPromise.catch(() => {}) - - await requested - - const res = await resPromise - assert.strictEqual(await res.text(), 'ok') - assert.strictEqual(connections, 1, 'keep-alive socket must be reused') - } -})