-
Notifications
You must be signed in to change notification settings - Fork 5k
module_loader: generate bun:main wrapper on demand instead of caching it #32493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, isDebug, tempDir } from "harness"; | ||
| import { bunEnv, bunExe, isDebug, isWindows, tempDir } from "harness"; | ||
| import { writeFileSync } from "node:fs"; | ||
| import { join } from "node:path"; | ||
|
|
||
|
|
@@ -13,7 +13,9 @@ import { join } from "node:path"; | |
| // free-then-reallocate on each reload. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The file-header comment here (lines 6-13) and the Extended reasoning...What's staleTwo comments in
Why they're wrong after this PRStep through the new code:
Both comments therefore describe a mechanism that no longer exists — and not incidentally: removing that mechanism is the entire point of this PR. Why it's in scope for this PRThe PR already edits this exact file: it changes the imports, rewrites ImpactNo runtime effect — this is documentation only. But the stale comments are actively misleading: they point at a field ( Suggested fixRewrite both comments to match the new model, e.g.:
|
||
|
|
||
| function stripAsanWarning(stderr: string): string[] { | ||
| return stderr.split("\n").filter(l => l.length > 0 && !l.startsWith("WARNING: ASAN interferes")); | ||
| return stderr | ||
| .split("\n") | ||
| .filter(l => l.length > 0 && !l.startsWith("WARNING: ASAN interferes") && !l.startsWith("debug warn:")); | ||
| } | ||
|
|
||
| test.concurrent("dynamic import('bun:main') returns the wrapper module", async () => { | ||
|
|
@@ -83,6 +85,71 @@ test.concurrent("import('bun:main') from a preload (before the module map is pop | |
| }); | ||
| }); | ||
|
|
||
| // Sentry BUN-36H7 / https://github.com/oven-sh/bun/issues/27192: | ||
| // import("bun:main") inside a compiled standalone executable faulted in | ||
| // getHardcodedModule reading the stored `entry_point.contents` slice | ||
| // (SEGV at a page-aligned high address, i.e. a freed mimalloc segment). | ||
| // The wrapper source is now regenerated on demand from `vm.main` at fetch | ||
| // time, so there is no stored buffer that can go stale between | ||
| // `reload_entry_point` and the fetch. This exercises the exact crash | ||
| // scenario: the bundled entry both (a) is reached via the initial | ||
| // bun:main fetch at boot and (b) explicitly re-imports bun:main after the | ||
| // top-level evaluation has completed. | ||
|
Comment on lines
+90
to
+99
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit: per repo convention (root CLAUDE.md), regression-test comments should be just the issue URL — the Sentry signature, freed-mimalloc diagnosis, and fix mechanism here all duplicate the PR description and will drift as the implementation changes. Suggest reducing this block to Extended reasoning...WhatRoot
The new comment block at Why it mattersThe rule exists because bug-history comments go stale. This one already references implementation internals that this very PR removes — The same file does have a pre-existing long bug-history comment at lines 6-13 (from #29450), so the author was following file-local precedent — but adding another instance compounds the problem rather than fixing it, and CLAUDE.md is unambiguous that this is a hard rule, not a suggestion. Step-by-step
FixReplace lines 90-99 with: // https://github.com/oven-sh/bun/issues/27192Optionally also trim the new source comment at |
||
| test.concurrent( | ||
| "import('bun:main') in a compiled standalone executable", | ||
| async () => { | ||
| using dir = tempDir("bun-main-compile", { | ||
| "package.json": "{}", | ||
| "entry.mjs": ` | ||
| // bun:main statically imports this file, so awaiting it at the top | ||
| // level would be a TLA self-cycle. Defer to a task so bun:main | ||
| // finishes evaluating first, then re-import it from user code. | ||
| setImmediate(async () => { | ||
| try { | ||
| Bun.gc(true); | ||
| const m = await import("bun:main"); | ||
| if (m[Symbol.toStringTag] !== "Module") throw new Error("expected module namespace"); | ||
| const keys = Object.keys(m); | ||
| if (keys.length !== 0) throw new Error("expected empty wrapper namespace, got keys: " + keys.join(",")); | ||
| console.log("OK"); | ||
| } catch (e) { | ||
| console.error(String(e)); | ||
| process.exit(1); | ||
| } | ||
| }); | ||
| `, | ||
| }); | ||
| const outfile = join(String(dir), isWindows ? "out.exe" : "out"); | ||
| { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "build", "--compile", "./entry.mjs", "--outfile", outfile], | ||
| 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({ stderr, exitCode }).toEqual({ stderr: expect.not.stringContaining("error:"), exitCode: 0 }); | ||
| void stdout; | ||
| } | ||
| await using proc = Bun.spawn({ | ||
| cmd: [outfile], | ||
| 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, stderr: stripAsanWarning(stderr), exitCode, signalCode: proc.signalCode }).toEqual({ | ||
| stdout: "OK\n", | ||
| stderr: [], | ||
| exitCode: 0, | ||
| signalCode: null, | ||
| }); | ||
| }, | ||
| isDebug ? 120_000 : 60_000, | ||
| ); | ||
|
|
||
| test.concurrent( | ||
| "ServerEntryPoint regenerates cleanly across --hot reloads", | ||
| async () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡
ServerEntryPoint::generate()now ignores both_is_hot_reload_enabledand_path_to_useand unconditionally returnsOk(()), leaving dead parameters and an infallibleResultreturn type. Per CLAUDE.md ("Delete dead code in the same PR that makes it dead … parameters discarded in the body"), drop the dead params and change the return to(); thejsc_hooks.rswrapper can then stop threadingwatch/entry_pathand stop calling.is_ok()on a never-Errvalue (theRuntimeHooksfn-pointer signature can adapt in the wrapper if you'd rather not touchVirtualMachine.rs).Extended reasoning...
What changed
Before this PR,
ServerEntryPoint::generate()consumedis_hot_reload_enabledandpath_to_useto render the wrapper source intoentry.contents, and thewrite!calls could (nominally) fail, justifyingResult<(), bun_core::Error>. After this PR the rendering moved to the new puregenerate_source(), andgenerate()(src/bundler/entry_points.rs:267-275) is reduced to:Both non-
selfparameters are discarded (underscore-prefixed) and the return is unconditionallyOk(()).Why it's flagged
CLAUDE.md:239 states this as required scope, not optional cleanup:
This PR is exactly the change that made these parameters dead, so per the project's own contributing guide they should be removed here rather than left for a follow-up.
Dead plumbing in the caller chain
Walking outward from
generate():src/runtime/jsc_hooks.rs:608-616—generate_entry_point(_vm, watch, entry_path) -> boolthreadswatchandentry_paththrough solely to discard them, and calls.is_ok()on aResultthat can never beErr. The doc comment on this function ("Returnsfalseon error (the error is already logged intovm.logbygenerate)") is now also stale —generateno longer logs anything and never fails.src/jsc/VirtualMachine.rs:1639— theRuntimeHooks.generate_entry_pointfn-pointer signature carrieswatch: bool, entry_path: &[u8]and returnsbool.src/jsc/VirtualMachine.rs:2289-2291— the call site branches on thebooland has anErr(bun_core::err!("ServerEntryPointGenerate"))arm that is now unreachable (the only condition under which the hook returnsfalseisruntime_state().is_null(), which would already have prevented the VM from booting).Step-by-step proof it's dead
generate()has exactly one caller:generate_entry_pointat jsc_hooks.rs:615 (verified by grep —ServerEntryPoint::generate(appears nowhere else).watchandentry_pathstraight through; neither is read anywhere else in the wrapper.generate()'s body contains no?, noreturn Err, no fallible call — onlyentry.generated = true; Ok(()). So.is_ok()at jsc_hooks.rs:615 is staticallytrue.generate_entry_pointreturnstruewheneverruntime_state()is non-null, and thewatch/entry_patharguments contribute nothing to its result or side effects.Impact
No runtime impact — this is purely dead surface area. But it leaves a misleading API: a reader of
RuntimeHooks.generate_entry_point's signature and doc comment will assume entry-point generation can fail and depends onwatch/entry_path, when in fact the real inputs are now read at fetch time inget_hardcoded_moduleviavm.main()/vm.is_watcher_enabled().Suggested fix
Minimal (keeps the cross-crate
RuntimeHooksvtable shape untouched):and in
jsc_hooks.rs:Full cleanup (preferred, since
RuntimeHooksis an in-tree Rust fn-pointer table with a single implementation, not a stable C ABI): change the vtable entry tofn(vm: &VirtualMachine), drop thewatch/entry_pathplumbing and the unreachableServerEntryPointGenerateerror branch at VirtualMachine.rs:2289-2291, and update the stale doc comments at jsc_hooks.rs:605-607 and VirtualMachine.rs:1636-1638.