Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 64 additions & 4 deletions src/jsc/bindings/ZigGlobalObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3588,6 +3588,26 @@
sourceOriginZ.deref();
}

// If this import() runs inside an AsyncLocalStorage context, record that
// context keyed by the resolved module key so the imported module's
// top-level evaluation can run with it (see moduleLoaderEvaluate). Node
// preserves the context across dynamic-import evaluation via V8's
// continuation-preserved embedder data; JSC's dynamic-import microtasks
// never touch m_asyncContextData, so we thread it through the module key.
JSC::JSString* asyncContextKey = nullptr;
if (globalObject->isAsyncContextTrackingEnabled()) {
JSC::JSValue asyncContext = globalObject->m_asyncContextData.get()->getInternalField(0);
if (!asyncContext.isUndefined()) {
JSC::JSMap* map = globalObject->m_pendingDynamicImportAsyncContexts.get();
if (!map) {
map = JSC::JSMap::create(vm, globalObject->mapStructure());
globalObject->m_pendingDynamicImportAsyncContexts.set(vm, globalObject, map);
}
asyncContextKey = jsString(vm, resolvedIdentifier.string());
map->set(globalObject, asyncContextKey, asyncContext);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
// The C++ module loader now extracts `with.type` into a
// ScriptFetchParameters before calling this hook, so `parameters` is
// already the parsed RefPtr (or null). Just forward it.
Expand All @@ -3597,6 +3617,14 @@
return JSC::JSPromise::rejectedPromiseWithCaughtException(globalObject, scope);
}

// Only a still-pending import reaches moduleLoaderEvaluate to consume the
// entry above; an already-evaluated (cached) module settles synchronously
// and never re-evaluates, so drop its entry to avoid retaining the context.
if (asyncContextKey && (!result || result->status() != JSC::JSPromise::Status::Pending)) {
if (auto* map = globalObject->m_pendingDynamicImportAsyncContexts.get())
map->remove(globalObject, asyncContextKey);
}

Check failure on line 3626 in src/jsc/bindings/ZigGlobalObject.cpp

View check run for this annotation

Claude / Claude Code Review

Map entry leaks when dynamic import rejects asynchronously

The map entry inserted at line 3607 is only removed when `moduleLoaderEvaluate` runs for the key or when the import promise has *already* settled here — if `importModule` returns a Pending promise that later **rejects asynchronously** (syntax error in the imported file, a transitive static dependency that fails to resolve/parse, or an async `Bun.plugin` `onLoad` rejection), neither path fires and the entry is retained for the lifetime of the global, pinning the captured async context and everyth
Comment thread
robobun marked this conversation as resolved.

ASSERT(result);
return result;
}
Expand Down Expand Up @@ -3703,13 +3731,45 @@
return Zig::ImportMetaObject::create(globalObject, key);
}

// Reinstate the AsyncLocalStorage context captured at this module's import()
// call site (see moduleLoaderImportModule) around its synchronous top-level
// evaluation. JSC drives dynamic-import evaluation from an internal microtask
// that never restores m_asyncContextData, so getStore() would otherwise be
// undefined during module init (#32693). A top-level-await module's post-await
// continuations resume through JSC's async-module machinery outside this hook
// and are not covered here.
static JSC::JSValue evaluateModuleWithCapturedAsyncContext(Zig::GlobalObject* globalObject,
JSModuleLoader* moduleLoader, JSValue key, JSValue moduleRecordValue,
RefPtr<JSC::ScriptFetcher>&& scriptFetcher, JSValue sentValue, JSValue resumeMode)
{
auto& vm = JSC::getVM(globalObject);

JSC::JSValue capturedAsyncContext;
if (auto* map = globalObject->m_pendingDynamicImportAsyncContexts.get()) {
if (map->size() && map->has(globalObject, key)) {
capturedAsyncContext = map->get(globalObject, key);
map->remove(globalObject, key);
}
}

if (!capturedAsyncContext || capturedAsyncContext.isUndefined() || !globalObject->isAsyncContextTrackingEnabled())
return moduleLoader->evaluateNonVirtual(globalObject, key, moduleRecordValue, WTF::move(scriptFetcher), sentValue, resumeMode);

auto* asyncContextData = globalObject->m_asyncContextData.get();
JSC::JSValue restoreAsyncContext = asyncContextData->getInternalField(0);
asyncContextData->putInternalField(vm, 0, capturedAsyncContext);
JSC::JSValue result = moduleLoader->evaluateNonVirtual(globalObject, key, moduleRecordValue, WTF::move(scriptFetcher), sentValue, resumeMode);
asyncContextData->putInternalField(vm, 0, restoreAsyncContext);
return result;
}

JSC::JSValue GlobalObject::moduleLoaderEvaluate(JSGlobalObject* lexicalGlobalObject,
JSModuleLoader* moduleLoader, JSValue key,
JSValue moduleRecordValue, RefPtr<JSC::ScriptFetcher> scriptFetcher,
JSValue sentValue, JSValue resumeMode)
{
return moduleLoader->evaluateNonVirtual(lexicalGlobalObject, key, moduleRecordValue,
WTF::move(scriptFetcher), sentValue, resumeMode);
return evaluateModuleWithCapturedAsyncContext(uncheckedDowncast<Zig::GlobalObject>(lexicalGlobalObject),
moduleLoader, key, moduleRecordValue, WTF::move(scriptFetcher), sentValue, resumeMode);
}

extern "C" bool Bun__VM__specifierIsEvalEntryPoint(void*, EncodedJSValue);
Expand All @@ -3724,8 +3784,8 @@
auto& vm = globalObject->vm();
auto scope = DECLARE_THROW_SCOPE(vm);

JSC::JSValue result = moduleLoader->evaluateNonVirtual(lexicalGlobalObject, key, moduleRecordValue,
WTF::move(scriptFetcher), sentValue, resumeMode);
JSC::JSValue result = evaluateModuleWithCapturedAsyncContext(globalObject, moduleLoader, key,
moduleRecordValue, WTF::move(scriptFetcher), sentValue, resumeMode);
// The new C++ loader propagates the module body's throw out of
// evaluateNonVirtual; the old JS-side ModuleLoader.js swallowed it before
// dispatching here. Don't call back into native code (which opens an
Expand Down
5 changes: 5 additions & 0 deletions src/jsc/bindings/ZigGlobalObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace JSC {
class Structure;
class Identifier;
class JSMap;
class LazyClassStructure;
class ScriptFetcher;
class ScriptFetchParameters;
Expand Down Expand Up @@ -502,6 +503,10 @@ class GlobalObject : public Bun::GlobalScope {
\
V(public, WriteBarrier<Bun::JSNextTickQueue>, m_nextTickQueue) \
\
/* AsyncLocalStorage context captured at an import() call site, keyed by resolved module key, */ \
/* reinstated around the dynamically imported module's top-level evaluation (#32693). */ \
V(public, WriteBarrier<JSC::JSMap>, m_pendingDynamicImportAsyncContexts) \
\
/* WriteBarrier<Unknown> m_JSBunDebuggerValue; */ \
V(private, ThenablesArray, m_thenables) \
\
Expand Down
93 changes: 92 additions & 1 deletion test/js/node/async_hooks/AsyncLocalStorage.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { AsyncLocalStorage, AsyncResource } from "async_hooks";
import { describe, expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";
import { bunEnv, bunExe, tempDir } from "harness";

describe("AsyncLocalStorage", () => {
test("throw inside of AsyncLocalStorage.run() will be passed out", () => {
Expand Down Expand Up @@ -567,3 +567,94 @@ describe("async context passes through", () => {
expect(a).toBe("value");
});
});

describe("dynamic import() preserves the AsyncLocalStorage context (#32693)", () => {
test("during the imported module's top-level evaluation", async () => {
using dir = tempDir("als-dynamic-import", {
"store.mjs": `
import { AsyncLocalStorage } from 'node:async_hooks';
export const store = new AsyncLocalStorage();
`,
// Imported lazily from inside store.run(). Its top-level code (module
// evaluation) must observe the store that was active at the import() site,
// matching Node. A nested run() inside the body must still scope correctly
// and restore to the imported context afterwards.
"imported.mjs": `
import { store } from './store.mjs';
console.log("eval:" + store.getStore());
store.run("NESTED", () => {
console.log("nested:" + store.getStore());
});
console.log("after-nested:" + store.getStore());
`,
// Imported with no active context: must evaluate with an undefined store,
// proving the captured context does not leak into unrelated imports.
"no-context.mjs": `
import { store } from './store.mjs';
console.log("no-context-eval:" + store.getStore());
`,
"index.mjs": `
import { store } from './store.mjs';
await store.run('CONTEXT', () => import('./imported.mjs'));
console.log("after-import:" + store.getStore());
await import('./no-context.mjs');
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "index.mjs"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout).toBe(
[
"eval:CONTEXT",
"nested:NESTED",
"after-nested:CONTEXT",
"after-import:undefined",
"no-context-eval:undefined",
"",
].join("\n"),
);
expect(exitCode).toBe(0);
expect(stderr).not.toContain("CONTEXT");
});

// A top-level-await module resumes through JSC's async-module machinery, which
// runs outside this hook; only the synchronous prefix observes the context. The
// guarantee here is that capturing the context does not crash that path.
test("a top-level-await module sees the context during its synchronous prefix and does not crash", async () => {
using dir = tempDir("als-dynamic-import-tla", {
"store.mjs": `
import { AsyncLocalStorage } from 'node:async_hooks';
export const store = new AsyncLocalStorage();
`,
"tla.mjs": `
import { store } from './store.mjs';
console.log("tla-sync:" + store.getStore());
await Promise.resolve();
console.log("done");
`,
"index.mjs": `
import { store } from './store.mjs';
await store.run('CONTEXT', () => import('./tla.mjs'));
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "index.mjs"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout).toBe("tla-sync:CONTEXT\ndone\n");
expect(exitCode).toBe(0);
});
});
Loading