MessagePort: ref the port when its first 'message' listener is added - #38144
MessagePort: ref the port when its first 'message' listener is added#38144robobun wants to merge 1 commit into
Conversation
Node's setupPortReferencing calls port.ref() when the first 'message'
listener is added, so a port that was unref()'d before anyone listened is
ref'd again once someone does. Bun only took the listener loop ref while
m_isRefd was still set, so after unref() a later on('message') kept the
port unref'd and the process exited with the listener installed.
The first 'message' listener now goes through the same gated path as
.ref() (setRefd()). The onmessage setter no longer takes a ref of its own:
installing a callable handler is a listener add, so a handler installed
next to other listeners leaves an unref() in force as in node; replacing
the only handler still refs, since node's setter re-registers a replaced
handler; anything non-callable still unrefs.
The listener loop ref is also released when the port itself is destroyed.
A listening port becomes collectable as soon as its peer closes, which can
happen before the posted peer-close notification (a weak pointer) runs, and
the ref then outlived the port and pinned the loop forever. The onmessage
path was shielded from this by the self-reference the setter used to take;
on()/addEventListener() ports were not.
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Status: fix pushed; CI build 94537 is green on every lane that ran (linux glibc/musl/asan, windows), the two macOS test lanes expired before getting an agent and will be re-run, and the one red test (test-cluster-shared-leak.js on Windows aarch64, node:cluster) is unrelated to this change and has been reported separately. Reproduced on main with node v26.3.0 as the reference: Related: #38009 covers the removal side (last listener removed should drop an explicit ref()); the two compose. |
![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 MessagePort's event-loop ref-counting semantics in native code (default m_isRefd flip, destructor-time release, removing the setter's jsRef()), and overlaps with #36434 / #38009, a human look would still be worthwhile.
Checked: setAttributeEventListener does swap in place on replace (so didSetMessageHandler() is the right hook for that case, and the double setRefd() on a fresh install is idempotent via m_listenerLoopRefActive); the destructor's updateListenerEventLoopRef() needs a live context, and every path that nulls the context (contextDestroyed(), disentangle()) clears m_isRefd first; internal ports in src/js/node/worker_threads.ts and src/js/internal/worker/messaging.ts add-then-unref, so the default flip does not change them.
Extended reasoning...
Overview
This PR aligns Bun's MessagePort ref semantics with Node's setupPortReferencing: the first 'message' listener now sets m_isRefd (via a new setRefd() extracted from jsRef()), the onmessage setter no longer takes a full jsRef() self-reference, m_isRefd defaults to false instead of true, and ~MessagePort() releases a lingering listener loop-ref so a port collected before its posted peerClosed() runs does not pin the loop forever. It also adjusts one existing leak-test fixture to call .ref() explicitly now that onmessage = alone no longer takes the self-ref that test measures. Ten new tests cover the hasRef() matrix, process-liveness, and the collected-port hang.
Security risks
None identified. This is event-loop lifetime bookkeeping; no untrusted input parsing, auth, or crypto is touched.
Level of scrutiny
High. This is native C++ reference-counting and GC-lifecycle code — precisely the category the review guide flags as most-blocked. The change is small in line count but semantically significant: it flips a default that every MessagePort instance sees, removes a self-reference the onmessage setter previously took, and adds a release in the destructor. Each of these has non-local consequences (internal ports in worker_threads.ts/messaging.ts, the context-destroy leak test, the GC-vs-peerClosed() race), and the PR description itself notes overlap with #36434 (a broader GC-pinning fix that is currently conflicting) and composition with #38009. A maintainer should confirm this piece is the one to land first and that the destructor release is safe on every thread that can drop the last reference.
Other factors
- The reasoning is exceptionally thorough, the test matrix is comprehensive, and the PR verifies against 34 upstream Node tests plus the full
worker_threads.test.tssuite. - I traced
setAttributeEventListenerinEventTarget.cpp: the in-placereplaceJSFunctionForAttributeListenerpath does not fireonDidChangeListener, confirmingdidSetMessageHandler()is needed for the replace case; on a fresh install the hook fires first (count 0→1, callssetRefd()) and thendidSetMessageHandler()callssetRefd()again with count already 1 — idempotent becauseupdateListenerEventLoopRef()gates onm_listenerLoopRefActive. - The destructor's
updateListenerEventLoopRef()needsscriptExecutionContext()to be non-null to actually unref. Every path that nulls the context (contextDestroyed()→close(),disentangle()) clearsm_isRefdand releases the ref first while the context is still valid, so the destructor path is only load-bearing when the port is collected with its context still alive — the case the fix targets. - Given the interaction with two other open PRs and the subtlety of the ref-counting model, I'm deferring rather than approving.
|
Updated 10:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit dbc535c has 1 failures in 🧪 To try this PR locally: bunx bun-pr 38144That installs a local version of the PR into your bun-38144 --bun |
1 participant
Problem
port.unref(); port.on("message", f)leavesport.hasRef()false and the process exits with the listener installed. Node printstrueand stays alive until the port is closed or unref'd again. Same foraddEventListener("message"),once("message"), and for a worker doing this onparentPort(the worker exits before any message can reach it). Node's owntest/parallel/test-messageport-hasref.jsasserts this (unref(); ref(); unref(); on("message"), thenhasRef() === true).MessagePort::onDidChangeListenerImpl()(src/jsc/bindings/webcore/MessagePort.cpp) only counted listeners, and the listener loop ref is held whilem_isRefd && m_messageEventCount > 0.jsUnref()clearsm_isRefdand only an explicit.ref()set it again, so after anunref()the first listener took nothing. Node has one ref flag, andsetupPortReferencing(lib/internal/worker/io.js) callsport.ref()when the first'message'listener is added.onmessagesetter (src/jsc/bindings/webcore/JSMessagePort.cpp) covered its own path by callingjsRef()on every callable assignment, which also madeport.on("message", g); port.unref(); port.onmessage = fre-ref the port; node leaves it unref'd, that handler being the second listener.notifyPeerClosed) runs and releases its listener loop ref. The ref then outlived the port and the process could never exit. On main this already happens foron()/addEventListener()ports (repro in the new tests: 20 unreferenced listening ports, peers closed,Bun.gc(), process hangs);onmessageports were shielded only by the self-reference the setter took, which this change removes.Fix
'message'listener (count 0 to 1 inonDidChangeListenerImpl()) callssetRefd(), the half ofjsRef()that setsm_isRefd, so it passes the same gate as.ref(): a closed or transferred port, or one whose peer closed by request, still takes nothing (node closes such a port outright; covered by the existing "ref()/onmessage after the peer closes" test and a new one).m_isRefdnow defaults to false, which is also node's state for a fresh port; the first listener is what sets it.onmessagesetter no longer callsjsRef(). Installing a handler registers a'message'listener, so the hook refs the port when it is the first one and leaves it alone otherwise. The one transition the hook cannot see is a callable handler replacing an installed one (setAttributeEventListenerswaps the function in place); node's setter re-registers it (removeListener, thennewListener), so replacing the port's only listener refs again even after anunref(), anddidSetMessageHandler()does exactly that case. The decision stays keyed on callability, as on main: a non-callable value (null, a string, or an object, whichsetAttributeEventListenerstores butJSEventListenernever invokes) unrefs, matching node, whose setter only counts functions.~MessagePort()releases the listener loop ref if the port still holds one. It is the only release path that does not need the port to still be alive when the peer's close is delivered;m_hasRefcannot be set there since it holds a reference to the port itself. This fixes the latenton()/addEventListener()hang on main and is what lets the setter drop its self-reference safely.setupPortReferencingrefs on the first listener regardless of whatref()/unref()did before, and the setter is an ordinary listener registration to it. Nothing is taken that is not released: the listener ref is still released byunref(), by the count dropping to zero, byclose()/transfer/peer close, and now by destruction; the gate keeps it from being taken on a port nothing would ever release. Bun's internal ports (theWorkerpublic port, the messaging hub ports, the stdio ports in src/js/node/worker_threads.ts and src/js/internal/worker/messaging.ts) add their listener first andunref()afterwards, so their behavior is unchanged; the upstreamtest-worker-ref*,test-worker-parent-port-refandtest-worker-stdio*tests below exercise them.hasRef()matrix over the registration paths (7 rows plus node'stest-messageport-hasrefsequence fail on main), the peer-closed gate, and spawned scripts plus a worker that must stay alive or exit on their own, including the four collected-port scripts (on(),addEventListener()and the peer-collected one fail on main; theonmessageone passes on main and guards the setter change). 8 of the 10 new tests fail on main (release and debug+ASAN); all pass with the change.port1.ref()becauseonmessage =alone no longer takes the self-reference that test measures.test-messagechannel,test-worker-message-port*,test-worker-message-channel,test-worker-workerdata-messageport,test-worker-message-event,test-worker-messaging,test-worker-onmessage*,test-worker-ref*,test-worker-parent-port-ref,test-worker-stdio*,test-worker-terminate-ref-public-port,test-worker-unref-from-message-during-exit. No upstream test becomes vendorable by itself:test-messageport-hasrefalso needs theMESSAGEPORTasync_hooks resource (worker_threads: MESSAGEPORT async_hooks init, worker-visible warnings, data: URL module formats (+4 tests) #35366) andhasRef()aroundclose()(MessagePort: keep hasRef() true until the 'close' event fires, and false inside it #38019)..ref(), is MessagePort: release an explicit ref() when the last 'message' listener is removed #38009; the two compose (that one clears on the last removal, this one sets on the first add). MessagePort: pin the entangled peer while a side holds a loop ref #36434 contains a similar first-listener change inside a broader GC-pinning fix and is currently conflicting; this is only that piece, against current main. Not copied, and already different on main: node'sremoveAllListeners()bypasses its own hooks and leaves a listener-less port ref'd, and a non-callableonmessagevalue assigned to a port without a handler (onmessage = null,onmessage = {}) registers a handler entry and refs the port; in both node keeps the process alive with nothing that can run.Background
hasRef()reports which. Node keeps one flag on the libuv handle:ref()/unref()set and clear it, andsetupPortReferencingsets it on the first'message'listener and clears it on the last removal.m_hasRef, the explicit.ref()hold (an event-loop ref plus a reference to the port object itself, so a ref()'d port outlives a collected wrapper the way node's never-collected ports do), and a listener-driven event-loop ref held whilem_isRefd && m_messageEventCount > 0.hasRef()reports either being held.m_isRefdis the piece this change turns into node's flag on the add side.onDidChangeListeneris the hookEventTargetinvokes after a listener was actually added, removed, or the list was cleared; duplicate adds do not invoke it, so the count is exact. Node-styleon()/once()/off()on a port are a shim in src/js/node/worker_threads.ts over the nativeaddEventListener/removeEventListener, and theonmessagesetter registers a listener through the sameEventTargetmachinery, which is why one native hook covers every registration form.hasPendingActivity()only while its peer is open or messages are queued for it, so once the peer closes an otherwise unreferenced listening port is garbage. The peer's close is delivered as a task posted to the port's thread that resolves aThreadSafeWeakPtrto the port; a port collected in between is simply skipped, hence the release in the destructor.MessagePortPipemarks a sideClosedByRequestonly for an explicitclose(), as opposed to the wrapper being garbage collected; that is what letssetRefd()refuse a port whose peer really closed without making.ref()depend on GC timing.hasRef() probe: node v26.3.0, bun main, this PR (each row is a fresh channel, peer kept reachable)
Process liveness, with an unref'd timer as the only other work:
unref; onandunref; addEventListenerstay alive in node and with this PR, exit on main;on g; unref; onmessage = fexits in node and with this PR, stays alive on main. 20 unreferenced listening ports whose peers are closed, followed byBun.gc(true): node and this PR exit; main exits foronmessage =but hangs foron()andaddEventListener(). Rows that differ only on the removal side (ref; on; offand friends) are unchanged here and belong to #38009.