Skip to content

ws: convert text frames to strings in server socket EventTarget API - #36061

Closed
robobun wants to merge 9 commits into
mainfrom
farm/55de29e7/ws-event-target-text-frames
Closed

robobun wants to merge 9 commits into
mainfrom
farm/55de29e7/ws-event-target-text-frames

Conversation

@robobun

@robobun robobun commented Jul 27, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #36060

Repro

import { WebSocketServer } from "ws";
const server = new WebSocketServer({ port: 0 });
server.on("connection", socket => {
  socket.on("message", (_data, isBinary) => console.log({ isBinary })); // { isBinary: false }
  socket.addEventListener("message", e => console.log(typeof e.data)); // "object" (Buffer), expected "string"
});

A text frame received on a server-side socket is correctly reported as isBinary: false through the EventEmitter API, but addEventListener("message") and the onmessage setter exposed event.data as a Buffer. Node with npm ws@8.x gives a string.

Cause

BunWebSocketMocked in src/js/thirdparty/ws.js converts incoming text to the socket's binaryType representation (a Buffer for the default nodebuffer) before emitting message. The EventTarget-style wrappers (addEventListener and the onmessage setter) then forwarded only the converted data, dropping the isBinary flag that npm ws's event-target shim uses to build the event (data: isBinary ? data : data.toString()).

Fix

The message wrappers now take (data, isBinary) and convert non-binary data back to a string, matching npm ws (verified against ws@8.18.0 on Node 26). The event object also carries type and target like npm ws. While in there:

  • addEventListener now honors { once: true } (previously options were ignored)
  • removeEventListener matches wrapped listeners the way npm ws does, so removing a { once } listener before it fires works
  • the onmessage getter returns the assigned handler instead of the internal wrapper

Two existing tests asserted the old behavior (e.data equal to Buffer.from("hello") for a text frame via onmessage); they were updated to the npm ws behavior.

Verification

New tests in test/js/first_party/ws/ws.test.ts fail on current bun and pass with this change; all 47 tests in the file pass. The 3 failures in ws-proxy.test.ts pre-exist on main without this diff.


[review] gate passed · iteration 1 · 2 files touched

fails on main (without fix)
ASAN without fix: 9 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/first_party/ws/ws.test.ts
bun test v1.4.0 (47a15b898)

test/js/first_party/ws/ws.test.ts:
Listening: ws://127.0.0.1:34289/
(pass) WebSocket > url [1814.76ms]
Listening: ws://127.0.0.1:39925/
Received upgrade: {
  host: "127.0.0.1:39925",
  connection: "Upgrade",
  upgrade: "websocket",
  "sec-websocket-version": "13",
  "sec-websocket-extensions": "permessage-deflate; client_max_window_bits",
  "sec-websocket-key": "XrrY4vamSw2FQDxR8MWrDw==",
}
Received connection: 127.0.0.1
(pass) WebSocket > readyState [2020.25ms]
Listening: ws://127.0.0.1:39071/
(pass) WebSocket > binaryType > (default) [1768.66ms]
Listening: ws://127.0.0.1:46873/
(pass) WebSocket > binaryType > (invalid) [1772.53ms]
Listening: ws://127.0.0.1:44763/
Received upgrade: {
  host: "127.0.0.1:44763",
  connection: "Upgrade",
  upgrade: "websocket",
  "sec-websocket-version": "13",
  "sec-websocket-extensions": "permessage-deflate; client_max_window_bits",
  "sec-websocket-key": "HMzQbX/GRSKKRbPOWk3yBg==",
}
Received connection: 127.0.0.1
Received message: <Buffer
... (truncated)

release without fix: all passed
bun test v1.4.0-canary.1 (6e98bbffa)

test/js/first_party/ws/ws.test.ts:
Listening: ws://127.0.0.1:34629/
(pass) WebSocket > url [43.12ms]
Listening: ws://127.0.0.1:35259/
Received upgrade: {
  host: "127.0.0.1:35259",
  connection: "Upgrade",
  upgrade: "websocket",
  "sec-websocket-version": "13",
  "sec-websocket-extensions": "permessage-deflate; client_max_window_bits",
  "sec-websocket-key": "K/e8DYEoSrCaZEiy4MKCAg==",
}
Received connection: 127.0.0.1
Received close: 1000 
(pass) WebSocket > readyState [47.74ms]
Listening: ws://127.0.0.1:45513/
(pass) WebSocket > binaryType > (default) [31.49ms]
Listening: ws://127.0.0.1:41625/
(pass) WebSocket > binaryType > (invalid) [38.08ms]
Listening: ws://127.0.0.1:35065/
Received upgrade: {
  host: "127.0.0.1:35065",
  connection: "Upgrade",
  upgrade: "websocket",
  "sec-websocket-version": "13",
  "sec-websocket-extensions": "permessage-deflate; client_max_window_bits",
  "sec-websocket-key": "+UYmZi75SlObjh5KL0kQNw==",
}
Received connection: 127.0.0.1
Received message: <Buffer 00>
Received ping: <Buffer >
Received pong: <Buffer >
(pass) WebSocket > binaryType > nodebuffer [43.11ms]
Listening: ws://127.0.0.1:38181/
Rec
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/first_party/ws/ws.test.ts
bun test v1.4.0 (47a15b898)

test/js/first_party/ws/ws.test.ts:
Listening: ws://127.0.0.1:36973/
(pass) WebSocket > url [1804.80ms]
Listening: ws://127.0.0.1:34165/
Received upgrade: {
  host: "127.0.0.1:34165",
  connection: "Upgrade",
  upgrade: "websocket",
  "sec-websocket-version": "13",
  "sec-websocket-extensions": "permessage-deflate; client_max_window_bits",
  "sec-websocket-key": "YHgorM9CQoqtB5HW1K06Sw==",
}
Received connection: 127.0.0.1
(pass) WebSocket > readyState [2030.89ms]
Listening: ws://127.0.0.1:34505/
(pass) WebSocket > binaryType > (default) [1737.75ms]
Listening: ws://127.0.0.1:41013/
(pass) WebSocket > binaryType > (invalid) [1756.17ms]
Listening: ws://127.0.0.1:33729/
Received upgrade: {
  host: "127.0.0.1:33729",
  connection: "Upgrade",
  upgrade: "websocket",
  "sec-websocket-version": "13",
  "sec-websocket-extensions": "permessage-deflate; client_max_window_bits",
  "sec-websocket-key": "zNhwX1f9Qvi6nepKPQgmyA==",
}
Received connection: 127.0.0.1
Received message: <Buffer
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 759ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/22] gen generated_host_exports.rs
generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 240 extern-C blocks audited
[2/22] gen JS modules (bundle-modules)
Preprocess modules (8916ms)
Bundle modules (91ms)
Postprocesss modules (216ms)
Bundle Functions (744ms)
Generate Code (30ms)

[10.02s] Bundled "src/js" for production
  2571 kb
  193 internal modules
  13 native modules
  90 internal functions across 19 files
[2/7] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m   Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m   Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys)
�[1m�[92m   Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety)
�[1m�[92m   Compiling�[0m bun_zlib_s
... (truncated)
diff hotspot
src/js/thirdparty/ws.js           |  56 +++++++++----
 test/js/first_party/ws/ws.test.ts | 170 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 205 insertions(+), 21 deletions(-)

gate history · 3 passed · 0 rejected · iteration 1

evidence per changed file
file                               reads  edits  tests
src/js/thirdparty/ws.js                7     10      0
test/js/first_party/ws/ws.test.ts      4      9      0

BunWebSocketMocked.addEventListener("message") and the onmessage setter
dropped the isBinary flag and forwarded the already-Buffer-converted data,
so text frames surfaced as Buffer in event.data. npm ws constructs the
MessageEvent with isBinary ? data : data.toString().

Also honor the { once } option in addEventListener and match npm ws's
removeEventListener semantics for wrapped message listeners.

Fixes #36060
@coderabbitai

coderabbitai Bot commented Jul 27, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

WebSocket message events

Layer / File(s) Summary
Message payload and handler wrapping
src/js/thirdparty/ws.js, test/js/first_party/ws/ws.test.ts
Text message data is delivered as a string, binary data remains a Buffer or becomes a Blob for binaryType = "blob", and onmessage preserves the assigned handler reference.
EventTarget listener lifecycle
src/js/thirdparty/ws.js, test/js/first_party/ws/ws.test.ts
Message listeners support { once: true } and removal by original callback while maintaining separate onmessage behavior.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy #36060 by making MessageEvent.data a string for text frames while keeping EventEmitter isBinary false and matching ws behavior.
Out of Scope Changes check ✅ Passed The extra updates to once handling, removeEventListener, and onmessage getter are aligned with the linked issue and PR objectives.
Title check ✅ Passed The title is concise and accurately summarizes the main change: EventTarget message events now deliver text frames as strings.
Description check ✅ Passed The description covers the change, motivation, and verification, though it uses custom sections instead of the template headings.

Comment @coderabbitai help to get the list of available commands.

Comment thread src/js/thirdparty/ws.js
Comment thread test/js/first_party/ws/ws.test.ts Outdated
Mirror npm ws's kForOnEventAttribute/kListener symbols so removing a
handler that is also assigned via onmessage only detaches the
addEventListener registration. Move test cleanup into finally blocks.
Comment thread src/js/thirdparty/ws.js Outdated
Comment thread src/js/thirdparty/ws.js Outdated
Comment thread src/js/thirdparty/ws.js Outdated
@robobun

robobun commented Jul 27, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 2:42 AM PT - Jul 27th, 2026

✅ @robobun, your commit 47a15b8981c7a504403e3a9878b371b391cdd3f7 passed in Build #83209! 🎉


🧪   To try this PR locally:

bunx bun-pr 36061

That installs a local version of the PR into your bun-36061 executable, so you can run:

bun-36061 --bun

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/thirdparty/ws.js`:
- Around line 791-796: Update textEventData and the surrounding `#message`
text-frame handling so text frames remain strings when binaryType is "blob",
rather than being converted to Blob before event delivery. Add the blob
binaryType case while preserving existing conversions for string, Buffer, and
ArrayBuffer inputs.

In `@test/js/first_party/ws/ws.test.ts`:
- Around line 376-405: Add an error handler that routes the client-side
WebSocket error to reject in both tests: test/js/first_party/ws/ws.test.ts lines
376-405 and 407-449. In each test, wire the client ws created alongside its
ws.on("open", ...) handler to the existing reject callback, while preserving the
server-side handlers and cleanup 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: 1141c94d-c11c-484a-8827-a420a164db1d

📥 Commits

Reviewing files that changed from the base of the PR and between 6c12afd and be32b79.

📒 Files selected for processing (2)
  • src/js/thirdparty/ws.js
  • test/js/first_party/ws/ws.test.ts

Comment thread src/js/thirdparty/ws.js Outdated
Comment thread test/js/first_party/ws/ws.test.ts
npm ws only applies binaryType to binary frames; text frames always
reach EventEmitter listeners as Buffer and EventTarget listeners as a
string. Drop the per-binaryType text conversion in #message so blob and
arraybuffer sockets match, and wire client errors to reject in tests.
Comment thread test/js/first_party/ws/ws.test.ts
Assign onmessage before addEventListener so the skip branch decides
which wrapper is removed, and reassign onmessage afterwards so a leaked
wrapper is observable. Verified the test fails with the guard deleted.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/js/thirdparty/ws.js (3)

1088-1115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Clear invalid onmessage assignments instead of installing them.

Assigning null removes the old wrapper but installs a new wrapper around null, which crashes on the next message; the unset getter also returns undefined rather than null. Match ws by accepting only callable handlers and returning null when absent. (raw.githubusercontent.com)

Proposed fix
   set onmessage(cb) {
     if (this.#onmessage) {
       this.removeListener("message", this.#onmessage);
     }
+    if (!$isCallable(cb)) {
+      this.#onmessage = undefined;
+      return;
+    }
     const l = createMessageEventWrapper(this, cb, true);
     this.on("message", l);
     this.#onmessage = l;
   }

   get onmessage() {
-    return this.#onmessage?.[kListener];
+    return this.#onmessage?.[kListener] ?? null;
   }
What are the npm ws onmessage getter and setter semantics when the assigned value is null or another non-function?
🤖 Prompt for 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.

In `@src/js/thirdparty/ws.js` around lines 1088 - 1115, Update the onmessage
setter to remove the existing listener and avoid installing a wrapper when cb is
not callable, and make the onmessage getter return null when no handler is
registered. Preserve wrapper installation and handler retrieval for callable
callbacks, matching the onmessage semantics used by ws.

793-803: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Support handleEvent listeners in addEventListener(). Object listeners currently hit listener.$call(...)/this.on(type, listener) and fail instead of dispatching like ws expects. Handle { handleEvent() {} } listeners explicitly, including the non-message events forwarded directly.

🤖 Prompt for 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.

In `@src/js/thirdparty/ws.js` around lines 793 - 803, Update
createMessageEventWrapper and the addEventListener dispatch path to support
object listeners exposing handleEvent(), invoking that method with the event
target as context instead of listener.$call. Apply the same handling to
non-message events forwarded directly, while preserving existing
function-listener behavior and listener registration metadata.

Source: Coding guidelines


1130-1138: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Restrict removeEventListener("message", …) to EventTarget wrappers. l === listener also matches callbacks registered through ws.on("message", listener), so this path removes an EventEmitter subscription it didn’t create. Keep the raw comparison out of the "message" branch and match only the tagged wrapper via kListener.

🤖 Prompt for 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.

In `@src/js/thirdparty/ws.js` around lines 1130 - 1138, Update removeEventListener
so the "message" branch only matches tagged EventTarget wrappers through
kListener, excluding the raw l === listener comparison that can remove ws.on
subscriptions; preserve the existing matching behavior for other event types.
🤖 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/first_party/ws/ws.test.ts`:
- Around line 408-438: The WebSocket message test does not verify the event
metadata. In the ws.addEventListener("message", ...) handler, retain the
received event and assert its type is "message" and its target is the
server-side ws instance, while preserving the existing data assertions.

---

Outside diff comments:
In `@src/js/thirdparty/ws.js`:
- Around line 1088-1115: Update the onmessage setter to remove the existing
listener and avoid installing a wrapper when cb is not callable, and make the
onmessage getter return null when no handler is registered. Preserve wrapper
installation and handler retrieval for callable callbacks, matching the
onmessage semantics used by ws.
- Around line 793-803: Update createMessageEventWrapper and the addEventListener
dispatch path to support object listeners exposing handleEvent(), invoking that
method with the event target as context instead of listener.$call. Apply the
same handling to non-message events forwarded directly, while preserving
existing function-listener behavior and listener registration metadata.
- Around line 1130-1138: Update removeEventListener so the "message" branch only
matches tagged EventTarget wrappers through kListener, excluding the raw l ===
listener comparison that can remove ws.on subscriptions; preserve the existing
matching behavior for other event types.
🪄 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: abe050bb-a8b9-478a-9c30-87856013ebc0

📥 Commits

Reviewing files that changed from the base of the PR and between be32b79 and e68c14d.

📒 Files selected for processing (2)
  • src/js/thirdparty/ws.js
  • test/js/first_party/ws/ws.test.ts

Comment thread test/js/first_party/ws/ws.test.ts Outdated
Assigning a non-function to onmessage now clears the handler instead of
installing a wrapper that throws on the next message, and the getter
returns null when unset, matching npm ws. removeEventListener matches
message listeners only via their wrapper tag so plain .on() subscriptions
are not removed. Tests assert event.type and event.target.
@robobun

robobun commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author

Re the outside-diff findings: commit 53f61f9 makes non-function onmessage assignments clear the handler (previously a wrapper around the value was installed and threw on the next message), makes the getter return null when unset, and restricts removeEventListener's message branch to wrapper-tag matching so plain .on("message") subscriptions are not removed. All covered by new assertions in ws.test.ts.

handleEvent-style object listeners are left out: this shim has never supported them for any event type, and doing it properly means wrapping close/error/open listeners in event objects too, which is a separate change from this fix.

Comment thread src/js/thirdparty/ws.js
Without the guard, removeEventListener(type, undefined) matched any
untagged listener via l[kListener] === undefined and silently removed
it. npm ws treats this as a no-op.
Comment thread src/js/thirdparty/ws.js Outdated
Comment thread src/js/thirdparty/ws.js

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — focused npm ws compat fix with thorough coverage.

What was reviewed:

  • Traced createMessageEventWrapper + #message change: text frames now always Buffer via .on() and string via EventTarget, matching ws@8.18.0; data.toString() always sees a Buffer.
  • removeEventListener lifecycle: verified {once:true} wrappers unwrap correctly through listeners()→removeListener, non-function early-return closes the l[kListener] === undefined hole, and the kForOnEventAttribute skip is now load-bearing per the reordered test.
  • All prior inline findings (cleanup-in-finally, client error wiring, guard testability, undefined-listener match) confirmed addressed in the current diff.
Extended reasoning...

Overview

Fixes #36060 in src/js/thirdparty/ws.js (BunWebSocketMocked, the server-side socket shim). Text frames delivered through the EventTarget-style API (addEventListener('message') / onmessage) now surface as strings instead of Buffers, and #message no longer converts text frames to the binaryType representation — both matching npm ws@8.18.0. Along the way the shim's listener lifecycle was hardened: addEventListener honors {once:true}, removeEventListener matches wrapped listeners via kListener/kForOnEventAttribute tags like npm ws, the onmessage getter returns the original handler, and assigning a non-function clears it. ~56 net lines in the shim, ~170 lines of new tests plus two existing assertions updated to the corrected behavior.

Security risks

None. This is a JS-level third-party compat shim; no auth, crypto, filesystem, or untrusted-input parsing paths are touched. The tamper-resistance idioms ($isCallable, listener.$call) are used for the new call sites.

Level of scrutiny

Medium. It's a Node-compat behavior change with a clear reference implementation (npm ws's event-target.js), and the PR description documents empirical verification against ws@8.18.0 on Node. The listener-matching logic is the trickiest part and went through four rounds of inline feedback (onmessage/removeEventListener interaction, test load-bearingness, undefined listener matching), each addressed with a follow-up commit and covered by an assertion in the new test. The two updated existing tests are the expected consequence of fixing the behavior they had certified.

Other factors

  • All 47 tests in the file pass per the evidence block; the release-build without-fix run shows 2 failures (the two headline tests), confirming the tests fail for the right reason.
  • The residual asymmetry (non-message addEventListener listeners remain unwrapped, so removeEventListener still sees plain .on('close', h) subscriptions) is pre-existing behavior, not introduced here, and out of scope for this fix.
  • No open review threads remain; all bot and prior claude comments are resolved in the current head (6e98bbf).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — well-scoped npm ws compat fix with thorough test coverage; all prior review threads have been addressed.

What was reviewed:

  • createMessageEventWrapper + addEventListener/onmessage/removeEventListener rewrite against npm ws's event-target semantics; {once: true} interacts correctly with listeners() unwrapping and removeListener's .listener match.
  • The #message change (text frames always Buffer.from, ignoring binaryType) — verified no other tests depend on the old arraybuffer/blob text-frame behavior.
  • removeEventListener edge cases from earlier rounds (non-function listener, kForOnEventAttribute skip, plain .on() invisibility) — all now guarded and pinned by load-bearing assertions.
Extended reasoning...

Overview

Fixes #36060: server-side ws sockets exposed text-frame event.data as a Buffer through the EventTarget API (addEventListener('message') / onmessage), diverging from npm ws which delivers strings. The change is confined to src/js/thirdparty/ws.js (BunWebSocketMocked, ~56 lines) plus ~170 lines of new tests in test/js/first_party/ws/ws.test.ts. Along the way it fixes several adjacent shim bugs: {once: true} was ignored, removeEventListener couldn't unregister wrapped message listeners, the onmessage getter returned the internal wrapper, and assigning a non-function to onmessage installed a broken wrapper.

Security risks

None. This is a JS-only third-party compat shim; no native code, auth, crypto, filesystem, or untrusted-input parsing. The only "hardening" is tamper-proof .$call usage per src/js conventions, which the new wrapper follows.

Level of scrutiny

Medium. It's not a mechanical one-liner — the listener-wrapping/tagging logic is subtle enough that it took four review rounds to shake out edge cases (kForOnEventAttribute skip ordering, removeEventListener(type, undefined) matching untagged listeners, load-bearing test coverage for the guard). But it's contained to one thirdparty shim with a clear reference implementation (npm ws@8.18.0, verified per the PR thread), and each edge case now has a dedicated assertion that fails without the corresponding guard. I re-traced removeEventListener against events.ts's listeners() (returns a fresh mapped array, so safe to mutate during iteration) and removeListener (matches list[i].listener === l, so onceWrapper removal works).

Other factors

  • All prior review threads (mine, coderabbit, comment-cop) are resolved in the current diff; the last two commits are a comment trim and a CI retrigger with no code change.
  • The one user-visible behavior change beyond the headline fix — #message now emits text frames as Buffer regardless of binaryType on the EventEmitter path — is a correctness fix toward npm ws parity, and grep found no other tests depending on the old behavior.
  • Tests follow harness conventions: port: 0, error→reject on both client and server sockets, cleanup in finally, no sleeps.
  • Two pre-existing tests that asserted the old (wrong) Buffer behavior for onmessage text frames were updated; this is the correct move for a compat fix.
  • The sibling onclose/onerror/onopen setters were not given the same non-function guard as onmessage; the author explicitly scoped that out as requiring event-object wrapping for all four types, which is reasonable for a follow-up.

@robobun

robobun commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: the ws changes and their tests pass on every lane. The remaining failures in build 83209 are unrelated known-flaky tests on other subsystems (hosted-git-info boundary-conditions timeout on x64-asan, no-orphans and run-crash-handler timeouts on macOS, issue 20144 on macOS aarch64); no-orphans was already reported for main-break triage from build 83184. The diff is ready for review.

Jarred-Sumner pushed a commit that referenced this pull request Aug 22, 2026
…to ServerWebSocket (#39808)

### Problem
- The built-in `ws` package differs from npm ws 8.18.3. A server socket
with `binaryType = "arraybuffer"` emits a plain `Uint8Array`, not an
`ArrayBuffer`. Ping and pong payloads follow `binaryType`, npm ws emits
a `Buffer`. The client socket rejects `"blob"` (#8721, #26669), which
the native `WebSocket` supports.
- The server socket converted binary frames in JS (`#message`,
`src/js/thirdparty/ws.js`).

### Fix
- `ServerWebSocket.binaryType` accepts `"blob"`. `binary_to_js`
(`src/runtime/server/ServerWebSocket.rs`) builds the `Blob` like the
other three types, so it applies to messages, pings and pongs, as on the
client `WebSocket`. The stored type is a socket local enum of the four
values. The old spellings still work.
- Both shim sockets forward `binaryType` to their native socket as is.
The server socket no longer converts binary frames, the client socket
accepts `"blob"`.
- The shared `controlPayload` wraps ping and pong payloads in a `Buffer`
in `"arraybuffer"` mode, as npm ws emits one. A `Blob` cannot be
unwrapped synchronously, so `"blob"` mode emits it as is.
- Verified: `test/js/first_party/ws/ws.test.ts` (one table of shapes for
both sockets), `test/js/bun/websocket/websocket-server.test.ts`,
`test/integration/bun-types`. Stock bun fails the `arraybuffer` and
`blob` cases.

### Background
- Bun replaces npm `ws` with `src/js/thirdparty/ws.js`. `BunWebSocket`
wraps the native client `WebSocket`. A server connection is a
`BunWebSocketMocked` over a `Bun.serve` `ServerWebSocket`, which calls
its `#message`, `#ping` and `#pong`.
- `ServerWebSocket` keeps its binary type in a 4 bit field of its packed
`Flags` word. `binary_to_js` reads it for every binary frame, ping and
pong.
- In npm ws, `binaryType` only changes binary data frames. Pings and
pongs are a `Buffer`.

<details><summary>Notes</summary>

Repro (raw client, so the bytes are exact). It prints `Uint8Array` on
Bun 1.4.0 and on main, and `ArrayBuffer` on node with ws@8.18.3
(installed in `test/node_modules`):

```js
import { WebSocketServer } from "ws";
import net from "node:net";
const wss = new WebSocketServer({ port: 0, host: "127.0.0.1" });
wss.on("connection", ws => {
  ws.binaryType = "arraybuffer";
  ws.on("message", (data, isBinary) => {
    console.log(isBinary, data instanceof ArrayBuffer ? "ArrayBuffer" : data.constructor.name);
    process.exit(0);
  });
});
wss.on("listening", () => {
  const c = net.connect(wss.address().port, "127.0.0.1", () => c.write(
    "GET / HTTP/1.1\r\nHost: x\r\nUpgrade: websocket\r\nConnection: Upgrade\r\nSec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\nSec-WebSocket-Version: 13\r\n\r\n"));
  c.once("data", () => c.write(Buffer.from([0x82, 0x83, 0, 0, 0, 0, 1, 2, 3])));
});
```

With ping, pong, binary and text frames in `arraybuffer` mode, main
prints `ping: Buffer, pong: Buffer, binary: Uint8Array, text:
ArrayBuffer`. node prints `Buffer, Buffer, ArrayBuffer, Buffer`. This PR
fixes the binary column and keeps the ping and pong columns. The text
column is the text branch of the same method. #36061 changes that branch
(it also removes the remaining `new Blob` there), so this PR leaves it
alone and the new tests do not assert the shape of text frames.

History of this PR:
1. a3da3e3 emitted `message.buffer` in the shim. Review asked for the
`ArrayBuffer` to be created natively.
2. 5a54f28 forwarded `"arraybuffer"` to the native socket and kept
wrapping `"blob"` in JS. Review asked for native blob support instead of
`new Blob`.
3. 355c538 made the native socket build every type. Follow up commits:
e4da89b (no constructor argument), bcc9f9f (GC size report), 18a6e57
(client socket).
4. 18a6e57 applies the same rule to the client socket. The self review
of 355c538 found that the client class in the same file had the same
divergences (ping and pong payloads emitted in the shape of
`binaryType`, `"blob"` rejected), and that `ws.test.ts` would otherwise
pin opposite rules for the two classes. The client change is the shared
helper, one accepted value in the setter, and the two emit lines. #18845
is an older attempt at the client `"blob"` value from before the native
client supported it, it wraps in JS and makes `send()` asynchronous.
This PR makes it unnecessary.

355c538 is the current native design. The ping and pong behavior in
`blob` mode is the one open question, noted in the comments below.
Making `"blob"` apply to messages only would be a small change in
`on_ping` and `on_pong`.

Why a socket local enum: the shared `bun_jsc::BinaryType` is also used
by UDP sockets and the HTTP/2 frame parser, and it cannot build a
`Blob`. Before this PR the socket stored that enum but only ever three
of its values. The local enum has exactly the storable values, so the 14
arm decode, the wildcard arm in `binary_to_js` and the `panic!` arm in
the getter go away. The accepted spellings are copied from the shared
map for the three old values.

The shim socket no longer takes a `binaryType` constructor argument
(e4da89b). Its field starts as `"nodebuffer"`, the default of the native
socket, so only the setter can change the mode and the two sides cannot
start out of sync. The ping and pong JSDoc in `serve.d.ts` says that the
payload follows `binaryType` and that the declared `Buffer` type is the
one of the default mode.

The blob arm wraps through `BlobExt::to_js` (bcc9f9f), like the slice
path in `Blob.rs`. The by-value `JsClass::to_js` skips
`calculate_estimated_byte_size`, so the GC would see a few bytes per
frame. The test "blob frames report their bytes to the garbage
collector" holds a 2 MiB blob frame and checks that
`heapStats().extraMemorySize` grows by at least half of it after
`Bun.gc(true)` (about 2 MiB with the fix, 663 bytes without). The bound
is half the payload because memory that earlier tests release in the
meantime lowers the number a little.

Behavior change for shim client users in `arraybuffer` mode: ping and
pong payloads were an `ArrayBuffer` and are now a `Buffer` view of it,
as in npm ws. The client test at the top of `ws.test.ts` asserted the
old shape and now asserts the new one. Messages are unchanged.

Behavior change for shim users in `blob` mode: ping and pong payloads
were a `Buffer` on main (the native socket stayed in `nodebuffer` mode)
and are now a `Blob`. `arraybuffer` mode is unchanged for pings and
pongs. `nodebuffer` mode is unchanged everywhere.

Tests:
- `ws.test.ts`: one `binaryTypes` table at the top holds the message
shape and the ping/pong shape per mode. The client block (echo server
subprocess) records the shapes of the echoed message, ping and pong into
one object per mode. The server block runs `it.each` over the same
table. The client sends a ping, a pong, a 3 byte binary frame and an
empty binary frame. One `toEqual` checks the shape and the bytes of
every event, plus `isBinary`, including the per mode shape of ping and
pong. A second test covers the default, the getter, and a change of mode
between frames (`arraybuffer`, then `blob` with a ping in between, then
`nodebuffer`). A third test sets the value in the `close` listener, when
the native handle is gone.
- `websocket-server.test.ts`: `blob` joins the binaryType matrix
(message, ping and pong are a `Blob`, the getter returns `"blob"`). A
second test checks `size`, `type` and the bytes of the three blobs. A
third test pins the old spellings. A fourth test checks the GC size
report described above.
- `test/integration/bun-types/fixture/serve-types.test.ts` asserts the
`binaryType` union. Removing `"blob"` from the assertion fails the types
test.

Without the `#ping`/`#pong` wrapping, the `arraybuffer` row of the shim
matrix fails. With a setter that forwards only `"arraybuffer"`, the mode
change test fails. Both were checked by mutating the source.

Suites run with the debug build: all of `ws.test.ts` (53 pass), the
`binaryType` block and the `send` related tests of
`websocket-server.test.ts`, the bun-types integration test, `cargo
clippy -p bun_runtime -p bun_jsc`. When the whole of
`websocket-server.test.ts` runs at once in this container, the 28 s
`send() (benchmark)` test starves 8 concurrent siblings past their 10 s
timeout. They pass when the benchmark is not in the filter, and they do
not touch this change. `ws-proxy.test.ts` has 3 failures that depend on
the ambient `HTTP_PROXY` variables of this container, see #37439.

Other open PRs that edit these files: #36061 (text branch of
`#message`), #39802 (`#close`, `#drain`, `send()`), #39642, #36650,
#39093, #39370 (`websocket-server.test.ts` fixture). None of them
touches `#ping`, `#pong`, the binary branch, the `binaryType` setter or
`binary_to_js`.
</details>

<!-- robobun:evidence:begin -->

---

**no test proof** · iteration 0 · Platform-specific test(s) that do not
run on this machine. Deferring to CI, which covers all platforms:
test/integration/bun-types/fixture/serve-types.test.ts
test/js/bun/websocket/websocket-server.test.ts

<!-- robobun:evidence:end -->
robobun added a commit that referenced this pull request Sep 27, 2026
The socket that a WebSocketServer hands to 'connection' gets addEventListener,
removeEventListener and the four on<event> accessors of npm ws 8.21.0
(lib/event-target.js): one tagged adapter per listener in the socket's own
EventEmitter list, and the event classes of npm ws, created on first use.

A listener gets an event object (MessageEvent with a string for a text frame,
CloseEvent, ErrorEvent, Event). A value that is not a function clears an
on<event> handler. { once: true }, { handleEvent } objects and the duplicate
check work. removeEventListener removes addEventListener registrations only.
A text frame is a Buffer for every binaryType.

Carries the tests and the text-frame change of #36061.
@robobun

robobun commented Sep 27, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: #44111 supersedes this PR.

@robobun robobun closed this Sep 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ws WebSocketServer addEventListener("message") returns Buffer for text frames

1 participant