Conversation
Collaborator
|
we never emit blobs currently though |
Contributor
Author
|
@Jarred-Sumner |
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 -->
Collaborator
|
Thank you for the early attempt at this. Client and server I verified this on current main:
The one part of this PR that #39808 does not include is a I am closing this PR as superseded. If a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prevents #18807 to crash (caused by this), which enables client-server communication.
Unable to provide a test as
WebSocketServerdon't supports it