Skip to content

Commit 07eb330

Browse files
divybotlittledivy
andauthored
fix(ext/node): per-request executionAsyncResource() for async_hooks (#34188)
## Summary `executionAsyncResource()` from `node:async_hooks` previously returned a fresh `{}` on every call, so writes never persisted and the standard CLS-on-async- hooks pattern blew up with: ``` TypeError: Cannot destructure property 'state' of 'executionAsyncResource(...)[sym]' as it is undefined. ``` This change tracks the current resource via an `AsyncVariable` (backed by V8 continuation-preserved embedder data), defaulting to a shared top-level singleton. The value propagates naturally across promise/await transitions and the existing setTimeout async-context capture/restore. - `AsyncResource.runInAsyncScope` enters `this` as the current resource for the scope. - The HTTP/1.1 server enters a per-request scope around the `request`/`checkContinue`/`checkExpectation` emit so concurrent requests get isolated resources. Without this, every concurrent request would race on anything stashed on the shared top-level resource. Enables these two Node compat tests that the issue called out: - `parallel/test-async-hooks-execution-async-resource.js` - `parallel/test-async-hooks-execution-async-resource-await.js` Closes denoland/orchid#129 ## Test plan - [x] Both target tests pass under `cargo test --test node_compat` - [x] Full `async-hooks` and `async-local-storage` suites — no regressions vs. tests already enabled in `tests/node_compat/config.jsonc` - [x] `parallel::test-http` suite — no new regressions in tests already in `config.jsonc` (one pre-existing flaky HTTP/2 test unrelated to this change) Co-authored-by: divybot <divybot@users.noreply.github.com> Co-authored-by: Divy Srivastava <me@littledivy.com>
1 parent 41523bf commit 07eb330

4 files changed

Lines changed: 107 additions & 48 deletions

File tree

ext/node/polyfills/_http_server.js

Lines changed: 59 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,10 @@ const {
7878
validateObject,
7979
} = core.loadExtScript("ext:deno_node/internal/validators.mjs");
8080
const { nextTick } = core.loadExtScript("ext:deno_node/_next_tick.ts");
81+
const {
82+
enterAsyncResource,
83+
exitAsyncResource,
84+
} = core.loadExtScript("ext:deno_node/internal/async_hooks.ts");
8185
const { enqueueNodePerformanceEntry } = core.loadExtScript(
8286
"ext:deno_node/perf_hooks.js",
8387
);
@@ -865,58 +869,69 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {
865869
});
866870
}
867871

868-
let handled = false;
869-
870-
if (req.httpVersionMajor === 1 && req.httpVersionMinor === 1) {
871-
if (
872-
server.requireHostHeader !== false &&
873-
req.headers.host === undefined
874-
) {
875-
res.writeHead(400, ["Connection", "close"]);
876-
res.end();
877-
return 0;
878-
}
872+
// Enter a new async-hooks resource scope for the duration of the request
873+
// emission. Each request gets its own resource (the IncomingMessage), so
874+
// executionAsyncResource() returns a per-request object that is preserved
875+
// across timers, await transitions, etc. Without this, every request would
876+
// share the top-level resource and concurrent requests would race on any
877+
// state stashed there.
878+
const prevAsyncResource = enterAsyncResource(req);
879+
try {
880+
let handled = false;
881+
882+
if (req.httpVersionMajor === 1 && req.httpVersionMinor === 1) {
883+
if (
884+
server.requireHostHeader !== false &&
885+
req.headers.host === undefined
886+
) {
887+
res.writeHead(400, ["Connection", "close"]);
888+
res.end();
889+
return 0;
890+
}
879891

880-
const isRequestsLimitSet =
881-
typeof server.maxRequestsPerSocket === "number" &&
882-
server.maxRequestsPerSocket > 0;
892+
const isRequestsLimitSet =
893+
typeof server.maxRequestsPerSocket === "number" &&
894+
server.maxRequestsPerSocket > 0;
883895

884-
if (isRequestsLimitSet) {
885-
state.requestsCount++;
886-
res.maxRequestsOnConnectionReached =
887-
server.maxRequestsPerSocket <= state.requestsCount;
888-
}
896+
if (isRequestsLimitSet) {
897+
state.requestsCount++;
898+
res.maxRequestsOnConnectionReached =
899+
server.maxRequestsPerSocket <= state.requestsCount;
900+
}
889901

890-
if (
891-
isRequestsLimitSet &&
892-
server.maxRequestsPerSocket < state.requestsCount
893-
) {
894-
handled = true;
895-
server.emit("dropRequest", req, socket);
896-
res.writeHead(503);
897-
res.end();
898-
} else if (req.headers.expect !== undefined) {
899-
handled = true;
900-
901-
if (continueExpression.test(req.headers.expect)) {
902-
res._expect_continue = true;
903-
if (server.listenerCount("checkContinue") > 0) {
904-
server.emit("checkContinue", req, res);
902+
if (
903+
isRequestsLimitSet &&
904+
server.maxRequestsPerSocket < state.requestsCount
905+
) {
906+
handled = true;
907+
server.emit("dropRequest", req, socket);
908+
res.writeHead(503);
909+
res.end();
910+
} else if (req.headers.expect !== undefined) {
911+
handled = true;
912+
913+
if (continueExpression.test(req.headers.expect)) {
914+
res._expect_continue = true;
915+
if (server.listenerCount("checkContinue") > 0) {
916+
server.emit("checkContinue", req, res);
917+
} else {
918+
res.writeContinue();
919+
server.emit("request", req, res);
920+
}
921+
} else if (server.listenerCount("checkExpectation") > 0) {
922+
server.emit("checkExpectation", req, res);
905923
} else {
906-
res.writeContinue();
907-
server.emit("request", req, res);
924+
res.writeHead(417);
925+
res.end();
908926
}
909-
} else if (server.listenerCount("checkExpectation") > 0) {
910-
server.emit("checkExpectation", req, res);
911-
} else {
912-
res.writeHead(417);
913-
res.end();
914927
}
915928
}
916-
}
917929

918-
if (!handled) {
919-
server.emit("request", req, res);
930+
if (!handled) {
931+
server.emit("request", req, res);
932+
}
933+
} finally {
934+
exitAsyncResource(prevAsyncResource);
920935
}
921936

922937
return 0;

ext/node/polyfills/async_hooks.ts

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,10 @@ const {
1616
emitBefore,
1717
emitDestroy: emitDestroyHook,
1818
emitInit,
19+
enterAsyncResource,
20+
exitAsyncResource,
1921
executionAsyncId: internalExecutionAsyncId,
22+
executionAsyncResource: internalExecutionAsyncResource,
2023
newAsyncId,
2124
} = core.loadExtScript("ext:deno_node/internal/async_hooks.ts");
2225

@@ -70,7 +73,14 @@ class AsyncResource {
7073
emitBefore(this.#asyncId);
7174
try {
7275
setAsyncContext(this.#snapshot);
73-
return ReflectApply(fn, thisArg, args);
76+
// Enter this resource as the current executionAsyncResource() so that
77+
// user code inside the scope observes `this` as the active resource.
78+
const prevResource = enterAsyncResource(this);
79+
try {
80+
return ReflectApply(fn, thisArg, args);
81+
} finally {
82+
exitAsyncResource(prevResource);
83+
}
7484
} finally {
7585
setAsyncContext(previousContext);
7686
emitAfter(this.#asyncId);
@@ -198,9 +208,7 @@ function triggerAsyncId() {
198208
return 0;
199209
}
200210

201-
function executionAsyncResource() {
202-
return {};
203-
}
211+
const executionAsyncResource = internalExecutionAsyncResource;
204212

205213
const asyncWrapProviders = ObjectFreeze({
206214
__proto__: null,

ext/node/polyfills/internal/async_hooks.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,10 @@ const {
2323
FunctionPrototypeApply,
2424
Symbol,
2525
} = primordials;
26+
const {
27+
AsyncVariable,
28+
setAsyncContext,
29+
} = core;
2630

2731
interface ActiveHooks {
2832
array: AsyncHook[];
@@ -80,6 +84,33 @@ function executionAsyncId(): number {
8084
return executionAsyncIdStack[executionAsyncIdStack.length - 1] || 0;
8185
}
8286

87+
// Per-async-context "current resource" tracked via the AsyncVariable
88+
// machinery (V8 ContinuationPreservedEmbedderData). This propagates across
89+
// promises and await transitions automatically. The top-level resource is a
90+
// shared singleton used before any specific resource has been entered.
91+
// deno-lint-ignore no-explicit-any
92+
const topLevelResource: any = { __proto__: null };
93+
// deno-lint-ignore no-explicit-any
94+
const executionResourceVariable: any = new AsyncVariable();
95+
96+
// deno-lint-ignore no-explicit-any
97+
function executionAsyncResource(): any {
98+
const r = executionResourceVariable.get();
99+
return r === undefined ? topLevelResource : r;
100+
}
101+
102+
// Enter a new "current resource" scope. The returned value is the previous
103+
// async context snapshot that must be restored by exitAsyncResource.
104+
// deno-lint-ignore no-explicit-any
105+
function enterAsyncResource(resource: any): any {
106+
return executionResourceVariable.enter(resource);
107+
}
108+
109+
// deno-lint-ignore no-explicit-any
110+
function exitAsyncResource(previousContext: any): void {
111+
setAsyncContext(previousContext);
112+
}
113+
83114
// Emit functions that work with the internal hook system
84115
function emitBefore(asyncId: number): void {
85116
ArrayPrototypePush(executionAsyncIdStack, asyncId);
@@ -461,6 +492,9 @@ return {
461492
emitInit: emitInitNative,
462493
constants,
463494
executionAsyncId,
495+
executionAsyncResource,
496+
enterAsyncResource,
497+
exitAsyncResource,
464498
emitBefore,
465499
emitAfter,
466500
emitDestroy,

tests/node_compat/config.jsonc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,8 @@
182182
"parallel/test-async-hooks-disable-gc-tracking.js": {},
183183
"parallel/test-async-hooks-enable-disable.js": {},
184184
"parallel/test-async-hooks-enabledhooksexits.js": {},
185+
"parallel/test-async-hooks-execution-async-resource-await.js": {},
186+
"parallel/test-async-hooks-execution-async-resource.js": {},
185187
"parallel/test-async-hooks-http-agent-destroy.js": {},
186188
"parallel/test-async-hooks-http-agent.js": {},
187189
"parallel/test-async-hooks-prevent-double-destroy.js": {},

0 commit comments

Comments
 (0)