Repository navigation
Conversation
|
Updated 3:30 AM PT - Apr 23rd, 2026
❌ @autofix-ci[bot], your commit 17af498 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29215That installs a local version of the PR into your bun-29215 --bun |
|
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:
WalkthroughReworks worker_threads parentPort emulation to forward native worker Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 4 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/worker_threads.ts`:
- Around line 245-299: Replace uses of regular Map prototype methods on the
listener-tracking maps with the intrinsic-prefixed variants to avoid prototype
tampering: update accesses to trackedByListener and bucket inside
parentPortAddEventListener (and the TrackEntry logic) to use
trackedByListener.$get/$has/$set/$delete and bucket.$has/$get/$set/$delete and
read bucket.$size instead of the plain .get/.has/.set/.delete/.size; keep the
same semantics (creating a new Map for a missing bucket, storing entry, and
deleting when empty) but call the intrinsic methods (e.g., replace
trackedByListener.get(listener) with trackedByListener.$get(listener),
bucket.size with bucket.$size) so tamper-resistant intrinsics are used
consistently.
🪄 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: 482a37ca-30dd-4f2d-aebc-c1d8697cadd7
📒 Files selected for processing (2)
src/js/node/worker_threads.tstest/regression/issue/29211.test.ts
Addresses two follow-up review comments on #29215: - `parentPortAddEventListener` called `acquireListener()` unconditionally for every event type — including `close`, `error`, etc. — so `parentPort.once('close', cleanup)` installed the capture-phase message forwarder on `self` even when the user was not listening for messages. The forwarder then intercepted parent messages, called `stopImmediatePropagation()`, and re-dispatched onto `parentPortTarget` where no `message` listener existed — so subsequent parent messages were silently dropped. And since native `close` events never reach `parentPortTarget`, `releaseListener()` never ran, pinning the worker event loop forever. Both `parentPortAddEventListener` and `parentPortRemoveEventListener` now gate `acquireListener`/`releaseListener` on `type === 'message' || type === 'messageerror'`, so listeners for unrelated event types pass straight through to `parentPortTarget` without touching the global-scope forwarder. - Use tamper-resistant intrinsics on the inner listener-tracking Map (`bucket.$get` / `$set` / `$has` / `$delete` / `$size`) and `$Array.from` for the ports clone — following src/js/CLAUDE.md. The outer `WeakMap` stays on plain prototype methods because `.$get` is not defined as an intrinsic for WeakMap. Regression test added: `parentPort listener for non-message events does not block parent messages` — registers `.on('close')` before `.on('message')` and asserts all three posted messages reach the handler. Fails on the previous commit; passes after this change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29211.test.ts`:
- Around line 99-117: Swap the assertion order so stdout is checked before
exitCode/stderr: capture stdout, stderr, exitCode as you do, but move the
JSON.parse(stdout.trim()) / expect(...) block to run prior to asserting
exitCode/stderr (the expect({ stderr: cleanStderr(stderr), exitCode
}).toEqual(...) line). This ensures the parsed worker output
(handleMessageCalls/selfAddListenerCalls/globalAddListenerCalls) is reported on
failures before verifying exitCode, improving debug info for failures in the
Promise.all-collected values (stdout, stderr, exited).
🪄 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: 4e6f1e69-2d55-4e21-88d0-5277fab450ff
📒 Files selected for processing (2)
src/js/node/worker_threads.tstest/regression/issue/29211.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29211.test.ts`:
- Line 116: The test currently asserts both cleaned stderr and exitCode with
expect({ stderr: cleanStderr(stderr), exitCode }).toEqual({ stderr: "",
exitCode: 0 }); which is brittle in CI/ASAN; change the assertion to only check
the subprocess exit code (e.g., assert exitCode is 0 via
expect(exitCode).toBe(0) or equivalent) and remove the strict empty stderr
comparison (leave stderr untouched unless the test explicitly validates a
message). Apply the same change for other occurrences that use
cleanStderr(stderr) in these tests (the spots referenced around lines 179, 237,
296, 342, 388, 443).
🪄 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: 8aa3d902-eeda-4589-81a6-ffc5b995351d
📒 Files selected for processing (1)
test/regression/issue/29211.test.ts
There was a problem hiding this comment.
The core logic is sound and all previously-flagged 🔴 issues have been addressed, but this is a substantial behavioral change to the worker_threads parentPort dispatch mechanism — worth a human pass before merge.
Extended reasoning...
Overview
This PR replaces the global-scope forwarding in fakeParentPort() with a dedicated parentPortTarget EventTarget, lazy capture-phase forwarders on self, and full listener lifecycle tracking. It touches ~250 lines of production JS runtime code in src/js/node/worker_threads.ts and adds a 446-line regression test suite.
Previous Bug Round
Three prior 🔴 bugs I flagged have all been addressed in the follow-up commits (6cc8135, 3f220d2): the AbortSignal leak that kept listenerCount stuck, missing ports in the cloned MessageEvent, and the EventListenerObject crash. The acquireListener gating for non-message event types was also fixed. The remaining bug reports are nits only (assertion ordering and missing test.concurrent).
Security Risks
No security concerns. The change is scoped to worker message dispatch within the JS builtin layer — no auth, crypto, or permission-sensitive paths involved.
Level of Scrutiny
Medium-high. While the fixes are correct in isolation, the implementation is complex: lazy forwarder install/uninstall, WeakMap-based listener tracking, signal interception, once-wrapper cleanup, and EventListenerObject dispatch. Any subtle mistake in reference counting could silently drop messages or leak event-loop refs. Human verification of the overall flow is warranted.
Other Factors
The test suite covers the core scenarios well (z3/emscripten pattern, all listener variants, off, transferred ports, EventListenerObject, AbortSignal, non-message events). The two remaining nits (assertion order and test.concurrent) are flagged as inline comments and do not affect correctness.
| // Two pre-removal messages: 'handler' fires twice (x2 registrations) = 4 | ||
| w.postMessage({ n: 1 }); | ||
| w.postMessage({ n: 2 }); | ||
| w.postMessage({ cmd: 'remove-one' }); | ||
| // After removal ack, main posts two more — handler should fire once | ||
| // each (only one registration left). Final total: 4 + 2 = 6. |
There was a problem hiding this comment.
🟡 The inline comment in the main.mjs fixture says "Two pre-removal messages: 'handler' fires twice (x2 registrations) = 4" and "Final total: 4 + 2 = 6", but the assertion below is toEqual({ fired: 8 }). The comment forgets that the {cmd:'remove-one'} message itself fires both handler registrations (bringing the pre-removal count to 6) before the third listener removes one — so the real total is 6 + 2 = 8. Either delete this inline comment (the block above the assertion at lines 565-568 already explains it correctly) or fix it to match.
Extended reasoning...
What the bug is
In the parentPort.on twice + off once test, the inline comment embedded in the main.mjs fixture string (test/regression/issue/29211.test.ts:544-549) reads:
// Two pre-removal messages: 'handler' fires twice (x2 registrations) = 4
w.postMessage({ n: 1 });
w.postMessage({ n: 2 });
w.postMessage({ cmd: 'remove-one' });
// After removal ack, main posts two more — handler should fire once
// each (only one registration left). Final total: 4 + 2 = 6.But the actual assertion at line 574 is expect(JSON.parse(stdout.trim())).toEqual({ fired: 8 }). A reader following the inline comment will expect 6 and be confused when the test asserts 8.
The specific code path
The worker registers handler twice via parentPort.on('message', handler), then registers a third anonymous listener that processes remove-one / report commands. Listeners dispatch in registration order, so for every incoming message both handler registrations fire before the anonymous command listener.
Why the comment is wrong
The inline comment counts only {n:1} and {n:2} as "pre-removal messages" (2 messages × 2 registrations = 4). It forgets that {cmd:'remove-one'} is also a pre-removal message from handler's perspective: both handler registrations fire on it (bringing fired to 6) before the third listener — registered last — runs removeListener. So the pre-removal subtotal is 6, not 4. After the removal ack, {n:1} → 7 and {cmd:'report'} → 8.
Step-by-step proof
w.postMessage({n:1})→handlerfires for reg Fix ?? operator #1 and reg Fix calling #private() functions in classes #2 →fired = 2w.postMessage({n:2})→handlerfires for reg Fix ?? operator #1 and reg Fix calling #private() functions in classes #2 →fired = 4w.postMessage({cmd:'remove-one'})→handlerfires for reg Fix ?? operator #1 and reg Fix calling #private() functions in classes #2 →fired = 6; then the anonymous listener (registered third) runsremoveListener('message', handler)and posts{removed:true}- Main receives
{removed:true}, posts{n:1}→ the single remaininghandlerregistration fires →fired = 7 - Main posts
{cmd:'report'}→ the single remaininghandlerregistration fires →fired = 8; then the anonymous listener posts{fired: 8}
The outer comment block above the assertion (lines 565-568) traces exactly this and is correct; only the inline fixture comment is stale.
Impact
Zero functional impact — the test passes and asserts the right value. But the file now contains two contradictory explanations of the same count (one wrong at 544-549, one right at 565-568), which will confuse anyone debugging this test in the future.
Fix
Either delete the inline fixture comment (the outer block already explains the count) or correct it, e.g.:
// Three pre-removal messages reach both registrations: {n:1}→2, {n:2}→4,
// {cmd:'remove-one'}→6 (handler fires before the remove-one listener runs).
...
// After removal ack, main posts two more — each fires the single remaining
// registration. Final total: 6 + 2 = 8.
| import { parentPort } from 'node:worker_threads'; | ||
| parentPort.on('message', (_msg, ports) => { | ||
| // Node passes ports as a second argument; the DOM-style MessageEvent | ||
| // also exposes them via event.ports. We test both surfaces below via | ||
| // addEventListener. |
There was a problem hiding this comment.
🟡 The comment "Node passes ports as a second argument" is factually incorrect — Node's parentPort 'message' event passes a single argument (the cloned value) and no further arguments; transferred ports are only reachable via event.ports on the DOM-style addEventListener path (which is what this test actually exercises below). Since the .on() listener body is empty and the ports parameter is unused, the simplest fix is to delete this no-op parentPort.on(...) block entirely.
Extended reasoning...
What the issue is
The worker.mjs fixture in the "transferred MessagePorts" test registers:
parentPort.on('message', (_msg, ports) => {
// Node passes ports as a second argument; the DOM-style MessageEvent
// also exposes them via event.ports. We test both surfaces below via
// addEventListener.
});The comment asserts that Node passes transferred ports as a second positional argument to parentPort.on('message', …) listeners. This is factually incorrect. Per the Node.js docs for the MessagePort 'message' event: "Listeners on this event receive a clone of the value parameter as passed to postMessage() and no further arguments." In Node, transferred MessagePort instances are only reachable either (a) by being included somewhere in the cloned value itself, or (b) via event.ports when using the DOM-style addEventListener('message', e => e.ports) path. There is no second positional argument.
Why nothing else contradicts this
Bun's own implementation in this PR confirms the single-argument convention: invokeEmitterListener (src/js/node/worker_threads.ts) calls listener.$call(fake, payload) with exactly one argument — event.data. So the ports parameter in this fixture's .on() callback is always undefined, the comment misdescribes both Node and Bun, and the listener body is empty anyway.
Impact
Zero functional impact — the listener body is a no-op and the test's actual assertion is driven by the addEventListener block below it, which correctly reads event.ports[0]. The only harm is documentation: this file's whole purpose is to encode Node-compat semantics for parentPort, and a factually-wrong comment about Node's calling convention will mislead future readers (same class of issue as the already-flagged "4+2=6 vs fired:8" comment discrepancy elsewhere in this file at inline-comment 3129793245).
Step-by-step proof
- Main thread:
w.postMessage({ cmd: 'use-port' }, [port2]). - Bun's capture forwarder fires, clones a
MessageEventwith{ data, ports: [port2] }, and dispatches it onparentPortTarget. - The emitter-style wrapper for the
.on()listener callsinvokeEmitterListener('message', listener, event)→listener.$call(fake, event.data)— i.e.listener({ cmd: 'use-port' }). The second parameterportsisundefined. - In Node.js itself,
parentPortis aNodeEventTarget; its'message'event is emitted with a single argument (the deserialized value). Same result:portsisundefined. - Therefore the comment "Node passes ports as a second argument" is wrong in both runtimes, and the
(_msg, ports) => {}signature documents a calling convention that doesn't exist.
Fix
Delete the no-op parentPort.on('message', (_msg, ports) => { … }) block — it serves no purpose in the test (the addEventListener block immediately below already exercises event.ports). Alternatively, correct the comment to say that Node's emitter-style 'message' listeners receive only the cloned value, and that transferred ports must be read from event.ports via addEventListener.
| // calling convention on a MessagePort-like object. | ||
| function invokeEmitterListener( | ||
| type: string, | ||
| listener: EventListener | EventListenerObject, | ||
| event: Event, | ||
| ): void { |
There was a problem hiding this comment.
🟣 Pre-existing: invokeEmitterListener reads (event as ErrorEvent).error for type === 'messageerror', but makeForwarder always re-dispatches a new MessageEvent(type, {data, ports}) — which has no .error property — so parentPort.on('messageerror', fn) receives undefined instead of the deserialization-error payload. This matches pre-PR behavior (the old injectFakeEmitter.errorEventHandler also read .error from the native messageerror MessageEvent), so it's not a regression; a one-line fix is to read (event as MessageEvent).data for the messageerror branch.
Extended reasoning...
What the bug is
invokeEmitterListener (src/js/node/worker_threads.ts:289-294) extracts the payload to pass to emitter-style listeners as follows:
if (type === "error" || type === "messageerror") {
payload = (event as ErrorEvent).error;
} else {
payload = (event as MessageEvent).data;
}However, the only producer of messageerror events on parentPortTarget is makeForwarder, which always constructs the re-dispatched event as:
const clone = new MessageEvent(type, {
data: messageEvent.data,
ports: nativePorts && nativePorts.length > 0 ? $Array.from(nativePorts) : undefined,
});A MessageEvent has .data but no .error property. So when invokeEmitterListener runs for messageerror, it reads clone.error, which is undefined, and the user listener receives undefined instead of the deserialization-error payload.
Why this is pre-existing, not a regression
Before this PR, parentPort.on('messageerror', fn) routed through injectFakeEmitter's functionForEventType, which used errorEventHandler(event) { return event.error; }. The native messageerror event dispatched on self is also a MessageEvent per the HTML spec (and per Bun's MessageEvent.cpp), carrying its payload in .data — not .error. So event.error was undefined pre-PR too. The user-visible behavior of parentPort.on('messageerror', fn) is identical before and after this PR: fn receives undefined.
The PR rewrote the dispatch path from scratch (the producer makeForwarder and consumer invokeEmitterListener are now in the same closure), making the inconsistency self-contained and trivially fixable, but it carries forward the exact same broken pattern rather than introducing it.
Why existing code doesn't prevent it
There is no code path that constructs an ErrorEvent for messageerror on parentPortTarget. makeForwarder is the only thing that calls parentPortTarget.dispatchEvent for messageerror, and it unconditionally builds a MessageEvent. The (event as ErrorEvent) cast is purely a TypeScript type assertion with no runtime effect.
Impact
messageerror only fires on structured-clone deserialization failure — a rare edge case. When it does fire, code like parentPort.on('messageerror', err => console.error(err)) logs undefined instead of the actual error/payload, making the failure harder to diagnose. No crash or hang. The file is also annotated // TODO: parent port emulation is not complete.
How to fix
Change the messageerror branch in invokeEmitterListener to read .data instead of .error:
if (type === "error") {
payload = (event as ErrorEvent).error;
} else {
// 'message' and 'messageerror' are both MessageEvent — payload is in .data
payload = (event as MessageEvent).data;
}(Node.js itself passes the deserialization error as the single argument to 'messageerror' listeners, sourced from the MessageEvent's data slot.)
Step-by-step proof
- Parent posts a value that fails structured-clone deserialization in the worker.
- Bun's native runtime dispatches a
MessageEventof type'messageerror'onselfwithevent.data = <the error>. messageErrorForwarder(installed viainstallMessageErrorForwarder) fires, callsstopImmediatePropagation(), then constructsnew MessageEvent('messageerror', { data: event.data, ports: undefined })and dispatches it onparentPortTarget.- The
wrappedlistener onparentPortTarget(registered for the user'sparentPort.on('messageerror', fn)) callsinvokeEmitterListener('messageerror', fn, clone). type === 'messageerror'matches the first branch →payload = (clone as ErrorEvent).error→cloneis aMessageEvent, has no.error→payload = undefined.fn.$call(fake, undefined)— user listener receivesundefined.
Pre-PR, step 3 didn't exist (the listener was on self directly), but step 5's equivalent (errorEventHandler) read .error from the same native MessageEvent and also got undefined.
…eads workers
Bun's native worker runtime dispatches messages posted by the parent onto
the worker's global scope, so `self.onmessage` and any
`self.addEventListener('message', ...)` listeners fired automatically.
That matches web-worker semantics but does not match Node's
`node:worker_threads`, where parent messages are only observable through
`parentPort` — setting `self.onmessage` in a Node worker is a dead
assignment.
Emscripten-generated pthread code (e.g. z3-solver) relies on the Node
behavior:
parentPort.on('message', (m) => onmessage({ data: m }));
self.onmessage = handleMessage;
Under Bun, every parent message reached `handleMessage` twice — once
via the automatic `self.onmessage` dispatch and once via the explicit
forwarding inside the parentPort listener — so z3's
`removeRunDependency('wasm-instantiate')` assertion failed on the second
call and the worker tore down with an unhandled error.
`fakeParentPort` in `src/js/node/worker_threads.ts` now owns its own
EventTarget. `addEventListener` / `removeEventListener` / `on` / `off`
/ `once` and the `onmessage` / `onmessageerror` accessors route through
it. While at least one user listener is registered, a capture-phase
forwarder is installed on `self` that stops immediate propagation (so
listeners on the global scope never see parent messages) and
re-dispatches on the parentPort's target. The forwarders are attached
lazily and removed when the last user listener goes away, so a worker
that never touches parentPort keeps the event loop refcount it had
before and still exits cleanly when its module finishes executing.
Fixes #29211
…tSignal leak
Addresses three issues in the parentPort EventTarget shim introduced to
stop parent messages from firing self.onmessage:
- The capture forwarder cloned MessageEvents with only `data`, silently
dropping `event.ports`. Any caller doing
`worker.postMessage(data, [port])` and reading `event.ports` from a
parentPort listener lost the transferred port. The clone now carries
`ports` through.
- `parentPortAddEventListener`'s wrapped callback unconditionally called
`listener.$call(fake, event)`, which throws on an EventListenerObject
(`{handleEvent(e) { … }}`). The wrapper now dispatches via a small
`invokeListener` helper that handles both the function and
handleEvent-object forms, matching DOM EventTarget semantics.
- `addEventListener` with `{signal: ac.signal}` passed the signal to the
inner `parentPortTarget.addEventListener`. On abort, the native
EventTarget removed the wrapped listener directly at the C++ level,
bypassing our JS `parentPortRemoveEventListener` and leaving
`listenerCount` at 1 — so the capture forwarder stayed installed on
`self`, pinning the worker event loop forever. We now strip the signal
from the forwarded options and drive abort ourselves, routing the
removal through `parentPortRemoveEventListener` so `releaseListener()`
runs.
Three new regression tests in test/regression/issue/29211.test.ts cover
each case; all three fail on the previous commit and pass after this
change.
Addresses two follow-up review comments on #29215: - `parentPortAddEventListener` called `acquireListener()` unconditionally for every event type — including `close`, `error`, etc. — so `parentPort.once('close', cleanup)` installed the capture-phase message forwarder on `self` even when the user was not listening for messages. The forwarder then intercepted parent messages, called `stopImmediatePropagation()`, and re-dispatched onto `parentPortTarget` where no `message` listener existed — so subsequent parent messages were silently dropped. And since native `close` events never reach `parentPortTarget`, `releaseListener()` never ran, pinning the worker event loop forever. Both `parentPortAddEventListener` and `parentPortRemoveEventListener` now gate `acquireListener`/`releaseListener` on `type === 'message' || type === 'messageerror'`, so listeners for unrelated event types pass straight through to `parentPortTarget` without touching the global-scope forwarder. - Use tamper-resistant intrinsics on the inner listener-tracking Map (`bucket.$get` / `$set` / `$has` / `$delete` / `$size`) and `$Array.from` for the ports clone — following src/js/CLAUDE.md. The outer `WeakMap` stays on plain prototype methods because `.$get` is not defined as an intrinsic for WeakMap. Regression test added: `parentPort listener for non-message events does not block parent messages` — registers `.on('close')` before `.on('message')` and asserts all three posted messages reach the handler. Fails on the previous commit; passes after this change.
…ion tests
Move the `JSON.parse(stdout)` assertion ahead of the
`expect({ stderr, exitCode })` check in all seven regression tests, so a
failure surfaces the actual worker output (handleMessageCalls counts,
fired counters, etc.) before the less-specific exit-code comparison.
Matches the CLAUDE.md convention for spawn-based tests.
Each test spawns an independent Bun subprocess with a unique tempDir and shares no state with the others, so they're safe to run in parallel. Matches the test/CLAUDE.md preference and cuts this suite's wall-clock from ~21s to ~7s.
…rr asserts
Three fixes rolled into one commit after reviewer feedback and a CI
failure on the debian-13-x64-asan lane:
- **Drop the cleanStderr/empty-stderr assertions (CI fix).** The regression
tests compared a hand-filtered stderr against the empty string to assert
the subprocess was clean. That filter only stripped the known
`ASAN interferes with JSC signal handlers` warning; any additional
ASAN noise (leak summaries, SUMMARY lines, stack frames) leaked through
and failed the assertion. Per test/CLAUDE.md convention for regression
tests, the crash/failure signal is `expect(exitCode).toBe(0)`. All
seven original assertions switched over; `cleanStderr` and the stderr
destructuring side are gone.
- **Fix parentPort.removeListener bypassing wrappedListener.**
`injectFakeEmitter`'s `Class.prototype.on` wraps `fn` into a callback
and stores `fn[wrappedListener] = callback`; the tracking WeakMap in
`fakeParentPort` is keyed on the callback. The previous code defined
`fake.removeListener = parentPortRemoveEventListener`, which bypassed
the wrappedListener lookup — so `parentPort.removeListener('message', fn)`
looked up `fn` in the map, missed, and silently kept the listener
(and its capture-forwarder ref) alive. `fake.addListener` /
`fake.removeListener` now alias `MessagePort.prototype.on` / `.off`
from `injectFakeEmitter`, which resolve `listener[wrappedListener]`
before calling `this.addEventListener` / `this.removeEventListener`
(both of which correctly hit `parentPortAddEventListener` /
`parentPortRemoveEventListener` via the own-property overrides on
`fake`).
- **Split message / messageerror listener counts.** `installForwarders`
previously installed BOTH the `message` and `messageerror` capture
forwarders on `self` as a single unit gated by one shared
`listenerCount`. That meant `parentPort.onmessageerror = handler` (or
`.on('messageerror', …)`) alone pulled in the `message` forwarder,
which in turn pinned `m_messageEventCount` via
`onDidChangeListenerImpl` in BunWorkerGlobalScope.cpp and kept the
worker event loop alive forever — a regression from the pre-fix code
where registering only an `onmessageerror` handler exited cleanly.
The forwarders are now installed/uninstalled independently based on
separate `messageListenerCount` and `messageErrorListenerCount`
counters. `acquireListener` / `releaseListener` take a type arg; all
callers thread that through.
Two new regression tests cover the fixes:
- `parentPort.removeListener unsubscribes through the wrapped-listener slot`
- `parentPort.onmessageerror alone does not keep the event loop alive`
Both fail on the previous commit and pass after this change. Each test
also gets a 30-second timeout to absorb the extra variance of running
the suite concurrently under ASAN.
Cross-API removal — `parentPort.on('message', fn)` followed by
`parentPort.removeEventListener('message', fn)` — was silently
no-opping and leaking the capture forwarder's event-loop ref.
`injectFakeEmitter.on` wraps `fn` into a callback and stashes it on
`fn[wrappedListener]`; the tracking WeakMap in `fakeParentPort` keys
that entry on the callback, not the original `fn`.
`parentPortRemoveEventListener` was looking up `listener` directly,
missing the entry, and returning early without decrementing
`messageListenerCount`. The forwarder stayed installed on `self` and
the worker event loop never exited.
`wrappedListener` is promoted from an `injectFakeEmitter`-local symbol
to module scope so `parentPortRemoveEventListener` can follow the same
indirection that `Class.prototype.off` already does:
`(listener as any)[wrappedListener] ?? listener`.
This matches Node's `NodeEventTarget`, which unifies all six
registration/removal APIs in a single registry — cross-API removal is
both documented and idiomatic there, and Emscripten-generated code in
particular often mixes the two styles.
Regression test added: `parentPort.removeEventListener unwraps
listeners registered via .on() (#29211)` — fails on the previous
commit, passes after this change.
…entListener" This reverts commit dc7375b.
test/CLAUDE.md is explicit: "Do not set a timeout on tests. Bun already has timeouts." The suite was setting 30000ms on every `test.concurrent` call. Rely on Bun's global timeout instead.
`parentPort.on(fn)` + `.on(fn)` + single `off(fn)` used to leak an orphaned wrapper forever because the emitter shim stashed each new wrapper at `fn[wrappedListener]`, clobbering the first. The stale slot also broke `add -> off -> addEventListener -> removeListener` cross-API cycles (the final `removeListener` chased a dead slot and missed the direct registration). And an `addEventListener` registered with an `AbortSignal`, removed, re-registered without one, then aborted, evicted the new registration because the abort closure only captured `(type, listener, capture)`. Override `on`/`once`/`off`/`addListener`/`removeListener`/prepend variants directly on `fake`, bypassing `injectFakeEmitter`. All adds go through `addListenerGeneric`, which: - keys tracking by the ORIGINAL user listener (WeakMap<listener, Map<slot, Entry[]>>), so `off(fn)` finds the right entries, - shares one registration list per `(listener, type, capture)` between `on` and `addEventListener`, matching Node where `removeListener` and `removeEventListener` are aliases, - allows duplicate emitter registrations (Node semantics) and removes LIFO, - records `style` per entry so the right calling convention (raw data vs MessageEvent) fires, and - captures the specific TrackEntry in the AbortSignal abort closure and checks `entry.removed` before evicting. Three regression tests added for the exact patterns above.
17af498 to
e06e383
Compare
| list.splice(idx, 1); | ||
| if (list.length === 0) { | ||
| bucket.$delete(slot); | ||
| if (bucket.$size === 0) trackedByListener.delete(listener); |
There was a problem hiding this comment.
🟡 bucket.$size is not a valid Map intrinsic — builtins.d.ts only declares $get/$set/$has/$delete/$clear for Map, so after codegen this becomes bucket.@size which is undefined at runtime, and undefined === 0 is always false. The eager trackedByListener.delete(listener) cleanup is therefore dead code; use the public bucket.size accessor instead. (No functional impact — trackedByListener is a WeakMap and all callers handle empty-but-present buckets correctly.)
Extended reasoning...
What the bug is
evictEntry() ends with:
if (list.length === 0) {
bucket.$delete(slot);
if (bucket.$size === 0) trackedByListener.delete(listener);
}bucket is a plain Map<string, TrackEntry[]>. The $-prefixed intrinsic methods available on Map in Bun's builtin context are exactly those declared in src/js/builtins.d.ts lines 628-634: $clear, $delete, $has, $set, $get. There is no $size. The ClassWithIntrinsics<T> helper at builtins.d.ts:564 only generates $-keys for Function-typed properties, and Map.prototype.size is an accessor getter, not a method, so it's excluded by construction.
The global declare function $size() at builtins.d.ts:486 is a stream-related private name (it sits among sink/start/startAlgorithm/state), and BunBuiltinNames.h:185 registers @size as a parseable private identifier — but registering a private name does not install a property under that name on Map.prototype. JSC's MapPrototype::finishCreation only installs private-name aliases for the method functions, not for the size accessor.
Why this means the cleanup never runs
After codegen (src/codegen/replacements.ts:183 + bundle-modules.ts:255), bucket.$size becomes bucket.@size — a private-name property lookup. Since Map.prototype has no property under that private symbol, the lookup yields undefined. undefined === 0 is always false, so trackedByListener.delete(listener) is unreachable. A grep across src/js/ confirms this is the only .$size usage on a Map instance in the entire codebase — it's not an established pattern.
Step-by-step proof
parentPort.addEventListener('message', fn)→getOrCreateBucket(fn)createsbucket = new Map(),bucket.$set('message:0', [entry]).parentPort.removeEventListener('message', fn)→evictEntry(fn, 'message:0', entry):list.splice(0, 1)→list.length === 0bucket.$delete('message:0')→ bucket is now empty (realsizeis 0)bucket.$size→bucket.@size→undefinedundefined === 0→falsetrackedByListener.delete(fn)is not called
trackedByListenerstill holdsfn → (empty Map).
Why the impact is negligible
This is dead cleanup code only — no observable behavior changes:
- No leak:
trackedByListeneris aWeakMap, so the empty bucket is reclaimed whenfnbecomes unreachable. removeListenerGeneric:getBucket(fn)returns the empty Map,bucket.$get(slot)returnsundefined, theif (!list || list.length === 0) returnguard fires. Correct.addListenerGeneric:getOrCreateBucket(fn)returns the existing empty Map and reuses it, then creates a freshlist. Correct.evictEntry(called again on a stale entry):bucket.$get(slot)returnsundefined→ returnsfalse. Correct.
None of the 12 regression tests in this PR would detect this. It was introduced by mechanically applying the CodeRabbit suggestion in commit 3f220d2, which recommended $size without verifying the intrinsic exists.
Fix
Use the public accessor — there is no $-intrinsic for Map's size getter:
if (bucket.size === 0) trackedByListener.delete(listener);There was a problem hiding this comment.
Good catch — $size isn't declared in builtins.d.ts so this line is dead cleanup code. As you note, the functional impact is zero (trackedByListener is a WeakMap, entries are reclaimed when the listener becomes unreachable). The repo here is currently locked by a concurrent session running git reset so I can't push a fix right now — but the one-line change is trivial: bucket.$size → bucket.size. Either the concurrent session will pick this up when it lands its own changes, or I can sweep it in a follow-up.
|
So glad to see this PR. |
|
Closing: the double delivery this PR targeted was fixed on main by #37075 (merged as 9d519e8). In a Every issue this PR listed is closed as fixed by #37075: #7816, #19453, #25454, #25860, #29211, #29635. Checked with a worker that registers both
main also carries a test for this case, "a parent message reaches parentPort only, not self.onmessage, in a node worker" in script used for the counts aboveimport { Worker } from "node:worker_threads";
const src = `
const { parentPort } = require("node:worker_threads");
if (typeof globalThis.self === "undefined") globalThis.self = globalThis;
const counts = { parentPort: 0, selfOnmessage: 0, globalAddEventListener: 0 };
parentPort.on("message", (m) => { counts.parentPort++; self.onmessage({ data: m }); });
self.onmessage = () => { counts.selfOnmessage++; };
if (typeof self.addEventListener === "function") self.addEventListener("message", () => { counts.globalAddEventListener++; });
parentPort.on("message", () => setImmediate(() => parentPort.postMessage(counts)));
`;
const w = new Worker(src, { eval: true });
const result = await new Promise((resolve, reject) => {
w.once("message", resolve);
w.once("error", reject);
w.postMessage("ping");
});
await w.terminate();
console.log(JSON.stringify(result)); |
Fixes #29211
Fixes #29635
Fixes #25860
Fixes #19453
Fixes #25454
Fixes #7816
Repro
Node prints
sat. Bun aborts withAborted(Assertion failed)inside z3's Emscripten runtime atremoveRunDependency('wasm-instantiate')and then segfaults when the worker error surfaces.Cause
Bun's native worker runtime dispatches messages posted by the parent onto the worker's global scope, so
self.onmessageand anyself.addEventListener('message', …)listeners fire automatically. That matches web-worker semantics but not Node'snode:worker_threads, where parent messages are only observable throughparentPort—self.onmessagein a Node worker is a dead assignment.Emscripten-generated pthread code (and z3-solver) relies on the Node behavior:
Under Bun, every parent message reached
handleMessagetwice — once via the automaticself.onmessagedispatch and once via the explicit forwarding inside the parentPort listener — so z3'sremoveRunDependency('wasm-instantiate')assertion failed on the second call and the worker tore down with an unhandled error, which then surfaced as a segfault in the main thread's error-handling path.Fix
fakeParentPortinsrc/js/node/worker_threads.tsnow owns its own EventTarget.addEventListener/removeEventListener/on/off/once/onmessage/onmessageerrorall route through it. While at least one user listener is registered, a capture-phase forwarder is installed onselfthat callsstopImmediatePropagation()(so handlers on the global scope never see parent messages) and re-dispatches the event on the parentPort target.The forwarders are attached lazily and removed when the last user listener goes away, so a worker that never touches
parentPort(or only sets listeners and then removes them) doesn't leak the event-loop ref that installing amessagelistener onselfotherwise keeps — viaonDidChangeListenerImplinBunWorkerGlobalScope.cpp. That way workers likenew Worker('require("worker_threads").setEnvironmentData(...)', { eval: true })still exit cleanly when their module finishes executing.Verification
test/regression/issue/29211.test.tscovers three scenarios:parentPort.on('message', …)forwarder that manually invokesself.onmessage. With the bug,handleMessagefires twice per parent message and global-scope listeners also fire; with the fix, it fires exactly once and global listeners stay silent (matching Node).parentPortlistener variant (on/addEventListener/onmessage) sees exactly one dispatch per parent message.parentPort.offactually removes a listener.The first test fails on main and passes with the fix. The pre-existing
test/js/node/worker_threads/worker_threads.test.tssuite still passes the same 21 tests it did on main (no new regressions; the same 6 tests time out in this environment both before and after the change).