MessagePort: fire 'close' on a port that is transferred away - #37991
MessagePort: fire 'close' on a port that is transferred away#37991robobun wants to merge 7 commits into
Conversation
Node closes the handle of a port listed in a transfer list, so the sender's port object emits 'close' (asynchronously) while the channel itself continues in the receiver's new port. disentangle() removed the listeners and left the context synchronously, so nothing was ever dispatched on that object. It now queues the same deferred close task close() uses and stays attached to its context until collected, like a closed port, so the pending task keeps the wrapper and its listeners alive until the event has fired.
|
Updated 5:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit 0b5c0bf has 2 failures in
🧪 To try this PR locally: bunx bun-pr 37991That installs a local version of the PR into your bun-37991 --bun |
|
Status: fix is up in this PR (code head 0aa542e; the commits after it are empty re-run commits), review threads resolved. Reproduced on main (f426a8e) and the 1.4.0 release with a port in a Fix: CI note: build 94455 had one real red, |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughChangesMessagePort close lifecycle
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 3
🤖 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/node/worker_threads/worker_threads.test.ts`:
- Around line 1301-1318: The transfer tests must exercise receiver endpoints,
not only sender-side detachment. In
test/js/node/worker_threads/worker_threads.test.ts:1301-1318, preserve each peer
and add message round trips through ports received via workerData,
Worker.postMessage(), and parentPort.postMessage(); in
test/js/web/workers/worker-postmessage-transfer.test.ts:147-176, add a worker
handler that receives and uses the parent-transferred port, and round-trip
through the port received by the main thread. Cover the complete sibling-API and
alternate-mode matrix.
- Around line 1239-1257: Replace fixed setImmediate waits with promises resolved
by the expected close events. In
test/js/node/worker_threads/worker_threads.test.ts lines 1239-1257, create
close-event promises before the relevant transfer/close operations and await
both before asserting event order or counts. In
test/js/web/workers/worker-postmessage-transfer.test.ts lines 152-172, resolve
worker and main-thread assertions from their close listeners. In
test/js/web/workers/message-channel.test.ts lines 373-375, resolve the GC test
promise when fired reaches 50 and await it instead of four event-loop turns.
In `@test/js/web/workers/message-channel.test.ts`:
- Around line 359-360: Replace the dynamic CommonJS require in the heap
statistics setup with a module-scope import of heapStats from bun:jsc, then keep
count using that imported symbol. Do not otherwise change the MessagePort
counting behavior.
🪄 Autofix
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: 61d132b5-6bd0-4d5b-a6dd-588a4439ce9a
📒 Files selected for processing (5)
src/jsc/bindings/webcore/MessagePort.cppsrc/jsc/bindings/webcore/MessagePort.htest/js/node/worker_threads/worker_threads.test.tstest/js/web/workers/message-channel.test.tstest/js/web/workers/worker-postmessage-transfer.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes ActiveDOMObject lifetime behavior in GC-sensitive C++ (a transferred-away port now stays attached to its ScriptExecutionContext instead of calling observeContext(nullptr)), a human look would still be worthwhile.
What was reviewed:
queueCloseEvent()extraction is a pure refactor ofclose()'s tail;m_closeEventDispatchedprevents a double fire when the peer later closes.- Staying attached to the context matches what
close()already does, andcontextDestroyed()/stop()on a detached (m_isDetached) port both no-op throughclose()'s early return. - Loop-ref accounting after transfer:
m_isRefd = falsereleases the listener ref viaupdateListenerEventLoopRef();jsRef()andaddEventListener('message')'s re-attach both gate onisEntangled(), so no ref is re-taken while listeners linger until the task runs. - GC-safety:
queueTaskKeepingObjectAlive's pending-activity count keepshasPendingActivity()true independently ofvirtualHasPendingActivity()(which returns false oncem_isDetached); the new heap-count test bounds retention afterwards.
Extended reasoning...
Overview
This PR makes a MessagePort fire 'close' on the sender's object when it is transferred away, matching Node.js. The core change is in src/jsc/bindings/webcore/MessagePort.cpp: the tail of close() that queues a deferred close-event task is extracted into queueCloseEvent(), and disentangle() now calls it instead of synchronously calling removeAllEventListeners() and detaching from the ScriptExecutionContext via willDestroyActiveDOMObject/willDestroyDestructionObserver/observeContext(nullptr). Three test files gain coverage for local transfers, worker transfers in both directions, GC survival of the pending event, and post-fire collectability.
Security risks
None identified. This is Node.js-compat event dispatch on an object that is already inert after transfer; no untrusted input parsing, auth, or crypto is involved.
Level of scrutiny
High. This is JSC-bindings C++ that touches wrapper-lifetime and GC visibility: it removes an explicit observeContext(nullptr) and the associated context-observer removals from disentangle(), relying instead on the same "stay attached until collected" behavior that close() already has. I traced through JSMessagePortOwner::isReachableFromOpaqueRoots (gates on !isContextStopped() && hasPendingActivity()), ActiveDOMObject::hasPendingActivity() (m_pendingActivityInstanceCount || virtualHasPendingActivity()), and queueTaskKeepingObjectAlive (holds a PendingActivity until the task runs), and the reasoning holds: the pending-activity count keeps the wrapper alive across GC while the task is queued, and once it runs the port is collectable (m_isDetached short-circuits virtualHasPendingActivity()). I also checked that leaving the port registered as an ActiveDOMObject/destruction observer is safe: contextDestroyed() → close() no-ops on m_isDetached, and close() never detached from the context either, so this is not a new lifetime shape.
Other factors
The PR is thoroughly tested (listener-order, listener-added-after-transfer, peer-untouched, channel-still-works, process-exit, all four transfer entry points, a 50-iteration GC stress with Bun.gc(true), and a heapStats retention bound). Loop-ref bookkeeping after the deferred removeAllEventListeners() was checked: m_isRefd is cleared before the task, so updateListenerEventLoopRef() releases the listener ref immediately, and jsRef()/addEventListener('message')'s re-attach both gate on isEntangled() so lingering listeners cannot re-ref the loop. The one thing I would want a maintainer to sanity-check is the removal of the explicit willDestroyActiveDOMObject/willDestroyDestructionObserver calls — the argument that closed ports already behave this way is sound, but removing explicit teardown from a transfer path in wrapper-lifetime code is exactly the kind of change the repo's review guidelines flag for close reading.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes MessagePort::disentangle() to keep the transferred-away object attached to its ScriptExecutionContext (dropping the explicit willDestroyActiveDOMObject/observeContext(nullptr) teardown) and relies on queueTaskKeepingObjectAlive for wrapper liveness, a human look at the GC/lifetime implications would still be worthwhile.
What was reviewed:
queueCloseEvent()factoring andm_closeEventDispatchedguarding a double fire when the received port later closes — covered by the local-channel test.virtualHasPendingActivity()returns false oncem_isDetachedis set, so the only post-transfer pin is the queued task's pending activity; the new heapStats test bounds retention after it runs.addEventListener("close")on an already-detached port skipsregisterCloseContext(guarded byisEntangled()), so the late listener just receives the queued event.contextDestroyed()on a now-still-attached disentangled port:close()no-ops onm_isDetached, then falls through toActiveDOMObject::contextDestroyed()— same as aclose()d port.
Extended reasoning...
Overview
This PR makes a transferred-away MessagePort fire 'close' on the sender's object, matching Node's TransferForMessaging() semantics. The native change is small but structural: the tail of close() is factored into queueCloseEvent(), and disentangle() now calls it instead of synchronously calling removeAllEventListeners() and detaching from the ScriptExecutionContext via willDestroyActiveDOMObject / willDestroyDestructionObserver / observeContext(nullptr). Three test files gain coverage for the local-channel path, all three worker-transfer routes, the web Worker API in both directions, and a GC survival + collection bound test.
Security risks
None identified. This is event-dispatch timing and object-lifecycle management for an existing API; no untrusted-input parsing, auth, or crypto paths are touched.
Level of scrutiny
High. MessagePort transfer is on the hot path of every worker communication, and the change alters when a disentangled port leaves its context's ActiveDOMObject / destruction-observer lists (previously: at disentangle time; now: at destruction time, same as a close()d port). That is a deliberate, well-argued decision — the queued close task dispatches through the context, and JSMessagePortOwner::isReachableFromOpaqueRoots only honors pending activity while the object has a context — but it changes GC-visible lifetime and the set of objects the context iterates on stop/destroy. The PR description and the new heapStats test address the leak concern directly, and the m_isDetached early-return in virtualHasPendingActivity() means the object itself does not self-pin after the task runs. Still, this is exactly the class of change (JSC GC interaction, queueTaskKeepingObjectAlive, ActiveDOMObject lifecycle) that the repo guidelines flag for careful maintainer review, and it interacts with two other open PRs touching neighboring lines (#33638, #37966).
Other factors
The bug-hunting system found nothing. All CodeRabbit and comment-cop threads are resolved (comments were shortened in 3ae5416 / 8b56966; the receiver-round-trip and module-scope-import feedback was applied in e05767e). Test coverage is thorough — event ordering, listener-added-after-transfer, peer untouched, channel still works, no double fire, process-exit behavior, GC survival and post-fire collection, and every disentangle entry point (MessagePort::postMessage, Worker::postMessage, worker-global postMessage, Worker constructor transferList). The bounded setImmediate windows are justified (ordering bound, not a timing guess) and match existing close-event tests in the same files. Given the lifecycle change in a GC-sensitive C++ path, deferring to a maintainer rather than auto-approving.
|
Follow-ups since the PR was opened, all on top of the same fix:
The first CI run (build 93875) had only unrelated flakes (each passed alone or on retry) plus two macOS jobs that were canceled before starting; the later pushes re-run it. All review threads are resolved. The PR description has been updated to describe the tests as they stand now. |
The transferred-away object now keeps its context, so tryTakeMessage's isEntangled() check is what keeps it from taking messages from the pipe side its receiver owns. Pin that with a message queued for the received port, and say so at the check.
|
Self-review outcome, addressed in 0aa542e:
|
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. Because it changes MessagePort lifetime semantics in the JSC bindings — a transferred-away port now stays registered as an ActiveDOMObject/ContextDestructionObserver on its original context instead of being detached, with the wrapper pinned across GC by the queued close task — a human look at the GC/context-teardown interactions would still be worthwhile.
What was reviewed:
disentangle()no longer callsobserveContext(nullptr); verifiedvirtualHasPendingActivity()still short-circuits onm_isDetachedso only the queued task pins the wrapper, andcontextDestroyed()/stop()on a detached port no-op throughclose()'sm_isDetachedguard.tryTakeMessageon a transferred-away port (which now keeps its context) — theisEntangled()check is what prevents it stealing the receiver's messages; pinned by the new test.- The heap-count leak bound in the new GC test and the
m_closeEventDispatchedguard against a second event when the channel later closes.
Extended reasoning...
Overview
The PR makes MessagePort::disentangle() queue the same deferred 'close' event that close() already queues, so a port listed in a transfer list fires 'close' on the sender's side (matching Node's TransferForMessaging). To make that possible it factors the tail of close() into queueCloseEvent() and — the substantive change — removes the removeAllEventListeners() / willDestroyActiveDOMObject / willDestroyDestructionObserver / observeContext(nullptr) calls from disentangle(), so the transferred-away object stays attached to its ScriptExecutionContext until it is collected (like a close()d port already does). Tests are added in three files covering local postMessage, all three Worker transfer routes, the web-Worker self.postMessage path, GC survival of the pending event, a leak bound after the event fires, and receiveMessageOnPort on the transferred-away object.
Security risks
None identified. This is event-dispatch and object-lifetime plumbing; no untrusted input parsing, auth, or crypto is involved.
Level of scrutiny
High. This is C++ in src/jsc/bindings/webcore/ touching GC lifetime — the repo's most-blocked review category. The change alters when a MessagePort unregisters from its context's ActiveDOMObject and DestructionObserver lists (previously: at transfer time; now: at destruction time via the base-class destructors). queueTaskKeepingObjectAlive holds a pending activity across GC; JSMessagePortOwner::isReachableFromOpaqueRoots only honors that while the object has a context, which is why the context detach had to go. I traced the surrounding guards (virtualHasPendingActivity() returns false on m_isDetached; contextDestroyed() → close() early-returns on m_isDetached then calls ActiveDOMObject::contextDestroyed(); dispatchEvent/dispatchOneMessage/peerClosed/jsRef all key on m_isDetached) and did not find a path that now misbehaves, but the interaction with worker termination and cross-thread context teardown while a close task is pending is subtle enough that a maintainer familiar with the ActiveDOMObject lifecycle should confirm.
Other factors
The PR description is unusually thorough (mechanism, Node source references, every guard audited, interaction with two other open PRs on the same file). Tests are comprehensive and include a GC-stress test with a heap-count leak bound and a hang-guard subprocess test. All CodeRabbit and comment-cop threads are resolved (comments were shortened to one line each; receiver-side round-trips were added; heapStats moved to a module-scope import). CI build #94455 is running on the latest head. No prior claude review exists on this PR.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes GC-lifetime behavior in the JSC MessagePort bindings (a transferred-away port now stays registered with its ScriptExecutionContext instead of detaching via observeContext(nullptr)), a human look would still be worthwhile.
What was reviewed:
disentangle()now reusesclose()'s deferred-event tail viaqueueCloseEvent();m_closeEventDispatchedprevents a second fire when the channel later closes.- Wrapper liveness across the queued task:
queueTaskKeepingObjectAlivebumpsm_pendingActivityInstanceCount, andJSMessagePortOwner::isReachableFromOpaqueRootshonors it whileisContextStopped()is false — the new heap-count test confirms collection afterwards. - Callers that previously could distinguish a transferred port by a null context (
tryTakeMessage,virtualHasPendingActivity,contextDestroyed,jsRef) all gate onm_isDetached/isEntangled()first, so behavior is unchanged; the newreceiveMessageOnPortassertion pins the one place where that matters. contextDestroyed()on a now-still-attached transferred port:close()early-returns onm_isDetached, matching what aclose()d port already does.
Extended reasoning...
Overview
The PR makes a transferred-away MessagePort fire 'close' on the sender's object, matching Node's TransferForMessaging() (Close(); return Detach();). The native change is small: the tail of MessagePort::close() is factored into queueCloseEvent(), and disentangle() calls it instead of synchronously stripping listeners and calling observeContext(nullptr). Three test files gain coverage for the local-channel case, all three Worker transfer routes (constructor transferList, worker.postMessage, worker-side parentPort.postMessage), the web Worker/self.postMessage pair, and a GC-survival + collection-afterwards check.
Security risks
None identified. This is event-dispatch timing and GC-lifetime bookkeeping on an existing object; no parsing of untrusted input, no auth/crypto, no new external surface.
Level of scrutiny
Higher than a mechanical fix. The functional change is straightforward, but the implementation removes observeContext(nullptr) and the willDestroyActiveDOMObject/willDestroyDestructionObserver calls from disentangle(), so a transferred-away port now remains a ContextDestructionObserver and ActiveDOMObject on its original context until collected — the same as a close()d port already does, but it is a lifetime-invariant change in JSC bindings. The author audited every site that could have relied on the old "transferred ⇒ no context" property (list in the PR description) and added a test that pins the one load-bearing guard (isEntangled() in tryTakeMessage). I traced JSMessagePortOwner::isReachableFromOpaqueRoots and queueTaskKeepingObjectAlive to confirm the pending-activity token keeps the wrapper alive across the queued task and releases afterwards, and that virtualHasPendingActivity()'s m_isDetached early-return means nothing else pins it once the task has run.
Other factors
All review threads (CodeRabbit and comment-cop) are resolved: the receiver-side round-trip coverage was added, heapStats moved to a module import, and the multi-line comments were shortened. The CI note about worker-transfer-terminate-stress on ASAN was reproduced against main with this PR's .cpp/.h reverted, so it's pre-existing. The PR description also flags a genuine interaction with in-flight PR #33638 (its m_pipe->close() placement and its removal of the isEntangled() guard would both regress the tests added here), which is exactly the kind of cross-PR concern a maintainer should be aware of. Given the GC-sensitive nature of the change and the documented interaction with another open PR, deferring to a human reviewer.
1 participant
Problem
MessagePortlisted in a transfer list never emits'close'on the sender's side. Node emits it:port.on("close", ...)followed byother.postMessage(port, [port])prints in node and prints nothing in bun. Same forworker.postMessage(port, [port]),new Worker(file, { workerData, transferList: [port] })andparentPort.postMessage(port, [port])inside a worker. This is theport1 closedline of Unexpected MessagePort+Worker behavior on Bun vs NodeJS #19862 that is still missing on main.MessagePort::disentangle()(src/jsc/bindings/webcore/MessagePort.cpp), which every transfer path ends in, calledremoveAllEventListeners()andobserveContext(nullptr)synchronously. The deferred close task thatclose()queues was never queued for a transfer, and after those two calls nothing could have been dispatched on the object anyway.Fix
close()(queue a task that dispatches'close'and then removes the listeners) becomesqueueCloseEvent();disentangle()calls it instead of removing the listeners and leaving the context. The pipe side is still handed to the receiver, so the peer is not notified and the channel keeps working through the received port; only the transferred-away object fires, once (m_closeEventDispatched).disentangle()no longer detaches the object from itsScriptExecutionContext: the task dispatches through that context (EventTargetdispatch asserts one), andJSMessagePortOwner::isReachableFromOpaqueRootsonly honors the task's pending activity whileisContextStopped()is false, so leaving early would let a GC collect the wrapper and the listeners before the event fires. A closed port already stays attached until it is collected; a transferred one now does the same. Nothing else pins it afterwards (heap count ofMessagePortreturns to baseline in the new test).m_isDetached. Everything that tells a dead object from a live one already keys on it (tryTakeMessage,peerClosed,dispatchOneMessage,dispatchEvent,virtualHasPendingActivity,jsRef,disentanglePorts, the transfer-list checks inSerializedScriptValue.cppandWorker.cpp;stop()/contextDestroyed()go throughclose(), which returns early on it). The one place where the context used to be a second guard istryTakeMessage(receiveMessageOnPort): the transferred-away object still refers to the pipe side its receiver now owns, so itsisEntangled()check is what keeps it from taking the receiver's messages. That check gets a comment and is pinned by the local test (a message queued for the received port,receiveMessageOnPort(transferredAway)isundefined, the received port gets the message). Node returnsundefinedthere as well once the port's'close'has fired (v26.3.0 segfaults if it is called in the same tick as the transfer, so the test asserts it after the event).MessagePort::TransferForMessaging(), which isClose(); return Detach();(node_messaging.cc#L921-L924); the handle's close callback then dispatches thecloseevent on the JS object (io.js#L171-L174). So node's event is asynchronous, reaches listeners added right after thepostMessagecall, and does not touch the peer. Bun fires it at the same point it already firesclose()'s event (a task: after the calling code and its microtasks, beforesetImmediate, whereas node fires both in the uv close phase, aftersetImmediate); the tests wait twosetImmediateturns so the same scripts pass under node.disentangle()(MessagePort::postMessage,Worker::postMessage, the worker-globalpostMessage, theWorkerconstructor'stransferList), so they are all covered by the one change. The ports bun transfers internally when constructing anode:worker_threadsworker have no listeners; they now each queue one no-op task.test/js/node/worker_threads/worker_threads.test.ts, newdescribe("a port transferred away fires 'close' on the sender's object"): localpostMessage(listener order, listener added after the transfer, peer untouched,receiveMessageOnPorton the transferred-away object, channel still works in both directions, no second event when the channel really closes), a process whose last statement is the transfer (event fires, process exits on its own), and the threeWorkerpaths (constructortransferList,worker.postMessage, worker-sideparentPort.postMessage), each also passing a message through the port the other side received. 3 fail on the release build, pass with the fix; whole file 125 pass.test/js/web/workers/message-channel.test.ts: 50 transferred ports made unreachable and GC'd while the event is pending all fire, and are collected afterwards. Fails before (0 fired), passes after.test/js/web/workers/worker-postmessage-transfer.test.ts:Worker#postMessageand worker-sideself.postMessage, withaddEventListener("close"); the peers stay open and both received ports carry a message. Fails before, passes after.message-port-pipe,message-port-closed-leak,message-port-context-destroy-leak,message-event,structured-clone,worker-transfer-list,worker-transfer-terminate-stresstest files and the 26 upstreamtest-worker-message-*/test-messagechannelfiles intest/js/node/test/parallelpass. Node's own suite has no case for this event, so no previously failing upstream test is enabled by this.MessagePort.cpp: structured clone: re-check the transfer list after serializing so a failed transfer detaches nothing #37966 (transfer-list re-validation) only editspostMessage()and is unaffected by this. node:worker_threads: receiveMessageOnPort drains the retained queue after close() #33638 (receiveMessageOnPortafterclose()) already conflicts with main on its own, and both of itsMessagePort.cpphunks need rework on top of this one: itsm_pipe->close()has to stay specific toclose()rather than go intoqueueCloseEvent(), whichdisentangle()also queues (the channel-intact tests here fail if it is put there), and it can no longer droptryTakeMessage'sisEntangled()check on the grounds that a transferred-away port has no context (thereceiveMessageOnPortassertion here fails if it does).Background
MessageChannelin bun is oneMessagePortPipewith two sides; each JSMessagePortobject owns one side. Transferring a port (disentangle()) detaches the object from its side and ships the side to the receiver, which wraps it in a newMessagePortobject. The old object is inert from then on; the peer is still entangled, now with the new object.MessagePortbacks bothglobalThis.MessageChannelandnode:worker_threads(the module addson()/once()overaddEventListener), and its'close'event already follows node (the closing port fires it too, node:worker_threads: +48 Node.js tests passing — MessagePort, stdio, SHARE_ENV, exit codes, transfer semantics, postMessageToThread + inspector #31216). The HTML spec has no event for the transferred-away object; in bun it is the same object either way, so it fires for both APIs.ActiveDOMObject::queueTaskKeepingObjectAliveposts a task on the object'sScriptExecutionContextand holds a pending activity on the object until it runs.hasPendingActivity()is what keeps a wrapper (and therefore the JS listener functions reachable only through it) alive across GC, but the wrapper's GC hook ignores it once the object has no context.Repro scripts and output (node v26.3.0, bun 1.4.0 release, this branch)
Local channel, the scenario of the new
postMessage to a local porttest:node v26.3.0 and this branch print the same thing:
bun 1.4.0:
Worker paths (the script from the
transfers to and from a Workertest; lines sorted, since the routes complete independently and node orders the two parent-side close lines the other way round from bun). node v26.3.0 and this branch print all six and exit 0:bun 1.4.0 prints only the three
received port workslines and exits 0.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file