Fire the close event on MessagePort#32565
Conversation
Closing one end of a MessageChannel (or a node:worker_threads MessagePort) now dispatches a `close` event on both the port that was closed and its entangled peer. It is delivered asynchronously, after any already-queued messages on that port have drained, matching Node's node:worker_threads semantics and the HTML MessagePort `close` event. Previously the event never fired: close() marked the side closed and removed listeners without notifying the peer or dispatching `close`, so code awaiting `close` (and the process staying alive for it) missed it and exited early. - MessagePortPipe::close() wakes the entangled peer (after its inbox drains) so it can emit `close`. - MessagePortPipe::drainAndDispatch() dispatches `close` once the inbox empties and the peer has closed. - MessagePort::close() schedules the closing port's own `close` event before tearing down its listeners. - hasPendingActivity() keeps the wrapper alive until a pending `close` has been delivered. Fixes #32563
|
Updated 12:32 PM PT - Jun 21st, 2026
✅ @robobun, your commit 26cb6e43057cc351b1224e31af262c70dd2df1d7 passed in 🧪 To try this PR locally: bunx bun-pr 32565That installs a local version of the PR into your bun-32565 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Thanks, but I checked each of these against this branch's build and none are fixed here, so I'm not adding the
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesMessagePort async close event dispatch
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/workers/message-port-pipe.test.ts`:
- Around line 412-419: Reorder the assertions in the test to place stdout
assertions before the exitCode check. Move the expect(exitCode).toBe(0)
assertion to the end of the test block, after all the log assertions (after the
final expect statement that checks log.indexOf("port2 message:
foobar")).toBeLessThan). This ensures that if the test fails, the log contents
will be visible in the error output, making debugging easier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7879bcc-9add-49a3-a1a0-3693aa371090
📒 Files selected for processing (4)
src/jsc/bindings/webcore/MessagePort.cppsrc/jsc/bindings/webcore/MessagePort.hsrc/jsc/bindings/webcore/MessagePortPipe.cpptest/js/web/workers/message-port-pipe.test.ts
|
On the duplicate check:
This PR is a small, focused fix for just #32563 against the current pipe-based code, so it can land on its own if the larger overhaul is still in flight. Happy to defer to maintainers on ordering, and to close this if #31216 supersedes it. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Two leaks in the initial close-event implementation, both pinning the JS wrapper for the lifetime of the context: - A close listener added after close() (when there was no listener at close time) set m_hasCloseEventListener while the pipe side was already Closed, and hasPendingActivity()'s close branch (which runs before the detached check) then pinned the wrapper forever. close() now marks the close consumed when no dispatch is scheduled, and hasPendingActivity() pins only while a close is actually pending: a scheduled self-close, or an attached port whose peer has closed. - A port with only a close listener was never attached to the pipe, so the peer's close() never woke it (close never fired, diverging from Node) and hasPendingActivity() pinned it. A close listener now attaches the port without starting message delivery, and attach() schedules a drain when the peer has already closed so a late close listener still fires. The drain leaves messages buffered for a port with no message listener rather than dispatching them to no one.
…e-pending
Fixes a leak caught by CI and two issues raised in review, all in the
close-event paths:
- Leak (CI): ~MessagePort runs during worker VM teardown and called
MessagePortPipe::close(), whose peer-wake scheduled a drain task that
never runs once the event loop is gone (and pins a Ref to the pipe,
cascading the leak). The peer-wake is moved out of the pipe's close()
into MessagePortPipe::wakePeerForClose(), which MessagePort::close()
invokes only when the context can still run JS. Destructor and
teardown paths no longer schedule anything.
- hasPendingActivity() no longer pins a close-only port that is sitting
on undelivered buffered messages: with no message listener the inbox
never drains, so the drain has no path to dispatch 'close' (it stays
blocked behind the messages, matching Node). Pin only when a message
listener exists or the inbox is already empty.
- close() followed by on('close') on the same port now fires: the
self-close task is posted whenever JS can run rather than only when a
close listener already exists. The task is a no-op dispatch with no
listener and then tears the port down, so the wrapper is still
collected.
…y unstarted ports Addresses two more wrapper-leak regressions and a stale comment from review: - hasPendingActivity() pinned a port whenever its peer was Closed and it had a close listener, assuming a drain was in flight to dispatch the close. That only holds when the peer closed via MessagePort::close() (wakePeerForClose) or an attach after the peer closed. A peer closed via ~MessagePort, ~TransferredMessagePort (dropped in transit), or contextDestroyed() with canRunScript()==false never schedules a drain, so the close could never fire and the wrapper leaked. Gate the peer sub-clause on DrainScheduled: the pin now exists only while the drain that would dispatch the close does. - The drain buffered messages for any port with no message listener, keyed on m_hasMessageEventListener. An explicitly started() port with no listener (port.start(), or onmessage set then cleared) then stranded the buffered message and leaked via the message-listener pin, because re-adding a listener calls start() which no-ops once m_started is set. Key the buffer decision on !started() instead: an unstarted (close-only) port buffers, a started port dispatches to no one per the HTML spec, which keeps its inbox draining and collectable. - Refresh the close() and hasPendingActivity() comments that still described the pre-"always post the self-close task" behavior. Tests extend the collectability guard with the drop-in-transit and explicitly-started scenarios.
…shold Review nits, no behavior change: - The canRunScript() gate and wakePeerForClose() comments claimed the drain task "would never run and would leak" during teardown, but the task posts to the peer's (live) context. Reword to the real rationale: firing the peer's close on teardown/GC is intentionally unsupported, and hasPendingActivity() matches by not pinning such a port. - The dispatchCloseEventFromPeer() comment credited a hasPendingActivity branch that is actually false during the dispatch (DrainScheduled is cleared and m_closeEventDispatched set beforehand). The object is kept alive by the drain's RefPtr + our Ref; the wrapper by event.target on the JS stack. - The collectability guard created 10 ports/iteration (not ~8) and used `leaked > N`, which a single-scenario regression (~N leaked) would slip past. Tighten to `> N / 2`, still far above post-settle GC residue.
…eue race A cross-thread TOCTOU could drop the last message of the canonical worker_threads shutdown pattern (postMessage(x); close()) when the receiving port has a close listener. After the drain loop emptied the inbox, cleared DrainScheduled, and released the lock, the peer thread could re-enqueue a message and re-arm a drain before this thread read the peer-closed state; it then dispatched close (which clears the inbox) and the freshly posted drain no-oped on the ctxId mismatch, losing x. Re-check under the side lock right before dispatching: only fire close if the inbox is still empty and no drain was re-armed (new helper readyToDispatchClose). Otherwise the freshly scheduled drain delivers the message and dispatches close itself. Applied at both dispatch sites (the empty-inbox fast path and the post-drain path). Also drop the explicit 60s timeout from the collectability test: the earlier hang was an unrelated onmessage keep-alive (now avoided), and the test runs in ~1.7s, within the default budget.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/jsc/bindings/webcore/MessagePortPipe.cpp:222-226— The!isOtherSideOpen(side)clause added here cannot tell a peer closed via explicitport.close()from one closed by~MessagePort(GC),~TransferredMessagePort(drop-in-transit), orcontextDestroyed()— all set the sameClosedbit. So acloselistener added after the peer was GC'd/dropped runsstartForClose()→attach(), schedules a drain, and firesdispatchCloseEventFromPeer()— making GC timing observable from JS and contradicting the PR's own documented invariant inMessagePort::close()/wakePeerForClose()("a close via teardown or GC deliberately does NOT fire the peer's close"). Consider a separateClosedByScriptbit that onlyMessagePort::close()sets, and check that here instead of!isOtherSideOpen().Extended reasoning...
What the bug is
attach()was extended in c76e234 to schedule a drain when!isOtherSideOpen(side)so that acloselistener added after the peer's explicitclose()still fires. ButisOtherSideOpen()reads onlystate(1-side) & Closed, andMessagePortPipe::close()stores that sameClosedbit regardless of why the side closed: an explicitport.close()from script,~MessagePort(GC of an unreferenced peer),~TransferredMessagePort(port dropped in transit), and the cascade worklist all funnel into the sames.state.store(Closed, …). Soattach()cannot distinguish "the peer calledclose()" from "the peer was garbage-collected / dropped", and schedules a close-dispatching drain in both cases.Step-by-step proof (GC path)
let { port1, port2 } = new MessageChannel(); port1 = null; Bun.gc(true); await Bun.sleep(0); port2.addEventListener('close', () => console.log('fired'));
port1has no listeners →hasPendingActivity()falls through to!m_hasMessageEventListener→ returnsfalse.locallyEntangledPort()isnullptr(MessagePort.h), soport2does not keepport1's wrapper alive.port1's wrapper is collectable.- GC collects it →
~MessagePortruns,!m_isDetached→m_pipe->close(0)→ side 0state = Closed.~MessagePortdoes not callwakePeerForClose()(by design — that lives only inMessagePort::close()). port2.addEventListener('close', cb)→startForClose()(its!isEntangled()guard ism_isDetached, which is false) →m_pipe->attach(1, …).attach()at line 226:!(st & DrainScheduled) && (queuedCount(st) > 0 || !isOtherSideOpen(1)).queuedCount = 0, but!isOtherSideOpen(1) = (state(0) & Closed) ≠ 0→ true.DrainScheduledis set, drain posted.- Drain runs:
portnon-null, inbox empty →emptied = true,limit = 0→emptied && !isOtherSideOpen(1)→port->dispatchCloseEventFromPeer(). Guards:m_isDetached=false,m_closeEventDispatched=false,m_hasCloseEventListener=true→ fires'close'.
Whether
'fired'prints depends entirely on whether GC ran between droppingport1and adding the listener — i.e., GC timing is directly observable from JS.Deterministic variant (no GC needed)
The
~TransferredMessagePortpath reaches the identical state without depending on GC: transfer a port through a carrier whose destination is closed, and~TransferredMessagePortcallspipe->close(side)directly. Acloselistener added to the entangled peer afterwards hits the same!isOtherSideOpenclause inattach()and firesclose. This also produces the inconsistency: acloselistener added before the drop-in-transit never fires (no drain is scheduled — the design choice in resolved #3448795284), but one added after does. Whetherclosefires depends on listener-add timing relative to the peer's non-script death.Why this is a problem
This directly contradicts the PR's own documented design intent in two places:
MessagePort::close()(line ~118): "A close via context teardown … or GC … deliberately does NOT fire the peer's 'close'".wakePeerForClose()(line ~243): "firing the peer's 'close' on teardown/GC is intentionally not supported".
attach()'s!isOtherSideOpen()clause undermines that invariant for the late-listener case. And for the GC path specifically, the HTML spec rationale for not firingcloseon GC is precisely that GC must not be observable from script — which this violates. (One verifier noted recent WHATWG changes do disentangle on GC; even granting that, the inconsistency — listener-before-death never fires, listener-after-death fires — still makes GC timing observable, and still contradicts this PR's stated design either way.)Why nothing prevents it
MessagePortPipe::close()is called identically from all paths and stores onlyClosed— no provenance.attach()reads onlystate(1-side) & ClosedviaisOtherSideOpen().startForClose()'s only guard is!isEntangled()(this side'sm_isDetached), which is false here.dispatchCloseEventFromPeer()has no "was this a script close?" check.Suggested fix
Add a separate state bit (e.g.
ClosedByScript) that onlyMessagePort::close()(gated oncanRunScript()) sets alongsideClosed, and haveattach()'s late-listener clause check that bit instead of!isOtherSideOpen(). The drain's post-loopemptied && !isOtherSideOpen(side)check can stay as-is, since it is only reached when a drain was scheduled (which already implies a script close or queued messages). Alternatively, if the deviation is acceptable, update the comments inMessagePort::close()/wakePeerForClose()to note that a late-addedcloselistener can still observe a non-script close.
A close listener added to a port after its peer died could fire 'close' regardless of how the peer died, because attach()/the drain derived "peer closed" from the bare Closed bit — which is set identically by an explicit port.close(), by ~MessagePort (GC), by ~TransferredMessagePort (drop-in-transit), and by context teardown. That made GC/drop/teardown timing observable from JS (fire iff a listener was added after the death) and contradicted the documented invariant that only a script close() fires the peer's 'close'. Add a ClosedByScript state bit that MessagePort::close() sets (alongside Closed) only when the context can run JS; the destructor, drop-in-transit, and teardown paths leave it unset. attach()'s late-listener wake and the drain's close-dispatch now gate on the peer being ClosedByScript rather than merely Closed, so a peer that died by GC/drop/teardown never surfaces a 'close' event and is never pinned waiting for one. Also refresh the startForClose() comment (the peer wake is done by wakePeerForClose(), not MessagePortPipe::close()).
|
Addressed the 🔴 GC-observability finding (the review-body "additional findings", which has no inline thread) in 26cb6e4. Added a |
There was a problem hiding this comment.
No new issues found on 26cb6e4 — all prior threads look addressed — but given the cross-thread locking, GC-lifetime (hasPendingActivity), and the open ordering question vs #31216, this still warrants a human pass before landing.
Extended reasoning...
Overview
This PR implements the close event on MessagePort for both the closing port and its entangled peer, touching four C++ files in src/jsc/bindings/webcore/ (MessagePort.{cpp,h}, MessagePortPipe.{cpp,h}) and adding ~250 lines of tests. It introduces a new ClosedByScript pipe state bit, a peer-wake mechanism (wakePeerForClose), a TOCTOU re-check helper (readyToDispatchClose), reworks hasPendingActivity() to pin the wrapper across a pending close dispatch, and changes the drain loop to buffer messages on unstarted ports.
Security risks
None identified. No untrusted input parsing, auth, crypto, or filesystem/network surface. The changes are confined to in-process event dispatch and lifetime management.
Level of scrutiny
High. This is hand-written cross-thread concurrency code in the JSC bindings layer: per-side locks, release/acquire atomics, GC-thread-concurrent hasPendingActivity() reads, and task scheduling across ScriptExecutionContexts. The review history bears this out — six iterative rounds surfaced and fixed multiple wrapper-pin leaks, a started-port message-stranding regression, a cross-thread TOCTOU that could silently drop the last message on the canonical postMessage(); close() shutdown pattern, and a GC-observability issue (now resolved via the ClosedByScript bit). All threads are resolved and the current revision passed the bug hunt cleanly, but the density of subtle issues found along the way is itself a signal that a human familiar with this subsystem should sign off.
Other factors
- The author explicitly flagged overlap with the in-flight #31216 (broader
worker_threadsoverhaul touching the same files) and deferred ordering to maintainers — that is a merge-strategy decision a human needs to make. - Test coverage is good: 12 new subprocess tests covering ordering, at-most-once, close-only peers, late listeners, drop-in-transit, and a 4-scenario GC collectability guard with a tightened threshold.
- The cross-thread TOCTOU fix (
readyToDispatchClose) was reasoned correct but explicitly not covered by a deterministic test (acknowledged as untriggerable without instrumentation). - Behavior change:
close()now always posts an async self-close task (even with no listener), and adding acloselistener now attaches the port to the pipe — both are intentional and Node/spec-aligned, but worth a human confirming the broaderworker_threads/ WPT suites are unaffected.
|
Agreed this wants a human pass. State for a reviewer:
|
1 participant
What
Closing one end of a
MessageChannel(or anode:worker_threadsMessagePort) now dispatches acloseevent on both the port that was closed and its entangled peer. The event is delivered asynchronously, after any already-queued messages on that port have drained, matching Node'snode:worker_threadssemantics and the HTMLMessagePortcloseevent.Fixes #32563.
Repro
Node prints
foobarthenclosed!. Before this change Bun printed onlyfoobarand exited immediately; now it matches Node.Cause
MessagePort::close()marked the side closed and removed its listeners, but never notified the entangled peer and never dispatched acloseevent on either port. Nothing kept the event loop alive for the notification either, so a program awaitingcloseexited first.Fix
MessagePortPipe::close()wakes the entangled peer (only when it is attached and no drain is already in flight) so it can emitcloseafter its inbox drains.MessagePortPipe::drainAndDispatch()dispatchescloseonce the inbox empties and the peer has closed, so queued messages are always delivered first.MessagePort::close()schedules the closing port's owncloseevent on its context before tearing down listeners.hasPendingActivity()keeps the JS wrapper (and itscloselistener) alive until a pendingclosehas been delivered. The event fires at most once per port.Ports without a
closelistener keep their previous behavior (close listener tracking mirrors the existingmessage-listener bookkeeping), so the fix adds no work to the message hot path.Verification
New tests in
test/js/web/workers/message-port-pipe.test.ts(nodeMessageChanneland the Web APIaddEventListener("close")):closecloselistener still closes and the process exits cleanlyFail-before / pass-after confirmed: the close tests fail on the released build (
USE_SYSTEM_BUN=1) and pass with the fix; the existingmessage-port-pipe,message-channel,message-port-closed-leak, andmessage-port-context-destroy-leaksuites remain green.