worker_threads: MessagePort.postMessage() returns true like Node.js#34011
worker_threads: MessagePort.postMessage() returns true like Node.js#34011robobun wants to merge 12 commits into
Conversation
Node.js returns true from MessagePort.postMessage() when the sending port is live, and undefined when the sending port is already closed or has been transferred. Bun was returning undefined unconditionally, so code that branches on the return value (if (!port.postMessage(msg)) ...) always took the failure path. MessagePort::postMessage now returns ExceptionOr<JSValue>: jsUndefined() on the detached early-return and jsBoolean(true) on every other non-throwing path. jsFunctionPostMessage (parentPort.postMessage / self.postMessage inside a worker) now returns true after a successful enqueue as well, since node's parentPort is a MessagePort. Worker.postMessage and BroadcastChannel.postMessage are unchanged (undefined), matching node.
|
Updated 4:14 AM PT - Jul 12th, 2026
❌ @robobun, your commit ac5f756 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34011That installs a local version of the PR into your bun-34011 --bun |
WalkthroughChanges
MessagePort return contract
🚥 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: 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/node/worker_threads/worker_threads.test.ts`:
- Around line 292-308: Wrap the worker test block centered on the Worker
instance w in a try/finally, registering cleanup before the asynchronous wait
and assertions. Move await w.terminate() into the finally block so the worker is
terminated when message handling, error rejection, or
expect(results).toEqual(...) fails.
🪄 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: 82b0654c-265c-4e64-a84f-4caa0ae61cca
📒 Files selected for processing (5)
src/jsc/bindings/webcore/JSMessagePort.cppsrc/jsc/bindings/webcore/MessagePort.cppsrc/jsc/bindings/webcore/MessagePort.hsrc/jsc/bindings/webcore/Worker.cpptest/js/node/worker_threads/worker_threads.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Revert jsFunctionPostMessage to return undefined (it also backs the web-spec self.postMessage inside workers); move the node-compat return true into the parentPort wrapper in worker_threads.ts. - Gate the undefined return on m_closeEventDispatched as well, so a port whose peer's close has propagated returns undefined like node instead of true forever. - Sample the detached state before serialization so a getter that closes or transfers the port mid-serialize still returns true, matching node. - Test: extend the peer-closed case to await the 'close' event and assert undefined afterwards.
worker_threads.test.ts has pre-existing debug-build timeouts unrelated to this change; putting the new coverage in a focused file lets the gate verify fail-before/pass-after cleanly.
…end work 70bc291 early-returned before disentanglePorts() when only m_closeEventDispatched was set (peer close propagated but self not detached), so a MessagePort in the transfer list stayed entangled. Compute the return value once from the entry-time snapshot and fall through to disentangle/send gated on m_isDetached alone, preserving pre-PR side-effect ordering while still returning undefined when the peer's close has propagated.
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/message-port-postmessage-return.test.ts`:
- Around line 4-43: Add a test case alongside the existing
MessagePort.postMessage() coverage that detaches the source port from a getter
invoked during message serialization, such as by closing or transferring it.
Assert that postMessage() returns true and that no message is delivered, while
retaining cleanup for all created ports.
- Around line 51-59: Update the Promise in the worker message collection flow to
listen for the worker’s "exit" event and settle based on the number of collected
messages: resolve only after exactly two messages have been received, and reject
when the worker exits before completing the protocol or sends unexpected extra
messages. Preserve rejection on the existing "error" event and ensure every
worker failure path settles the awaited operation.
- Around line 6-43: Wrap each MessageChannel test block in try/finally so
cleanup is registered before assertions or awaits execute. In the normal,
peer-closed, own-closed, and transferred-port cases, move or centralize closure
of every live port into the finally block, preserving the existing assertions
and close-event await behavior.
🪄 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: d0f1cacd-81b0-42cb-8d67-0a25cc2777e6
📒 Files selected for processing (3)
src/js/node/worker_threads.tssrc/jsc/bindings/webcore/MessagePort.cpptest/js/node/worker_threads/message-port-postmessage-return.test.ts
The synchronous delivered check was vacuous: MessagePort delivery is always async, so delivered is false right after postMessage() whether or not the message was enqueued. Await port1's 'close' event (which fires after peer-close propagation flushes port1's inbox) so a regression that re-enabled the send would actually fail the assertion.
There was a problem hiding this comment.
LGTM — all four rounds of review feedback have been addressed and the final diff is minimal.
Checked: returnValue is sampled before SerializedScriptValue::create() and gates only the return value; the sole post-serialize early return is still !isEntangled(), so disentanglePorts()/m_pipe->send() run exactly as pre-PR. jsFunctionPostMessage is untouched, so web-spec self.postMessage still returns undefined. Test now awaits port1's 'close' before asserting delivered === false, so that assertion is no longer vacuous.
Extended reasoning...
Overview
Changes MessagePort::postMessage from ExceptionOr<void> to ExceptionOr<JSValue>, returning jsBoolean(true) when the port was live at call time and jsUndefined() when already detached/closed or the peer's close has propagated (m_closeEventDispatched). The two JSMessagePort.cpp overloads surface the value via toJS<IDLAny>. parentPort.postMessage returns true via the worker_threads.ts wrapper (native jsFunctionPostMessage unchanged). One new test file with 6 sub-cases.
Security risks
None. No untrusted-input parsing, no allocation/lifetime changes, no new state. returnValue is a stack-local tagged JSValue (jsUndefined/jsBoolean) — no rooting concerns.
Level of scrutiny
Medium — native C++ in the cross-thread MessagePort path, but the change is return-value-only. Control flow after serialization is byte-identical to pre-PR (the only early return is still if (!isEntangled())), so disentanglePorts(), the entangled-peer warn-and-close, and m_pipe->send() all run on exactly the same conditions as before. No new fields, no GC/refcount/lock changes.
Other factors
This PR went through four rounds of my own review: (1) peer-close propagation never flipping the return to undefined — fixed by folding m_closeEventDispatched into the entry-time snapshot; (2) sampling after serialization instead of before — fixed; (3) jsFunctionPostMessage leaking true into web-spec self.postMessage — reverted, moved to the JS wrapper; (4) the m_closeEventDispatched early return skipping disentanglePorts() — fixed by gating only the return value; plus a vacuous delivered assertion — fixed with a 'close'-event barrier. All resolved and re-verified in the current diff. CodeRabbit's test-hygiene comments (try/finally cleanup, worker exit handler, close-during-serialize coverage) were also applied. The bug hunting system found nothing on the final revision. Test fails on main and passes on the PR per the evidence block.
close() sets m_isClosing before flushQueuedMessagesBeforeClose() runs user handlers and only sets m_isDetached afterwards, so a flushed handler calling postMessage() on the closing port saw true where node returns undefined. Sibling of the m_closeEventDispatched case; the disentangle/send path below is unaffected (still gated on m_isDetached).
…rgence Node delivers the message in the close-during-serialize case (data_ and sibling_ stay non-null until the async OnClose callback); Bun's close() detaches synchronously so the post-serialize guard drops it. That drop is pre-existing and not what this PR changes, so don't lock it in. The .toBe(true) on the return value is the load-bearing assertion here.
There was a problem hiding this comment.
No further issues found — all six rounds of prior feedback have been addressed and the bug hunter is clean on ac5f756. Deferring to a human for the final sign-off since this touches the C++ MessagePort state machine and makes the shared Web/Node MessagePort.prototype.postMessage() return true (a minor Web-spec divergence that's probably fine given Bun's MessagePort is already Node-flavored, but worth a maintainer nod).
What was reviewed:
- Return-value snapshot now covers
m_isDetached || m_isClosing || m_closeEventDispatched, sampled before serialization; control flow (disentangle/send) is unchanged from pre-PR so transfer-list ports are still detached on the peer-closed path. jsFunctionPostMessage(webself.postMessage) left returningundefined;parentPort.postMessagereturnstruevia theworker_threads.tswrapper.Worker.prototype.postMessageandBroadcastChannel.postMessageunchanged.- Test: try/finally cleanup on all channels and the worker, worker
exitwired to reject, and thedeliveredassertion that enshrined a pre-existing Bun/Node divergence was dropped.
Extended reasoning...
Overview
Changes MessagePort::postMessage to return ExceptionOr<JSValue> instead of ExceptionOr<void>, computing jsBoolean(true) vs jsUndefined() from an entry-time snapshot of m_isDetached || m_isClosing || m_closeEventDispatched. The two JSMessagePort.cpp binding overloads switch toJS<IDLUndefined> → toJS<IDLAny> to surface the value. parentPort.postMessage returns true via the JS wrapper in worker_threads.ts (the shared native jsFunctionPostMessage is untouched, so web self.postMessage stays undefined). A new test file covers open, peer-closed (sync and post-propagation), self-closed, transferred, close-during-serialization, and parentPort cases.
Security risks
None identified. No new input parsing, no allocation sizing, no auth/crypto/permission surfaces. The return value is a boolean/undefined derived from existing internal state flags.
Level of scrutiny
Medium-high. The native diff is small (~14 lines) and control flow is unchanged from pre-PR — only the return values differ. But it sits inside the MessagePort lifecycle state machine (m_isDetached/m_isClosing/m_closeEventDispatched/isEntangled()), which took six review iterations to get right: peer-close propagation, entry-time vs post-serialization sampling, the disentanglePorts() regression, the m_isClosing flush-window sibling, and two test-quality passes. That convergence history is reassuring, but also signals the semantics are subtle enough that a maintainer familiar with MessagePortPipe should confirm the final shape.
Other factors
- Web-spec divergence:
MessagePort.prototype.postMessage()is a WHATWG-specvoidmethod; this PR makes it returntruein Bun. That's consistent with Bun's already-Node-flavoredMessagePort(it has.ref()/.unref()/.on()), and the PR carefully keptWorker.prototype.postMessage,BroadcastChannel.postMessage, andself.postMessagereturningundefined. Still a small design call worth a human ack. - Test coverage: comprehensive, hermetic (try/finally on every channel and the worker,
exitwired to reject), and verified to fail on main / pass on the branch. Existingmessage-channel.test.ts,worker.test.ts,broadcast-channel.test.ts, etc. reported passing. - Known limitation acknowledged by the author: a bare sender port with no listeners never learns the peer closed (no
peerClosed()notification), so it keeps returningtrue— pre-existing property of Bun's peer-close notification path, out of scope here.
|
The diff itself is green. The new test Remaining CI failures are unrelated flakes:
Ready for a maintainer to merge. |
1 participant
What does this PR do?
In Node.js,
MessagePort.postMessage()returnstruewhen the sending port is live, andundefinedwhen the sending port is already closed, has been transferred, or its peer's close has propagated. Bun was returningundefinedunconditionally, so Node-targeting code that branches on the return value (if (!port.postMessage(msg)) ...) always took the failure branch.Repro
Fix
MessagePort::postMessagenow returnsExceptionOr<JSValue>instead ofExceptionOr<void>. The closed/detached state is sampled on entry (before serialization runs user JS) and also treats a propagated peer close (m_closeEventDispatched) as closed; that snapshot decides betweenjsUndefined()andjsBoolean(true). A port that becomes detached during serialization returnstruewith the message dropped, matching Node. The two JS binding overloads surface the value withtoJS<IDLAny>instead oftoJS<IDLUndefined>.parentPort.postMessagereturnstruevia theworker_threads.tswrapper; the shared nativejsFunctionPostMessage(which also backs the web-specself.postMessageinside workers) is unchanged and still returnsundefined.Worker.prototype.postMessageandBroadcastChannel.prototype.postMessageare unchanged (stillundefined), which also matches Node.How did you verify your code works?
New test in
test/js/node/worker_threads/worker_threads.test.tscovering open, peer-closed (synctrue, thenundefinedafter'close'fires), self-closed, transferred, andparentPortfor both thetransferListand{ transfer }overloads. The test fails without thesrc/changes and passes with them. Existingmessage-channel.test.ts,message-port-pipe.test.ts,worker-transfer-list.test.ts,worker-postmessage-transfer.test.ts,worker.test.ts,broadcast-channel.test.ts, andtest-worker-message-port.jsall still pass.[review] gate passed · iteration 2 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 1 rejected · iteration 2
evidence per changed file