Conversation
|
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:
WalkthroughImplements a complete fake Node parentPort (listeners, postMessage, close, ref/unref/hasRef), adds a JSC host binding to adjust the worker event-loop ref count, and adds subprocess tests validating parentPort lifecycle transitions. ChangesWorker parentPort Lifecycle Implementation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:36 PM PT - Jul 22nd, 2026
❌ @robobun, your commit b33e63d has 3 failures in
🧪 To try this PR locally: bunx bun-pr 30549That installs a local version of the PR into your bun-30549 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/js/node/worker_threads.ts`:
- Around line 235-292: The add/remove logic must treat the capture flag as part
of the listener identity; currently addEventListener and removeEventListener
dedupe and remove only by callback which breaks the EventTarget spec. Modify the
storage for listeners[type] to track entries with both callback and capture
(e.g., objects {callback, capture, entryWrapper}) or use a per-listener WeakMap
to map capture->wrapper so that addEventListener(type, fn, true) and
addEventListener(type, fn, false) are distinct and removeEventListener honors
the capture argument. Update addEventListener (checks that search list by
callback+capture, create once-wrapper and store it keyed by capture), update
removeEventListener (match and remove by callback+capture and clear the
per-capture wrapper), and keep calls to
ensureForwarder/dropForwarder/syncRef/afterListenerRemoved unchanged except they
should operate on the updated list length/entries.
- Around line 407-414: The current Object.defineProperty calls set
fake.addListener/fake.removeListener to directly alias
addEventListener/removeEventListener which bypasses the EventEmitter wrapper
(functionForEventType) so listeners on parentPort added via addListener receive
raw events; change the definitions so addListener and removeListener reuse the
EventEmitter wrapper path used by on/off (i.e., call the same wrapper that wraps
handlers with functionForEventType for the "message" event) rather than directly
aliasing addEventListener/removeEventListener, ensuring addListener -> the
wrapped add path and removeListener -> the wrapped remove path (use the same
internal helper used for on/off).
🪄 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: be4125bd-7677-4a39-9255-9f0be1bf8de0
📒 Files selected for processing (1)
src/js/node/worker_threads.ts
CI statusThe diff is green on everything it touches; Build 77972 (at
Ready for review/merge. |
|
Heads up for anyone reviewing this: #32828 is a fresh report of the same One thing this PR does not cover that #32828's reproduction also hits: |
parentPort in a node:worker_threads worker is a facade over the worker
global scope. close(), ref(), unref() were empty stubs and hasRef()
returned a hard-coded false, so the idiomatic self-shutdown pattern
(parentPort.on('message', h); ...; parentPort.close()) hung forever:
the message listener's auto-ref on the event loop was never released,
and a message posted after close() was still delivered.
The facade now keeps its own listener lists for 'message' and
'messageerror' and installs a single forwarder on self per type.
Installing a 'message' listener on self takes one auto-ref on the
worker loop (BunWorkerGlobalScope.cpp); a new native hook,
jsFunctionNodeWorkerIncRef, lets the JS layer apply a compensating
delta so the net ref is driven purely by hasRef() and close() state.
ref()/unref() flip the flag, close() drops both forwarders and fires
'close' on nextTick, hasRef() returns the live value, and postMessage
after close() is a no-op.
Fixes #11760
02aa55a to
b4879f3
Compare
The worker never called parentPort.postMessage, so the parent's
worker.once('message') handler (which sends the 'late' message) never
fired and the test degenerated to 'close() lets the process exit'. Have
the worker ack before close() so the parent actually posts 'late' while
the worker is held open by the timeout; if close() were a no-op the
worker would then print 'worker: got late' and the assertion would fail.
…-bearing close() was doing listeners.message.length = 0, which aliases the array dispatch()'s per-entry liveness check reads — so if handler A calls close() mid-dispatch, every not-yet-reached handler B is skipped. Node's MessagePort.close() doesn't touch the listener list (in-flight dispatch continues); closed=true + dropForwarder already make the arrays unreachable for future events, so the truncation was redundant. Also unref the setImmediate in the ref() test so parentPort.ref() is the only thing holding the loop; previously setImmediate's own ref kept the worker alive regardless of whether ref()'s syncRef() fired.
…ually fires The squash/autofix rebase dropped the per-test timeout args, so without the fix the default 5 s test timeout fired before the 30 s spawn timeout and the hung subprocess survived, leaving dozens of processes behind that starved later build steps. With the timeout back, the spawn SIGKILL fires first and the fail-before run exits cleanly (7 fail, 0 leftover processes).
…ener options; cover unref-still-delivers - close() no longer nulls onmessageEntry: Node's MessagePort.close() leaves the onmessage getter returning the last-set handler, and since the wrapper is still reachable via listeners.message the null didn't help GC anyway. - removeEventListener now forwards its third (options/capture) arg to self for non-message event types, matching addEventListener and the pre-PR self.removeEventListener.bind(self) behaviour. - New test: unref() still delivers messages while another handle holds the loop. This is the property the compensating incEventLoopRef path exists for; without it a dropForwarder()-based unref() passed every test byte-identically.
|
Superseded by the Worker rewrite in #37075, which landed on main and closed #11760. In a node:worker_threads worker, parentPort is now a real MessagePort entangled with the parent's public port, so close(), ref(), unref() and hasRef() are the MessagePort implementations; the emulated parentPort this PR extended is only left as a fallback for non-node workers that import worker_threads. Verified on current main (165dc9f, debug build): 8 of the 9 cases in this PR's parentport-lifecycle.test.ts pass, including the #11760 repro (parentPort.close() inside a message handler lets the process exit) and the unref() cases. The remaining case expects hasRef() to still report true immediately after close(); that is a MessagePort-level ordering difference from node that applies to MessageChannel ports too and is unrelated to the code this PR changed, so it is being looked at separately. Closing. |
1 participant
Closes #11760
Repro
Node exits immediately. Bun hangs forever, and a message posted after
close()is still delivered. Same hang forparentPort.unref().Cause
parentPortin anode:worker_threadsworker is an emulated object (fakeParentPortinsrc/js/node/worker_threads.ts). Its.on("message", ...)forwards toself.addEventListener("message", ...), andWorkerGlobalScope::onDidChangeListenerImpl(BunWorkerGlobalScope.cpp) takes an event-loop ref while anymessagelistener is registered on the global scope. Butclose(),ref(),unref()andhasRef()were all no-ops:so the listener ref was never released,
vm.isEventLoopAlive()stayed true inWebWorker.spin(), the worker never reachedshutdown(), and the parentparent_poll_refwas never dropped.Fix
fakeParentPortnow keeps its own listener arrays formessage/messageerrorand installs a single forwarder onselfper event type. A new host functionjsFunctionNodeWorkerIncRef(Worker.cpp) adjusts the worker event loop concurrent ref so the port ref state is purely driven byhasRef && !closed, independent of the WorkerGlobalScope auto-ref that the forwarder presence implies. This letsunref()drop the loop ref while still delivering messages, which is not expressible by just removing the listener.Semantics verified against Node:
hasRef():falseinitially;trueafter amessage/messageerrorlistener orref();falseafterunref()or after the last listener is removed; unchanged byclose().close(): drops the ref, emitscloseon next tick, stops dispatching, makespostMessagea no-op; other handles (timers etc.) keep the worker alive until they complete.ref()with no listener keeps the worker alive on its own.onmessageis stored in the same list so dispatch order relative to.on()matches registration order.The forwarder install/remove and ref-adjustment are sequenced so the concurrent ref counter never dips negative during transitions.
Verification
New test file
test/js/node/worker_threads/parentport-lifecycle.test.tswith 8 cases coveringclose(), late-postMessage rejection after close,unref(),ref(),hasRef()transitions,onmessage, and last-listener removal.test/js/node/worker_threads/worker_threads.test.tsis unchanged at 91 pass.[review] gate passed · iteration 8 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 8
evidence per changed file