-
Notifications
You must be signed in to change notification settings - Fork 4.7k
resolver: add bun:main to HardcodedModule.Alias (fix flaky test) #29719
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🟡 Adding
bun:maintobun_aliasesmakesprocess.getBuiltinModule("bun:main")reachgetHardcodedModule, which clonesentry_point.contentsinto a freshbun.Stringand returns it with.tag = .esm; the C++ side (ModuleLoader.cpp,case SyntheticModuleType::ESM: return {};) then drops theErrorableResolvedSourcewithout deref'ingsource_code, leaking one copy of the wrapper source per call. Tiny leak via an obscure API, but it's a new path opened by this PR — easiest fix is to derefres.result.value.source_codebeforereturn {}in the ESM case (or have thebun:mainbranch returnnullwhen called fromBun__resolveAndFetchBuiltinModule).Extended reasoning...
What the bug is
process.getBuiltinModule("bun:main")now allocates a heap-backedWTF::StringImpl(a clone ofjsc_vm.entry_point.contents) and discards it without decrementing its refcount. Each call leaks one copy of the entry-point wrapper source.Before this PR,
Bun__resolveAndFetchBuiltinModulereturnedfalsefor"bun:main"(it wasn't inHardcodedModule.Alias.bun_aliases), soprocess.getBuiltinModule("bun:main")returnedundefinedwithout allocating anything. Adding the alias opens a code path that was never exercised by this caller.Code path that triggers it
process.getBuiltinModuleis bound directly toProcess_functionLoadBuiltinModule(BunProcess.cpp:4032) — there is no JS-side cache in front of it.Process_functionLoadBuiltinModule(BunProcess.cpp:3913) callsBun::resolveAndFetchBuiltinModule(ModuleLoader.cpp:577), which declares a stack-localErrorableResolvedSource resand callsBun__resolveAndFetchBuiltinModule(&res, ...).ModuleLoader.zig:837),HardcodedModule.Alias.bun_aliases.get("bun:main")now succeeds (the line added by this PR).HardcodedModule.map.get("bun:main")(line 839) yields.@"bun:main", andgetHardcodedModuleis called (line 844).getHardcodedModulefor.@"bun:main"(ModuleLoader.zig:1148-1155), whenjsc_vm.entry_point.generatedis true (the normal case forbun run script), returns:cloneUTF8→toBunStringComptimeheap-allocates a fresh ref-countedWTF::StringImpl.ModuleLoader.cpp:605-607), the switch onres.result.value.taghits:jsUndefined()to the caller.Why existing code doesn't prevent it
ErrorableResolvedSourceis a plaintypedef struct(headers-handwritten.h:132-135) with no destructor, andBunStringis a tagged value type with no RAII cleanup. TheSyntheticModuleType::ESMcase inresolveAndFetchBuiltinModulesimply returns without touchingres.result.value.source_code, so the cloned string's refcount stays at 1 forever.The other non-registry builtin reachable on this path,
bun:wrap, usesString.initon a static buffer (no heap allocation) and.tag = .javascript(hits thedefault:branch, alsoreturn {}), so it never tickled this.bun:mainis the first alias whosegetHardcodedModulebranch heap-allocates and is then thrown away byresolveAndFetchBuiltinModule.Step-by-step proof
Under
bun run /tmp/x.mjs:ServerEntryPoint.generate()setsentry_point.generated = trueandentry_point.contents = "import '/tmp/x.mjs';\n..."(~a few hundred bytes).process.getBuiltinModule("bun:main").cloneUTF8allocates aWTF::StringImplof those ~hundreds of bytes, refcount = 1.case ESM: return {};—resgoes out of scope, no destructor runs, theStringImplis leaked.Process_functionLoadBuiltinModulereturnsjsUndefined().for (;;) process.getBuiltinModule("bun:main");→ RSS grows unbounded.Before this PR, step 3 returned
null→return falseatModuleLoader.zig:837, so step 4 never ran.Impact
A small per-call memory leak (one wrapper-source string, typically a few hundred bytes) on every call to
process.getBuiltinModule("bun:main"). The call also returnsundefined, so users have no reason to invoke it repeatedly — impact in real programs is negligible. It is, however, a genuine regression introduced by this PR rather than a pre-existing issue.How to fix
Either:
ModuleLoader.cpp'sresolveAndFetchBuiltinModule, derefres.result.value.source_codebeforereturn {}in theESM(anddefault) cases whensource_code_needs_derefis set; orBun__resolveAndFetchBuiltinModule(ModuleLoader.zig), special-case.@"bun:main"toreturn falsebefore callinggetHardcodedModule, since this caller can't do anything useful with an ESM-tagged source anyway.