Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
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
7 changes: 7 additions & 0 deletions src/jsc/bindings/ModuleLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,13 @@ extern "C" void Bun__onFulfillAsyncModule(
JSC::JSPromise* promise = uncheckedDowncast<JSC::JSPromise>(JSC::JSValue::decode(encodedPromiseValue));

if (!res->success) {
// The module failed to load and will never evaluate, so no
// moduleLoaderEvaluate call will consume a dynamic-import async context
// captured for it (#32693); drop it here so it can't pin the store.
if (auto* map = globalObject->m_pendingDynamicImportAsyncContexts.get()) {
if (map->size())
map->remove(globalObject, JSC::jsString(vm, specifier->toWTFString(BunString::ZeroCopy)));
}
RELEASE_AND_RETURN(scope, promise->reject(vm, JSValue::decode(res->result.err.value)));
}

Expand Down
86 changes: 82 additions & 4 deletions src/jsc/bindings/ZigGlobalObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3483,6 +3483,37 @@
}
}

// Record the AsyncLocalStorage context active at an import() call site, keyed by
// the resolved module key, so the imported module's top-level evaluation can run
// with it (see evaluateModuleWithCapturedAsyncContext). JSC's dynamic-import
// microtasks never restore m_asyncContextData; Node preserves it via V8's
// continuation-preserved embedder data. Returns the key to remove on cleanup, or
// null when nothing was recorded.
static JSC::JSString* captureDynamicImportAsyncContext(Zig::GlobalObject* globalObject, JSC::VM& vm, const JSC::Identifier& resolvedIdentifier)
{
if (!globalObject->isAsyncContextTrackingEnabled())
return nullptr;
JSC::JSValue asyncContext = globalObject->m_asyncContextData.get()->getInternalField(0);
if (asyncContext.isUndefined())
return nullptr;
JSC::JSMap* map = globalObject->m_pendingDynamicImportAsyncContexts.get();
if (!map) {
map = JSC::JSMap::create(vm, globalObject->mapStructure());
globalObject->m_pendingDynamicImportAsyncContexts.set(vm, globalObject, map);
}
JSC::JSString* key = jsString(vm, resolvedIdentifier.string());
map->set(globalObject, key, asyncContext);
return key;
}

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

View check run for this annotation

Claude / Claude Code Review

Missing exception checks after new JSMap operations (x64-asan CI failure)

The new `JSMap` operations (`set`/`has`/`get`/`remove`) all open a `ThrowScope` in `JSOrderedHashTable`, but none of the call sites this PR adds follow with an exception check — this is the x64-asan CI failure ("unchecked exception at requestImportModule … thrown from JSOrderedHashTable.h:83") on `bun-server.test.ts` and `AsyncLocalStorage.test.ts`. Since these are JSString-keyed ops on an internal builtin map that provably cannot throw, declare a `ThrowScope` in the helpers and add `scope.asser
Comment thread
robobun marked this conversation as resolved.

static void dropDynamicImportAsyncContext(Zig::GlobalObject* globalObject, JSC::JSString* key)
{
if (!key)
return;
if (auto* map = globalObject->m_pendingDynamicImportAsyncContexts.get())
map->remove(globalObject, key);
}

JSC::JSPromise* GlobalObject::moduleLoaderImportModule(JSGlobalObject* jsGlobalObject,
JSModuleLoader*,
JSString* moduleNameValue,
Expand Down Expand Up @@ -3532,10 +3563,14 @@
if (auto resolution = globalObject->onLoadPlugins.resolveVirtualModule(moduleName, sourceURL.protocolIsFile() ? sourceOriginStringHolder : String())) {
resolvedIdentifier = JSC::Identifier::fromString(vm, resolution.value());

JSC::JSString* asyncContextKey = captureDynamicImportAsyncContext(globalObject, vm, resolvedIdentifier);
auto result = JSC::importModule(globalObject, resolvedIdentifier, JSC::Identifier(), parameters, nullptr, /* deferred */ false, referrerAsyncOrder);
if (scope.exception()) [[unlikely]] {
dropDynamicImportAsyncContext(globalObject, asyncContextKey);
return JSC::JSPromise::rejectedPromiseWithCaughtException(globalObject, scope);
}
if (asyncContextKey && (!result || result->status() != JSC::JSPromise::Status::Pending))
dropDynamicImportAsyncContext(globalObject, asyncContextKey);
return result;
}
}
Expand Down Expand Up @@ -3588,15 +3623,26 @@
sourceOriginZ.deref();
}

JSC::JSString* asyncContextKey = captureDynamicImportAsyncContext(globalObject, vm, resolvedIdentifier);

// 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.
auto result = JSC::importModule(globalObject, resolvedIdentifier,
JSC::Identifier(), WTF::move(parameters), nullptr, /* deferred */ false, referrerAsyncOrder);
if (scope.exception()) [[unlikely]] {
dropDynamicImportAsyncContext(globalObject, asyncContextKey);
return JSC::JSPromise::rejectedPromiseWithCaughtException(globalObject, scope);
}

// Only a still-pending import reaches moduleLoaderEvaluate to consume the
// entry; a cached (already-evaluated) module settles synchronously and never
// re-evaluates. A pending import that later rejects without evaluating is
// cleaned up at its fetch-failure seam (Bun__onFulfillAsyncModule) or, for
// transitive failures, overwritten on the next import of the same key.
if (asyncContextKey && (!result || result->status() != JSC::JSPromise::Status::Pending))
dropDynamicImportAsyncContext(globalObject, asyncContextKey);
Comment thread
coderabbitai[bot] marked this conversation as resolved.

ASSERT(result);
return result;
}
Expand Down Expand Up @@ -3703,13 +3749,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 +3802,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
128 changes: 127 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,129 @@ 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);
});

// A dynamic import that fails to load never evaluates, so the captured context
// is cleaned up at the fetch-failure seam instead of being retained. Exercises
// that path and confirms it neither crashes nor disturbs a later import.
test("a dynamic import that fails to load is caught and does not disturb later imports", async () => {
using dir = tempDir("als-dynamic-import-fail", {
"store.mjs": `
import { AsyncLocalStorage } from 'node:async_hooks';
export const store = new AsyncLocalStorage();
`,
"bad.mjs": `export const x = ;`,
"good.mjs": `
import { store } from './store.mjs';
console.log("good-eval:" + store.getStore());
`,
"index.mjs": `
import { store } from './store.mjs';
const outcome = await store.run('A', () => import('./bad.mjs').then(() => 'loaded', () => 'caught'));
console.log("bad-import:" + outcome);
await store.run('B', () => import('./good.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("bad-import:caught\ngood-eval:B\n");
expect(exitCode).toBe(0);
});
});
Loading