From 0020a899536b5ae30c7c10c47e4e74e97c17ddf4 Mon Sep 17 00:00:00 2001 From: Ic3b3rg Date: Tue, 3 Nov 2026 23:39:48 +0100 Subject: [PATCH 1/3] http: release async context from idle sockets Clear context references from socket handles and timeout timers when keep-alive connections enter the agent's free pool, including TCP handles beneath TLS. Rebind parent handles and timers to the current request context on reuse. Preserve shared context frames and existing async_hooks notifications. Add regression coverage for garbage collection and context propagation. Fixes: https://github.com/nodejs/node/issues/67181 Signed-off-by: Ic3b3rg --- lib/internal/http/agent.js | 24 ++++ src/async_wrap.cc | 13 +++ src/async_wrap.h | 2 + .../test-http-agent-idle-async-context-gc.js | 68 +++++++++++ .../test-http-agent-idle-async-context.js | 108 ++++++++++++++++++ .../test-https-agent-idle-async-context-gc.js | 77 +++++++++++++ ...ttps-agent-queued-timeout-async-context.js | 98 ++++++++++++++++ 7 files changed, 390 insertions(+) create mode 100644 test/parallel/test-http-agent-idle-async-context-gc.js create mode 100644 test/parallel/test-http-agent-idle-async-context.js create mode 100644 test/parallel/test-https-agent-idle-async-context-gc.js create mode 100644 test/parallel/test-https-agent-queued-timeout-async-context.js diff --git a/lib/internal/http/agent.js b/lib/internal/http/agent.js index 3871c5b56a2f..2fdfbbc3fb32 100644 --- a/lib/internal/http/agent.js +++ b/lib/internal/http/agent.js @@ -43,6 +43,7 @@ const { getGlobalAgent, } = require('internal/http'); const { AsyncResource } = require('async_hooks'); +const AsyncContextFrame = require('internal/async_context_frame'); const { async_id_symbol, owner_symbol, @@ -56,6 +57,10 @@ const { const { onStreamRead, } = require('internal/stream_base_commons'); +const { + async_context_frame, + kTimeout, +} = require('internal/timers'); const { kReadBytesOrError, streamBaseState, @@ -261,6 +266,8 @@ function Agent(options) { socket._httpMessage = null; this.removeSocket(socket, options); + setSocketAsyncContext(socket, undefined, socket._handle); + socket.once('error', freeSocketErrorListener); installFreeSocketDataGuard(socket); freeSockets.push(socket); @@ -700,6 +707,7 @@ function setRequestSocket(agent, req, socket) { } function asyncResetHandle(socket) { + const frame = AsyncContextFrame.current(); // Guard against an uninitialized or user supplied Socket. const handle = socket._handle; if (handle && typeof handle.asyncReset === 'function') { @@ -707,6 +715,22 @@ function asyncResetHandle(socket) { handle.asyncReset(new ReusedHandle(handle.getProviderType(), handle)); socket[async_id_symbol] = handle.getAsyncId(); } + // asyncReset() has already updated the socket's own handle. + setSocketAsyncContext(socket, frame, handle?._parent); +} + +function setSocketAsyncContext(socket, frame, handle) { + // TLS keeps parent transport handles alive, and socket timeouts can be + // refreshed across requests. Each holds its own reference to the frame. + while (handle && typeof handle.setAsyncContextFrame === 'function') { + handle.setAsyncContextFrame(frame); + handle = handle._parent; + } + + for (let current = socket; current != null; current = current._parent) { + const timer = current[kTimeout]; + if (timer) timer[async_context_frame] = frame; + } } module.exports = { diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 7b7930187bd5..a3672536035e 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -319,6 +319,16 @@ void AsyncWrap::AsyncReset(const FunctionCallbackInfo& args) { wrap->AsyncReset(resource, execution_async_id); } +void AsyncWrap::SetAsyncContextFrame(const FunctionCallbackInfo& args) { + CHECK(args[0]->IsUndefined() || args[0]->IsMap()); + + AsyncWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.This()); + + // Replace the reference without mutating the shared frame or emitting hooks. + wrap->set_context_frame(args[0]); +} + // Useful for debugging async context propagation. Not intended for public use. void AsyncWrap::GetAsyncContextFrame(const FunctionCallbackInfo& args) { AsyncWrap* wrap; @@ -376,6 +386,8 @@ Local AsyncWrap::GetConstructorTemplate( FIXED_ONE_BYTE_STRING(isolate_data->isolate(), "AsyncWrap")); SetProtoMethod(isolate, tmpl, "getAsyncId", AsyncWrap::GetAsyncId); SetProtoMethod(isolate, tmpl, "asyncReset", AsyncWrap::AsyncReset); + SetProtoMethod( + isolate, tmpl, "setAsyncContextFrame", AsyncWrap::SetAsyncContextFrame); SetProtoMethod(isolate, tmpl, "getAsyncContextFrameForDebuggingOnly", @@ -509,6 +521,7 @@ void AsyncWrap::RegisterExternalReferences( registry->Register(RegisterDestroyHook); registry->Register(AsyncWrap::GetAsyncId); registry->Register(AsyncWrap::AsyncReset); + registry->Register(AsyncWrap::SetAsyncContextFrame); registry->Register(AsyncWrap::GetAsyncContextFrame); registry->Register(AsyncWrap::GetProviderType); } diff --git a/src/async_wrap.h b/src/async_wrap.h index a14d84088b38..a42e52f0fdc0 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -168,6 +168,8 @@ class AsyncWrap : public BaseObject { static void ClearAsyncIdStack( const v8::FunctionCallbackInfo& args); static void AsyncReset(const v8::FunctionCallbackInfo& args); + static void SetAsyncContextFrame( + const v8::FunctionCallbackInfo& args); static void GetAsyncContextFrame( const v8::FunctionCallbackInfo& args); static void GetProviderType(const v8::FunctionCallbackInfo& args); diff --git a/test/parallel/test-http-agent-idle-async-context-gc.js b/test/parallel/test-http-agent-idle-async-context-gc.js new file mode 100644 index 000000000000..29c45be56b62 --- /dev/null +++ b/test/parallel/test-http-agent-idle-async-context-gc.js @@ -0,0 +1,68 @@ +// Flags: --expose-gc +'use strict'; + +const common = require('../common'); +const { gcUntil } = require('../common/gc'); +const assert = require('node:assert'); +const { AsyncLocalStorage } = require('node:async_hooks'); +const { once } = require('node:events'); +const http = require('node:http'); + +// This test ensures that an idle keep-alive socket does not retain the +// context of the requests that created and reused it, through either its +// handle or its timeout. +// Refs: https://github.com/nodejs/node/issues/67181 + +const storage = new AsyncLocalStorage(); +const agent = new http.Agent({ + keepAlive: true, + maxSockets: 1, + timeout: 60_000, +}); +const server = http.createServer(common.mustCall((req, res) => { + res.end('ok'); +}, 2)); +server.keepAliveTimeout = 60_000; + +function requestInContext() { + const store = {}; + const reference = new WeakRef(store); + return storage.run(store, common.mustCall(() => new Promise((resolve, reject) => { + // Use an IP address, so the first DNS lookup does not create the default + // resolver, which keeps the context it was created in, inside the store. + http.get({ + agent, + host: '127.0.0.1', + port: server.address().port, + }, common.mustCall((res) => { + const socket = res.socket; + res.on('end', common.mustCall(() => resolve({ reference, socket }))); + res.resume(); + })).on('error', reject); + }))); +} + +function assertPooled(socket) { + assert.deepStrictEqual(Object.values(agent.freeSockets).flat(), [socket]); +} + +async function main() { + server.listen(0, '127.0.0.1'); + await once(server, 'listening'); + + const created = await requestInContext(); + const reused = await requestInContext(); + assert.strictEqual(reused.socket, created.socket); + + await gcUntil('idle agent socket releases request contexts', () => { + return created.reference.deref() === undefined && + reused.reference.deref() === undefined; + }); + // The contexts must be collected while the socket is still pooled. + assertPooled(created.socket); + + agent.destroy(); + server.close(); +} + +main().then(common.mustCall()); diff --git a/test/parallel/test-http-agent-idle-async-context.js b/test/parallel/test-http-agent-idle-async-context.js new file mode 100644 index 000000000000..4b9feff3581a --- /dev/null +++ b/test/parallel/test-http-agent-idle-async-context.js @@ -0,0 +1,108 @@ +'use strict'; + +const common = require('../common'); +const assert = require('node:assert'); +const { AsyncLocalStorage } = require('node:async_hooks'); +const { once } = require('node:events'); +const http = require('node:http'); +const { setImmediate } = require('node:timers/promises'); + +// This test ensures that a keep-alive socket runs its callbacks in the context +// of the request currently using it, and in no request context while it is +// idle in the agent pool. Other work started by a request must keep its +// context after the socket is released. +// Refs: https://github.com/nodejs/node/issues/67181 + +const storage = new AsyncLocalStorage(); +const agent = new http.Agent({ + keepAlive: true, + maxSockets: 1, + timeout: common.platformTimeout(1000), +}); +const server = http.createServer(common.mustCall((req, res) => { + if (req.url !== '/stall') res.end('ok'); +}, 3)); +server.keepAliveTimeout = 60_000; + +function request(path = '/') { + return http.get({ agent, port: server.address().port, path }); +} + +function completedRequest(expectedStore) { + return new Promise((resolve, reject) => { + request().on('response', common.mustCall((res) => { + assert.strictEqual(storage.getStore(), expectedStore); + const socket = res.socket; + const asyncId = socket._handle.getAsyncId(); + res.on('end', common.mustCall(() => resolve({ socket, asyncId }))); + res.resume(); + })).on('error', reject); + }); +} + +function assertPooled({ socket, asyncId }) { + assert.deepStrictEqual(Object.values(agent.freeSockets).flat(), [socket]); + // Entering the pool must not reset the handle's async identity. + assert.strictEqual(socket._handle.getAsyncId(), asyncId); +} + +async function main() { + server.listen(0); + await once(server, 'listening'); + + // The socket and independent work must capture the same context frame. + const firstStore = { requestId: 'first' }; + let releaseBackgroundWork; + let backgroundWork; + const created = await storage.run(firstStore, common.mustCall(() => { + backgroundWork = new Promise((resolve) => { + releaseBackgroundWork = resolve; + }).then(common.mustCall(() => { + assert.strictEqual(storage.getStore(), firstStore); + })); + return completedRequest(firstStore); + })); + const { socket } = created; + await setImmediate(); + assertPooled(created); + + // A request that reuses the socket must observe its own context in the + // socket timeout. + const secondStore = { requestId: 'second' }; + await storage.run(secondStore, common.mustCall(() => new Promise((resolve) => { + const req = request('/stall'); + req.on('socket', common.mustCall((reused) => { + assert.strictEqual(reused, socket); + })); + req.on('timeout', common.mustCall(() => { + assert.strictEqual(storage.getStore(), secondStore); + req.destroy(); + })); + req.on('error', common.expectsError({ code: 'ECONNRESET' })); + req.on('close', common.mustCall(resolve)); + }))); + + // Releasing the socket must not alter the context of other work. + releaseBackgroundWork(); + await backgroundWork; + + // An idle socket times out and is closed by the agent outside of the + // context of the request that created it. + const thirdStore = { requestId: 'third' }; + const idle = await storage.run(thirdStore, + common.mustCall(() => completedRequest(thirdStore))); + const idleSocket = idle.socket; + await setImmediate(); + assertPooled(idle); + idleSocket.on('timeout', common.mustCall(() => { + assert.strictEqual(storage.getStore(), undefined); + })); + idleSocket.on('close', common.mustCall(() => { + assert.strictEqual(storage.getStore(), undefined); + })); + await once(idleSocket, 'close'); + + server.close(); +} + +main().then(common.mustCall()); diff --git a/test/parallel/test-https-agent-idle-async-context-gc.js b/test/parallel/test-https-agent-idle-async-context-gc.js new file mode 100644 index 000000000000..022d5555bb4e --- /dev/null +++ b/test/parallel/test-https-agent-idle-async-context-gc.js @@ -0,0 +1,77 @@ +// Flags: --expose-gc +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const fixtures = require('../common/fixtures'); +const { gcUntil } = require('../common/gc'); +const assert = require('node:assert'); +const { AsyncLocalStorage } = require('node:async_hooks'); +const { once } = require('node:events'); +const https = require('node:https'); + +// This test ensures that an idle keep-alive TLS socket does not retain the +// context of the requests that created and reused it, through either its +// handles, including the TCP handle beneath TLS, or its timeout. +// Refs: https://github.com/nodejs/node/issues/67181 + +const storage = new AsyncLocalStorage(); +const agent = new https.Agent({ + keepAlive: true, + maxSockets: 1, + timeout: 60_000, + ca: fixtures.readKey('ca1-cert.pem'), + servername: 'agent1', +}); +const server = https.createServer({ + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), +}, common.mustCall((req, res) => { + res.end('ok'); +}, 2)); +server.keepAliveTimeout = 60_000; + +function requestInContext() { + const store = {}; + const reference = new WeakRef(store); + return storage.run(store, common.mustCall(() => new Promise((resolve, reject) => { + // Use an IP address, so the first DNS lookup does not create the default + // resolver, which keeps the context it was created in, inside the store. + https.get({ + agent, + host: '127.0.0.1', + port: server.address().port, + }, common.mustCall((res) => { + const socket = res.socket; + res.on('end', common.mustCall(() => resolve({ reference, socket }))); + res.resume(); + })).on('error', reject); + }))); +} + +function assertPooled(socket) { + assert.deepStrictEqual(Object.values(agent.freeSockets).flat(), [socket]); +} + +async function main() { + server.listen(0, '127.0.0.1'); + await once(server, 'listening'); + + const created = await requestInContext(); + const reused = await requestInContext(); + assert.strictEqual(reused.socket, created.socket); + + await gcUntil('idle agent TLS socket releases request contexts', () => { + return created.reference.deref() === undefined && + reused.reference.deref() === undefined; + }); + // The contexts must be collected while the socket is still pooled. + assertPooled(created.socket); + + agent.destroy(); + server.close(); +} + +main().then(common.mustCall()); diff --git a/test/parallel/test-https-agent-queued-timeout-async-context.js b/test/parallel/test-https-agent-queued-timeout-async-context.js new file mode 100644 index 000000000000..f70e162f075c --- /dev/null +++ b/test/parallel/test-https-agent-queued-timeout-async-context.js @@ -0,0 +1,98 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const fixtures = require('../common/fixtures'); +const assert = require('node:assert'); +const { AsyncLocalStorage } = require('node:async_hooks'); +const { once } = require('node:events'); +const https = require('node:https'); + +// A socket handed directly to a queued request must use that request's +// context for timeout, error, and close, including the parent TCP under TLS. +async function main() { + const storage = new AsyncLocalStorage(); + const timeout = common.platformTimeout(1000); + const agent = new https.Agent({ + keepAlive: true, + maxSockets: 1, + timeout, + ca: fixtures.readKey('ca1-cert.pem'), + servername: 'agent1', + }); + let heldResponse; + let firstReceived; + const received = new Promise((resolve) => { firstReceived = resolve; }); + const respond = common.mustCall((req, res) => { + if (req.url === '/hold') { + heldResponse = res; + firstReceived(); + } else { + assert.strictEqual(req.url, '/stall'); + } + }, 2); + const server = https.createServer({ + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), + }, respond); + const options = { agent, host: '127.0.0.1' }; + + server.listen(0, '127.0.0.1'); + await once(server, 'listening'); + options.port = server.address().port; + + try { + let socket; + const firstStore = { requestId: 'first' }; + const first = storage.run(firstStore, common.mustCall(() => + new Promise((resolve, reject) => { + const onResponse = common.mustCall((res) => { + assert.strictEqual(storage.getStore(), firstStore); + res.on('error', reject); + res.on('end', common.mustCall(resolve)); + res.resume(); + }); + const req = https.get({ ...options, path: '/hold' }, onResponse); + req.on('socket', common.mustCall((assigned) => { socket = assigned; })); + req.on('error', reject); + }))); + await received; + + const secondStore = { requestId: 'queued' }; + const error = new Error('test timeout'); + const second = storage.run(secondStore, common.mustCall(() => + new Promise((resolve) => { + const req = https.get({ ...options, path: '/stall' }, + common.mustNotCall('The response must stall')); + req.on('socket', common.mustCall((assigned) => { + assert.strictEqual(assigned, socket); + assert.strictEqual(storage.getStore(), secondStore); + })); + const onTimeout = common.mustCall(() => { + assert.strictEqual(storage.getStore(), secondStore); + req.destroy(error); + }); + req.setTimeout(timeout, onTimeout); + req.on('error', common.mustCall((received) => { + assert.strictEqual(received, error); + assert.strictEqual(storage.getStore(), secondStore); + })); + req.on('close', common.mustCall(() => { + assert.strictEqual(storage.getStore(), secondStore); + resolve(); + })); + assert.strictEqual(req.socket, null); + }))); + assert.strictEqual(Object.values(agent.requests).flat().length, 1); + heldResponse.end('ok'); + await Promise.all([first, second]); + } finally { + agent.destroy(); + server.closeAllConnections(); + await new Promise((resolve) => server.close(resolve)); + } +} + +main().then(common.mustCall()); From 5089f91ffb407a6d8ed69dd3249475f2323c6dce Mon Sep 17 00:00:00 2001 From: Ic3b3rg Date: Wed, 4 Nov 2026 00:03:49 +0100 Subject: [PATCH 2/3] test: simplify http agent test comments Signed-off-by: Ic3b3rg --- .../parallel/test-http-agent-idle-async-context-gc.js | 9 ++++----- test/parallel/test-http-agent-idle-async-context.js | 11 ++--------- .../test-https-agent-idle-async-context-gc.js | 9 ++++----- 3 files changed, 10 insertions(+), 19 deletions(-) diff --git a/test/parallel/test-http-agent-idle-async-context-gc.js b/test/parallel/test-http-agent-idle-async-context-gc.js index 29c45be56b62..bfaf2a4754a6 100644 --- a/test/parallel/test-http-agent-idle-async-context-gc.js +++ b/test/parallel/test-http-agent-idle-async-context-gc.js @@ -8,9 +8,8 @@ const { AsyncLocalStorage } = require('node:async_hooks'); const { once } = require('node:events'); const http = require('node:http'); -// This test ensures that an idle keep-alive socket does not retain the -// context of the requests that created and reused it, through either its -// handle or its timeout. +// Idle keep-alive sockets must release request contexts held by their +// handles and timeout timers. // Refs: https://github.com/nodejs/node/issues/67181 const storage = new AsyncLocalStorage(); @@ -28,8 +27,8 @@ function requestInContext() { const store = {}; const reference = new WeakRef(store); return storage.run(store, common.mustCall(() => new Promise((resolve, reject) => { - // Use an IP address, so the first DNS lookup does not create the default - // resolver, which keeps the context it was created in, inside the store. + // Avoid initializing the default DNS resolver inside ALS: it can retain + // the request context independently of the Agent. http.get({ agent, host: '127.0.0.1', diff --git a/test/parallel/test-http-agent-idle-async-context.js b/test/parallel/test-http-agent-idle-async-context.js index 4b9feff3581a..015c85a7aa44 100644 --- a/test/parallel/test-http-agent-idle-async-context.js +++ b/test/parallel/test-http-agent-idle-async-context.js @@ -7,10 +7,8 @@ const { once } = require('node:events'); const http = require('node:http'); const { setImmediate } = require('node:timers/promises'); -// This test ensures that a keep-alive socket runs its callbacks in the context -// of the request currently using it, and in no request context while it is -// idle in the agent pool. Other work started by a request must keep its -// context after the socket is released. +// Idle socket callbacks must not inherit a completed request's context. +// Active requests and independent asynchronous work must keep theirs. // Refs: https://github.com/nodejs/node/issues/67181 const storage = new AsyncLocalStorage(); @@ -66,8 +64,6 @@ async function main() { await setImmediate(); assertPooled(created); - // A request that reuses the socket must observe its own context in the - // socket timeout. const secondStore = { requestId: 'second' }; await storage.run(secondStore, common.mustCall(() => new Promise((resolve) => { const req = request('/stall'); @@ -82,12 +78,9 @@ async function main() { req.on('close', common.mustCall(resolve)); }))); - // Releasing the socket must not alter the context of other work. releaseBackgroundWork(); await backgroundWork; - // An idle socket times out and is closed by the agent outside of the - // context of the request that created it. const thirdStore = { requestId: 'third' }; const idle = await storage.run(thirdStore, common.mustCall(() => completedRequest(thirdStore))); diff --git a/test/parallel/test-https-agent-idle-async-context-gc.js b/test/parallel/test-https-agent-idle-async-context-gc.js index 022d5555bb4e..5b41e31d155a 100644 --- a/test/parallel/test-https-agent-idle-async-context-gc.js +++ b/test/parallel/test-https-agent-idle-async-context-gc.js @@ -12,9 +12,8 @@ const { AsyncLocalStorage } = require('node:async_hooks'); const { once } = require('node:events'); const https = require('node:https'); -// This test ensures that an idle keep-alive TLS socket does not retain the -// context of the requests that created and reused it, through either its -// handles, including the TCP handle beneath TLS, or its timeout. +// Idle TLS sockets must release request contexts held by their timeout +// timers and handles, including the TCP handle beneath TLS. // Refs: https://github.com/nodejs/node/issues/67181 const storage = new AsyncLocalStorage(); @@ -37,8 +36,8 @@ function requestInContext() { const store = {}; const reference = new WeakRef(store); return storage.run(store, common.mustCall(() => new Promise((resolve, reject) => { - // Use an IP address, so the first DNS lookup does not create the default - // resolver, which keeps the context it was created in, inside the store. + // Avoid initializing the default DNS resolver inside ALS: it can retain + // the request context independently of the Agent. https.get({ agent, host: '127.0.0.1', From b47a0ee623f51226f2f5b505347cab9c12bc5a11 Mon Sep 17 00:00:00 2001 From: Ic3b3rg Date: Wed, 4 Nov 2026 12:00:53 +0100 Subject: [PATCH 3/3] test: cover async context updates after close Signed-off-by: Ic3b3rg --- ...test-async-wrap-set-async-context-frame.js | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 test/parallel/test-async-wrap-set-async-context-frame.js diff --git a/test/parallel/test-async-wrap-set-async-context-frame.js b/test/parallel/test-async-wrap-set-async-context-frame.js new file mode 100644 index 000000000000..a366527bae2f --- /dev/null +++ b/test/parallel/test-async-wrap-set-async-context-frame.js @@ -0,0 +1,26 @@ +// Flags: --expose-internals +'use strict'; + +const common = require('../common'); +const assert = require('node:assert'); +const { internalBinding } = require('internal/test/binding'); +const { TCP, constants } = internalBinding('tcp_wrap'); + +// Context updates preserve the handle's identity and tolerate native destruction. +const handle = new TCP(constants.SOCKET); +const asyncId = handle.getAsyncId(); +const frame = new Map(); + +handle.setAsyncContextFrame(frame); +assert.strictEqual(handle.getAsyncContextFrameForDebuggingOnly(), frame); +handle.setAsyncContextFrame(undefined); +assert.strictEqual(handle.getAsyncContextFrameForDebuggingOnly(), undefined); +assert.strictEqual(handle.getAsyncId(), asyncId); + +handle.close(common.mustCall(() => { + // The native wrapper is deleted after the close callback returns. + setImmediate(common.mustCall(() => { + handle.setAsyncContextFrame(frame); + handle.setAsyncContextFrame(undefined); + })); +}));