Repository navigation
Conversation
GlobalEventScope refs the event loop while the global scope has a
'message' listener, so a worker stays alive to receive messages from
its parent. The main thread used the same rule. It has no parent, so
nothing can ever dispatch to that listener, and `self.onmessage = fn`
or `addEventListener("message", fn)` kept the process alive forever.
Take the ref only when the VM belongs to a worker.
Fixes #24256
Co-authored-by: 0xfandom <kashyapshivank01@gmail.com>
|
Warning Review limit reached
On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file. Or wait 1 minute for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 11:42 AM PT - Sep 6th, 2026
⏳ @robobun, your commit db2b5ee is still building in |
|
Status: ready for review. Reproduced on 1.4.3-canary (d316760) with: // keepalive.mjs
self.onmessage = e => { console.log("got", e.data); };
console.log("handler installed");
CI (build 111657): |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the isWorkerVM() gate is set once at VM creation and never changes, so Add/Remove/Clear stay balanced by construction.
What was reviewed:
- Refcount balance:
m_isWorkerVMis assigned once inJSVMClientData::createand immutable, so everyAddthat refs has a matchingRemove/Clearthat passes the same gate. - Both entry paths (
self.onmessage =andaddEventListener("message", ...)) are covered by the main-thread test; the worker test guards the preserved keep-alive behavior throughremoveEventListener. - Tests follow harness conventions (
test.concurrent,await using, concurrent pipe drain, output asserted before exit code,tempDir/bunExe/bunEnv).
Extended reasoning...
Overview
This PR changes GlobalEventScope::onDidChangeListenerImpl in src/jsc/bindings/GlobalEventScope.cpp to only ref/unref the event loop for global "message" listeners when the VM is a worker VM. Previously, adding self.onmessage or addEventListener("message", ...) on the main thread would keep the process alive forever even though nothing can dispatch to that listener. Two tests are added to test/js/web/web-globals.test.js: one asserting the main thread exits on its own with a global message listener installed, and one asserting a worker still stays alive until its message listener is removed.
Security risks
None. This is event-loop keep-alive bookkeeping with no untrusted input parsing, no auth/crypto, and no filesystem or network surface. The only observable effect is whether an idle main-thread process with a dangling message listener exits or hangs.
Level of scrutiny
Low-to-moderate. The diff is ~25 lines of C++ that adds a single guard and otherwise reflows the existing switch to early-return style. The critical question for any refcount change is balance across all terminal paths: here the gate (clientData(vm)->isWorkerVM()) is a per-VM boolean set exactly once in JSVMClientData::create (m_isWorkerVM = !!worker) and never mutated, so any Add that took a ref is guaranteed to have its corresponding Remove/Clear pass the same gate. The Clear arm's semantics are unchanged. The !context null check is defensive and harmless.
Other factors
The tests are well-constructed per repo conventions: placed in the existing web-globals.test.js alongside sibling worker/onmessage tests, use test.concurrent with await using spawns, drain stdout/stderr/exited via Promise.all, assert output before exit code, and cover both the setter and addEventListener paths on the main thread. The main-thread test would hang on stock Bun (satisfying the USE_SYSTEM_BUN=1 must-fail rule), and the worker test guards against regressing the original #4114 behavior. No CODEOWNERS cover these paths, and there are no outstanding third-party review objections in the timeline.
|
Rebased #32482 onto current main, so it applies to Verified locally: the two So if you're still up for it, closing this one in favor of #32482 works for me. Thanks for the heads up about the rename. |
|
This also fixes #42512 (same hang, reported against 1.4.2). That issue adds a real world case: |
|
Closing in favor of #32482, as offered there on 2026-09-06. @0xfandom rebased #32482 onto current main and took the The two I built #32482 on current main (09bb546) and ran the tests of both PRs on that build. All of them pass, including the two tests from this PR. Without the The branch |
Problem
self.onmessage = fnoraddEventListener("message", fn)keeps the process alive forever. Nothing can dispatch to that listener (main-threadpostMessage()has no target), so the process hangs. Node exits with code 0. Fixes Bun process doesn’t exit whenglobalThis.onmessageis set #24256.GlobalEventScope::onDidChangeListenerImpl(src/jsc/bindings/GlobalEventScope.cpp:16). It refs the event loop while the global scope has amessagelistener. Fix worker event loop ref/unref + leak #4114 added that rule for workers, and the main-thread global shares the same class.Fix
clientData(vm)->isWorkerVM()is true. A worker keeps the existing behavior: a globalmessagelistener holds it open until the listener is removed.WorkerMessagingProxy) that dispatchesmessageevents onto the global scope. The flag is per VM, so it also covers aShadowRealmor other extra global created on the main thread, whichScriptExecutionContext::isMainThread()(identifier== 1) does not.test/js/web/web-globals.test.js(two new tests, the main-thread one times out on stock bun). Also rantest/js/web/workers/worker.test.ts,worker-terminate-lifetime.test.ts, andworker-late-completion.test.ts.Background
GlobalEventScopeis theEventTargetbehindglobalThis.addEventListener/self.onmessage. EveryZig::GlobalObjectowns one, on the main thread and in workers.EventTarget::onDidChangeListeneris a hook that fires on each listener add, remove, or clear.MessagePortandGlobalEventScopeuse it to ref or unref the event loop so that a pendingmessagelistener keeps the thread alive.JSVMClientData::isWorkerVM()is set once inJSVMClientData::createfrom whether aWorkerMessagingProxycreated the VM.Notes
isMainThread(). It still targetsBunWorkerGlobalScope.cpp, which Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075 renamed toGlobalEventScope.cpp, so it has conflicted since then. This PR carries the same idea onto the renamed file and credits the author in the commit.postMessage(x)stays a no-op that returnsundefined. Whether it should throw or self-dispatch (browserwindow.postMessagesemantics) is a separate decision.terminate() while fs.readFile completions keep arriving(5 s timeout under debug ASAN) and thedns.lookup()LSAN case (thenode:fsBindingbox, covered by theBun::generateInternalModulesuppression when the stack is deep enough). Neither touches a main-thread global listener. Latest main CI (build 111080) does not list them.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/web-globals.test.js