ws: close server sockets through the drain path and keep them CLOSING until 'close' is emitted - #39802
ws: close server sockets through the drain path and keep them CLOSING until 'close' is emitted#39802robobun wants to merge 5 commits into
Conversation
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 29 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 12 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: head b86b381 (rebased onto main). CI build 103395 finished with 160 of 181 jobs green. All 21 red jobs are the same break from main's #38952: Reproduced on bun 1.4.0 with a raw RFC 6455 client against
Tests: the block "server socket close lifecycle" in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked that Buffer.from(reason) in #close is safe (native on_close always passes a JS string via create_utf8_for_js, never undefined), and that deferring 'close' to setImmediate doesn't break wss.clients cleanup — completeUpgrade registers its clients.delete(ws) listener before user code sees the socket, and the new test asserts clients: 0 at emit time.
Extended reasoning...
The PR reworks server-side ws socket close lifecycle (readyState timing, deferred 'close' emission, send-after-close callbacks, bufferedAmount, #drain retry semantics). It's well-researched against npm ws 8.18.3 with 6 new tests, but it's a non-trivial timing/behavioral change to a widely-used compat shim, so a human look is appropriate. The one finding is a pre-existing edge case (zero-length send() misread as dropped) in the same return-value contract the PR reworks. I verified two adjacent concerns were not issues: the new Buffer.from(reason) cannot receive undefined because the native close handler always constructs a string, and the setImmediate deferral of 'close' still lets wss.clients/_shouldEmitClose shutdown work because that listener is attached in completeUpgrade before the connection callback runs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed the follow-up commits and didn't find any bugs. The wasDropped() fix for empty payloads (from the earlier review) is applied at both call sites and covered by a wire-level test, and the comment-cop feedback is addressed. Since this reworks the server-socket close lifecycle (sync → setImmediate emission, chosen specifically to work around a separately-reported nextTick bug) and changes the 'close' reason type, a human look at the timing decision and the kernel-buffer assumptions in the two Windows-skipped backpressure tests would be worthwhile.
What was reviewed: the CLOSING/CLOSED state transitions and re-entry from close()/terminate() while an immediate is pending — #ws is nulled so the second call is a no-op; Buffer.from(reason) — the native on_close always passes a JS string, so no undefined risk; the -1 (uws-buffered) path in send()/#drain now correctly dequeues and fires the callback instead of resending; payloadByteLength covers strings, ArrayBuffer/views (.byteLength), and Blob (.size).
Extended reasoning...
Overview
This PR reworks the server-side socket state machine in the ws shim (src/js/thirdparty/ws.js, ~70 lines changed) and adds a 340-line test block (test/js/first_party/ws/ws.test.ts). The core change: #close now sets CLOSING, nulls #ws, fails queued send callbacks, and defers the CLOSED transition + 'close' emission (with Buffer.from(reason)) to a setImmediate. Supporting changes: send() after close now calls back with the npm-ws error and counts bytes in bufferedAmount; bufferedAmount includes native getBufferedAmount(); #drain uses per-entry byte lengths and no longer resends messages uws accepted with -1; a new wasDropped() helper distinguishes a delivered empty frame from a backpressure drop.
Security risks
None identified. This is Node-compat lifecycle/state logic in a JS shim; no auth, crypto, or untrusted-input parsing paths are touched.
Level of scrutiny
Medium-high. The ws shim is loaded for every user of the ws package under Bun and is a compatibility surface. The change moves 'close' emission from synchronous to a later event-loop turn — that is the correct npm-ws behavior, but it's a timing change that could interact with user code that (incorrectly) depended on the old synchronous emit. The setImmediate (vs nextTick) choice is explicitly a workaround for a runtime bug the author says is reported separately; a maintainer should be aware of that coupling.
Other factors
The earlier review's empty-payload finding was fixed in 9334774 with wasDropped() and a test that asserts exact frames on the wire. Comment-cop feedback (long comments) was addressed in 7d90a9c/3e823e6. The seven new tests are hermetic (raw TCP client, port: 0), assert exact values, and are documented as failing on the released build. Two backpressure tests are skipIf(isWindows) with a stated reason matching an existing precedent in test/js/bun/websocket/. First CI build was green apart from an unrelated aarch64 flake. I checked that terminate() and close() called while the deferred immediate is pending are no-ops (both guard on state / #ws), and that the native close handler always passes a string reason so Buffer.from(reason) is safe.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This PR is garbage. Completely rewrite this PR from scratch.
We should not be using setImmediate anywhere here at all. We should use the drain event and send whatever was undelivered. send enqueues the undelivered data already.
3e823e6 to
754d6ca
Compare
robobun
changed the title
|
Rewritten from scratch in 754d6ca (plus a one-line comment trim in 4ba3ea0). No
Doing it that way exposed a uws problem: a The PR description is rewritten to match. |
1607583 to
ca65c95
Compare
There was a problem hiding this comment.
I reviewed this again and didn't find any new issues — the last round's note about the raw client not being torn down on assertion failure is addressed in 76b5fd6 (rawClient is now Disposable and every test holds it with using). Because this reworks the server-socket close/drain state machine and touches the native uws onWritable shutdown path, a human look would still be worthwhile.
What was reviewed:
#close/#drain/#pendingCloseinteraction — queued messages go out before the native close, undelivered callbacks fail with the CLOSING error, late-send bytes stay inbufferedAmount(per-entry subtraction, not a blanket zero).wasDropped/payloadByteLength— 0-byte payloads and non-string/non-buffer values are now sized as the native side sends them, so a native0is only treated as a drop for a non-empty frame.- The
WebSocketContext.hchange — mirrors the existingonDatapost-consume shutdown; guarded by!us_socket_is_closedandisShuttingDown, so it only fires whenend()was called inside the corked drain handler. - Test cleanup —
using clientdisposes the raw socket on every exit path, including thrown assertions.
Extended reasoning...
Overview
Four files: src/js/thirdparty/ws.js (the ws shim's server-socket BunWebSocketMocked — close(), terminate(), send(), #drain, #close, bufferedAmount), packages/bun-uws/src/WebSocketContext.h (a post-drain-handler shutdown in onWritable), and ~310 lines of new tests across test/js/first_party/ws/ws.test.ts and test/js/bun/websocket/websocket-server-backpressure-buffer.test.ts. Net effect: the shim's server socket now stays CLOSING until 'close' is emitted asynchronously with a Buffer reason, queued messages drain before the Close frame, send() after close calls back with the npm-ws error and counts toward bufferedAmount, and a close() issued inside the uws drain handler now sends the FIN promptly instead of waiting for the 16 s idle timer.
Security risks
None identified. No auth, crypto, or untrusted-input parsing changes. The uws change adds a shutdown() call gated on state that end() already set; it can't be reached with the socket already closed (!us_socket_is_closed) or without isShuttingDown.
Level of scrutiny
Moderate-to-high. This is a state-machine rework of the ws server socket's close lifecycle plus a native change in vendored uws. The PR has been through three review rounds here: the empty-payload / wasDropped sentinel, the payloadByteLength fallback for coerced values, the #bufferedAmount per-entry subtraction, and the test-cleanup pattern were all raised and fixed. The design was also rewritten once (from setImmediate to routing close through the drain path). That churn, the native-code edit, and the behavioral surface (every WebSocketServer connection's close path) make this worth a maintainer's eyes rather than an auto-approve.
Other factors
- All earlier inline findings are addressed in the current head; commit 76b5fd6 specifically resolves the last open one by making
rawClientaDisposableheld withusing, so the raw TCP socket is destroyed on assertion failure beforewss.close()runs. - Test coverage is thorough: 7 lifecycle tests plus one plain
Bun.servetest for the uws FIN fix, all verified to fail on the released build per the PR notes. The backpressure-dependent tests areskipIf(isWindows)for the same reason as the neighbouring tests in that file. - The
'close'reason is nowBuffer.from(reason)— I checked thatreasonis always a string on this path (nativeon_closepasses a string,terminate()yields""), soBuffer.fromwon't throw.
There was a problem hiding this comment.
I re-reviewed after 76b5fd6 and 38abaf6 — the using-based client disposal, per-entry #bufferedAmount subtraction, payloadByteLength string-coercion fallback, and the new 'connection'-handler test all address the earlier rounds. No further findings this pass. Given the rework of the server-socket close/drain state machine and the onWritable shutdown addition in vendored uws, a maintainer look is still worthwhile.
What was reviewed:
#close/#drain/close()/send()state transitions in ws.js against the nativesend()return-value contract and re-entrancy fromWebSocket::end()'s synchronouscloseHandler.- The
WebSocketContext.honWritablechange:us_socket_is_closedguard,isShuttingDowngate, andgetBufferedAmount() == 0— mirrors the existingonDatapost-consume check. - The new 'connection'-handler test vs. the description's
nextTicknote — the request dispatchers hold an event-loop scope (enter_event_loop_scope_without_checkpointinsrc/runtime/server/mod.rs), soon_close's nested scope does not drain the deferred emit beforeclose()returns. - Test hermeticity:
rawClientnow returns a Disposable and every test holds it withusing.
Extended reasoning...
Overview
The PR reworks the server-side socket in Bun's built-in ws shim (src/js/thirdparty/ws.js, BunWebSocketMocked) so that close() routes through the drain path: queued messages go out first, the native close is issued once getBufferedAmount() is 0, #close keeps the socket in CLOSING and defers 'close' (with a Buffer reason) to process.nextTick, send() on a non-OPEN socket calls back with the npm-ws "not open" error and counts bytes in bufferedAmount, and a native 0 return is treated as a drop only for non-empty payloads. It also adds a 6-line change to packages/bun-uws/src/WebSocketContext.h so that a close() issued inside the (corked) drain handler still sends the TCP FIN, mirroring what onData already does after consume(). Eight new tests cover the lifecycle and the uws FIN case.
This is my fourth pass. The three earlier rounds each surfaced real issues (empty-payload send() misclassified as dropped; payloadByteLength returning 0 for coerced non-string/non-buffer values; blanket #bufferedAmount = 0 wiping late-send bytes; test cleanup ordering) — all were fixed in follow-up commits and the threads are resolved. The commits since my last comment (76b5fd6, 38abaf6) are the using-based client disposal and one added test.
Security risks
None identified. The change is to a Node-compat shim's close/drain sequencing and a targeted uws shutdown() call; no auth, crypto, untrusted-input parsing, or resource-limit handling is touched.
Level of scrutiny
High. The ws.js side is a state-machine rewrite over a re-entrant native layer (uws runs the close handler synchronously inside end()), and the WebSocketContext.h change is in vendored uws on the writable path for every server WebSocket. Three review rounds each found something; the churn history alone argues for a maintainer sign-off on the final shape and the uws hunk.
Other factors
The one candidate issue this run — that the new "close() inside the 'connection' handler" test contradicts the description's known-limitation note — was examined and refuted: the node-http request dispatchers hold enter_event_loop_scope_without_checkpoint (src/runtime/server/mod.rs:1002/1149/1201), so on_close's nested enter_event_loop_scope does not reach a zero enter-count and the deferred 'close' does not drain before close() returns on that path. All comment-cop flags on ws.js are resolved. CI on the pre-38abaf6 head was reported green modulo an unrelated Docker-less MySQL lane; build 102180 for the current head is in progress. Not approving because this is not a simple/mechanical change and it edits vendored uws.
|
Heads up from #39843: it moves the FIN into |
…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 -->
… until 'close' is emitted The server side socket of the ws shim sits on the Bun.serve websocket callbacks, and uws runs the close callback from inside ws.close() and ws.terminate(). The shim moved to CLOSED and emitted 'close' before close() returned, so a listener added after close() never fired and readyState read 3 where npm ws reads 2. close() also went straight to the native close while messages were still queued for the drain callback, which lost them, and once uws was over its backpressure limit the Close frame itself was dropped and the connection cut. close() now records the pending close and runs the drain step: queued messages go out first, and the native close is issued from the drain callback once uws reports nothing buffered. The close callback keeps the socket in CLOSING, fails the callbacks of anything still undelivered, and emits 'close' on the next tick with the reason as a Buffer, as npm ws does. send() on a closing or closed socket calls back with the "not open" error and adds the payload to bufferedAmount, bufferedAmount includes what uws has buffered, queue entries carry their byte length, and a 0 from the native send() only counts as a drop when the payload is not empty (a delivered empty frame also returns 0). The drain step no longer resends a message that uws accepted with backpressure. uws itself did not send the TCP FIN for a close() issued inside the drain handler: Bun runs that handler corked, so end() left the FIN to the uncork, and onWritable never sent it, unlike onData. The connection only went away through the end() timer. onWritable now shuts the socket down after the handler the same way onData does.
… bytes in bufferedAmount at close A payload that is neither a string nor buffer-like goes out as the text frame of its string form, so payloadByteLength() sizes it that way instead of returning 0, which made a drop of such a message at the backpressure limit look like a delivered empty frame. The close callback now subtracts the queued entries it discards instead of zeroing the counter, so bytes counted by a send() during CLOSING stay, as in npm ws.
With request handlers running inside the event loop (#39826) the 'close' event deferred from the close callback no longer fires before close() returns when the socket is closed from the 'connection' handler, so pin that case too.
38abaf6 to
b86b381
Compare
There was a problem hiding this comment.
I re-reviewed after the latest commits (through b86b381) and found no new issues — the earlier findings (empty-payload drop detection, payloadByteLength coercion fallback, per-entry #bufferedAmount subtraction in #close, using-based client disposal, and the restored 'connection'-handler test) are all addressed. Given the state-machine rework of the server socket close path, the vendored uws change in WebSocketContext.h, and the open coordination with #39843 over that same hunk, a human look before merge would still be worthwhile.
What was reviewed
#close/#drain/close()/send()lifecycle: CLOSING held until the deferred'close', queued messages drained before the native close, late-send bytes retained inbufferedAmount.onWritableshutdown-after-drain inWebSocketContext.hmirrors the existingonDatapost-consume()FIN path; guarded by!us_socket_is_closed && isShuttingDown && bufferedAmount==0.- The 8 lifecycle tests + 1
Bun.servedrain-close test: raw RFC 6455 clients,usingdisposal,skipIf(isWindows)on the backpressure cases; verified they map onto the fixed paths.
Extended reasoning...
Overview
Four files: the ws shim's server socket (BunWebSocketMocked) close/drain/send state machine in src/js/thirdparty/ws.js, a 5-line addition to onWritable in vendored packages/bun-uws/src/WebSocketContext.h, 8 new tests in test/js/first_party/ws/ws.test.ts ("server socket close lifecycle" block) driven by a raw RFC 6455 client, and 1 new test in test/js/bun/websocket/websocket-server-backpressure-buffer.test.ts for the FIN-after-drain case against plain Bun.serve.
Security risks
None identified. No auth, crypto, or untrusted-input parsing surface changes. The WebSocketContext.h change adds a conditional asyncSocket->shutdown() after the drain handler, mirroring the pattern already in onData; it does not touch frame parsing or bounds.
Level of scrutiny
High. This reworks the observable lifecycle of every ws-package server socket (readyState after close()/terminate(), 'close' emission timing and reason type, bufferedAmount, send() callbacks after close), and edits vendored uws C++ that every Bun.serve websocket goes through. It is Node/Web-compat work per the situational REVIEW.md sections. The PR has already been through three automated review rounds where five distinct issues were found and fixed; the current run found nothing new.
Other factors
- All prior findings are verifiably addressed in the current diff:
wasDropped()gates onpayloadByteLength(data) !== 0;payloadByteLengthfalls back toBuffer.byteLength(String(data))for non-buffer/non-string inputs;#closesubtracts each queued entry'sbyteLengthinstead of a blanket= 0;rawClient()returns aDisposableheld withusing; and the 'connection'-handler test is present now that #39826 (event-loop entry for request handlers) has landed in main. - robobun's own note flags that #39843 moves the FIN into
WebSocket::end()itself, which would supersede theWebSocketContext.hhunk here. Deciding merge order and whether to drop that hunk in favor of #39843 is a coordination call for a maintainer. - The uws change is small and pattern-matched to
onData's existing post-consume()shutdown, but it is a change to shared native code in a hot path — the kind REVIEW.md says warrants human review.
For those reasons I'm deferring rather than shadow-approving.
|
A second report of the first Problem bullet arrived, with a user-visible leak.
On Bun the listener never runs. Each such client leaves a live interval that holds the socket, the context and the request. A server that rejects a client in the
The Reproconst { WebSocketServer, WebSocket } = require("ws");
const wss = new WebSocketServer({ port: 0, host: "127.0.0.1" });
let closeEvents = 0;
wss.on("connection", socket => {
socket.close(4406, "Subprotocol not acceptable");
socket.once("close", () => closeEvents++);
});
wss.on("listening", async () => {
const N = 20;
for (let i = 0; i < N; i++) {
await new Promise(resolve => {
const c = new WebSocket(`ws://127.0.0.1:${wss.address().port}`);
c.on("close", resolve);
c.on("error", () => {});
});
}
await new Promise(r => setTimeout(r, 300));
console.log(`server-side 'close' events ${closeEvents}/${N}, wss.clients.size=${wss.clients.size}`);
process.exit(0);
}); |
… upgrade request (#43153) ### Problem - A client sends frames in the same TCP read as the upgrade request. `Bun.serve` answers 101 and never sees them: `server saw: ["later"] ping events: 0 pongs on the wire: 0`. A frame cut by the read closes the connection later. - After `server.upgrade()`, `HttpContext::onData` (`packages/bun-uws/src/HttpContext.h:560`) stops the HTTP parser, sends the 101 and returns the WebSocket (line 746). Nothing parses the rest of the read. ### Fix - `consumePostPadded` reports how much of the read it used. `onData` sends the 101, then gives the rest to the WebSocket with `us_dispatch_data`, in the same call. It stores no bytes. A declared request body is never handed over. - This matches `ws` on Node, whose upstream test now passes. A `server.upgrade()` in a later event-loop turn still gets `400 Bad Request` (#43149 tracks the `ws` shim). - Only for the socket of this read: `upgradedWebSocket` can name another connection's WebSocket. Without the check, a test shows one connection's bytes in another's `message` handler. - Verified: `test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts` (8 of 13 fail before), three tests in `test/js/first_party/ws/ws.test.ts`, plus the websocket, `serve.test.ts` and node:http upgrade suites. Self-reviewed (Notes). ### Background - `HttpResponse::upgrade` destroys the socket's HTTP state, moves the socket into the WebSocket context in place, and sets `upgradedWebSocket` so that `onData` sees the change. - A request head that spans two reads is parsed from the parser's `fallback` buffer, which the upgrade frees. So the parser reports an offset, not a pointer. - `us_dispatch_data` is how the event loop delivers read bytes to a socket. Its first C++ caller needs `extern "C"`. <details><summary>Notes</summary> **Repro** (bun only). Before: `server saw: ["later"] ping events: 0 pongs on the wire: 0`. After: `server saw: ["early","later"] ping events: 1 pongs on the wire: 1`. ```js const net = require("node:net"); const crypto = require("node:crypto"); function frame(opcode, payload) { const p = Buffer.from(payload), mask = crypto.randomBytes(4); const out = Buffer.alloc(6 + p.length); out[0] = 0x80 | opcode; out[1] = 0x80 | p.length; mask.copy(out, 2); for (let i = 0; i < p.length; i++) out[6 + i] = p[i] ^ mask[i % 4]; return out; } const seen = []; let pings = 0; const server = Bun.serve({ port: 0, hostname: "127.0.0.1", fetch(req, server) { if (server.upgrade(req)) return; return new Response("no", { status: 400 }); }, websocket: { message(ws, msg) { seen.push(String(msg)); if (String(msg) === "later") ws.close(1000); }, ping() { pings++; }, }, }); const key = crypto.randomBytes(16).toString("base64"); const upgrade = `GET / HTTP/1.1\r\nHost: 127.0.0.1:${server.port}\r\nUpgrade: websocket\r\nConnection: Upgrade\r\nSec-WebSocket-Key: ${key}\r\nSec-WebSocket-Version: 13\r\n\r\n`; const c = net.connect(server.port, "127.0.0.1", () => { c.write(Buffer.concat([Buffer.from(upgrade), frame(0x1, "early"), frame(0x9, "p")])); }); let got = Buffer.alloc(0), sentLater = false; c.on("data", d => { got = Buffer.concat([got, d]); if (!sentLater && got.includes("\r\n\r\n")) { sentLater = true; c.write(frame(0x1, "later")); } }); c.on("close", () => { const body = got.subarray(got.indexOf("\r\n\r\n") + 4); let pongs = 0; for (let i = 0; i < body.length; ) { if ((body[i] & 0x0f) === 0xa) pongs++; i += 2 + (body[i + 1] & 0x7f); } console.log("server saw:", JSON.stringify(seen), "ping events:", pings, "pongs on the wire:", pongs); server.stop(true); }); ``` **Who sees this.** No user reported it. RFC 6455 section 4.1 tells a client to wait for the 101, and browsers do. A client that writes the request and the first frame back to back sees it only when TCP puts both in one read, so it loses frames some of the time and gets no error. gorilla/websocket rejects such a client on purpose. The `ws` package on Node parses the frames. This PR takes the `ws` behavior, because an open connection that lost data with no signal is the worst of the three. **Where the boundary is.** The fix applies when `server.upgrade()` runs before the dispatch of the request returns to the parser. That includes an `async` handler whose awaits need no new turn of the event loop (`await Promise.resolve()`, `await req.text()` on a GET). Two tests cover that. After a timer or I/O, the read is over. The parser has then read the frame bytes as a pipelined request, `getHeaders` fails, and uWS writes `HTTP/1.1 400 Bad Request` with `Connection: close`. `server.upgrade()` then returns `false`. A test pins this. One exception is left as it is: early bytes that can still begin a request line (for example the single byte `0x41`) wait in the parser's buffer, and the upgrade frees that buffer. **`ws` shim.** #43149 has the full table. A `handleUpgrade()` inside the 'upgrade' event takes the fixed path. A `handleUpgrade()` in a later task, a `verifyClient` that answers later, and frames that arrive in a read of their own before a deferred `handleUpgrade()` still lose the frames: they are in `head` or in the socket's stream, and `src/js/thirdparty/ws.js` has no way to give bytes to the native WebSocket. That needs a design decision, so it has three `it.todo` tests and the issue. The new test `handles data passed along with the upgrade request` is a port of the test of the same name in websockets/ws `test/websocket-server.test.js`. **The check on the socket.** `upgradedWebSocket` is one field per HTTP context. `HttpResponse::upgrade` sets it when any socket of the context is in `onData`. Two ways lead to a value that belongs to another connection. (1) A handler of connection A resolves a promise of connection B, and B's `server.upgrade()` runs in the microtask checkpoint of A's dispatch. The test `never reach the WebSocket of another connection` covers this: with the check removed in a local build, the `message` handler of B received the frame that A sent. (2) A `server.upgrade()` from a request body handler (node:http with a body on the upgrade request, `handleUpgrade()` from `req.on("end")`) leaves the field set. I instrumented a build: the next request on another connection saw the stale field in its request handler (`consumed=32 length=44`). #43163 tracks that bug. Both happen on main today, and this PR does not change what main does there. #37463 fixes (1) at its source. The two PRs are independent and work in either order. With #37463, a connection whose synchronous upgrade is followed by another connection's upgrade in the same dispatch also gets its frames. The check has to stay with #37463 too, because of (2). **`upgrade()` adopts in place.** On linux x64, `sizeof(WebSocketData)` is 160 and `sizeof(HttpResponseData<SSL>)` is 224, and `us_socket_adopt` keeps the block when the new ext is not larger. If a future layout makes the adopt move the socket, the check fails, the frames are dropped as before, and the new tests fail. **Request bodies.** An upgrade request can declare a body. Node reads it as the body of the request. `server.upgrade()` never looked at it. On main, the bytes of such a body in the same read are dropped and the WebSocket works. In a later read they go to the WebSocket parser, which closes the connection. This PR keeps both. When the request declared a body (a `Content-Length` above 0, or chunked), the parser returns the count `HttpParserResult::WHOLE_READ` and `onData` hands nothing over. Every other count is the exact end of the request head, also for a head that fills the parser's 16 KiB buffer for split heads (a test covers that size). The first push of this PR handed the body over too, and that closed connections that main keeps open. Three tests in the new file and one in `ws.test.ts` pin it: they pass on main, fail on the first push, and pass now. The count is a named value and not a new field, because `HttpParserResult` is 16 bytes and comes back in two registers. **Cost on the normal path.** One pointer copy at the top of `consumePostPadded`. The other new code runs only after a handler took the socket. The request handler lambda has no new captures: it lives in a `MoveOnlyFunction` with a 16-byte inline buffer, and a larger closure would allocate on every `onData` call. **Earlier work.** #33692 fixed the same bug in July and was closed as stale, with no judgment on the fix. This PR also covers a request head that spans two reads and a read that starts with the rest of another request's body, adds no lambda captures, and has the check on the socket. **Self-review.** Addressed: the description of the failure (a parser that starts in the middle of a frame, not only a drop), the boundary (a turn of the event loop, not an `await`), the scope of "clear failure" (Bun.serve only), the port of the upstream `ws` test, tests for the microtask upgrade and for the body-tail offset, and the `ws` shim gaps (todo tests and #43149). Rejected: to fold #37463 in and drop the check on the socket. #37463 is open and green on its own, and case (2) above needs the check with or without it. **Reentrancy.** A handler that runs the event loop inside itself after `server.upgrade()` (for example `Bun.build` with an async plugin `setup()`) can let a later read reach the WebSocket before these bytes. HTTP reads have the same property (#42794). **Related open PRs in the same files.** #37463 (`HttpResponse.h`, the `isParsingHttp` lines of `HttpContext.h`), #42789 (the fallback return in `HttpParser.h`, a textual conflict only: `had` stays a local there), #39843 (the uncork lines above the new block), #38128, #39802. **Why a new test file.** `websocket-server.test.ts` does not pass as a whole under a debug ASAN build on my machine: its subprocess-client tests time out on main without this change. A run of that file before and after the fix proves nothing. The directory already has one file for each raw-frame topic (`websocket-server-rsv-frames`, `-unmasked-frames`, `-upgrade-reentrant`). **Suites run with the debug build:** the new file (36 runs, all pass), `test/js/first_party/ws/ws.test.ts`, `test/js/bun/websocket/`, `test/js/bun/http/serve.test.ts`, `request-smuggling.test.ts`, `http-server-chunking.test.ts`, `bun-server.test.ts`, `node-http-with-ws.test.ts`, `node-http-req-socket-pause.test.ts`, `node-http-connect.test.ts`. The subprocess-client tests in `websocket-server.test.ts` time out on my machine with and without this change, and pass when run alone. Two `serve.test.ts` tests fail on my machine for reasons of the machine (it runs as root, and its network blocks the external address). </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 10 failed, 3 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts test/js/first_party/ws/ws.test.ts bun test v1.4.3 (c6b7fcb) test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts: 146 | // One write, so the request and the frames reach the server in one read. 147 | client.socket.write(Buffer.concat([Buffer.from(upgradeRequest), text("early"), ping("p")])); 148 | expect(await client.status()).toBe("HTTP/1.1 101 Switching Protocols"); 149 | client.socket.write(text("later")); 150 | 151 | expect(await client.framesUntil("text:echo:later")).toEqual(["text:echo:early", "pong:p", "text:echo:later"]); ^ error: expect(received).toEqual(expected) [ - "text:echo:early", - "pong:p", "text:echo:later", ] - Expected - 2 + Received + 0 at <anonymous> (/workspace/bun/test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts:151:57) 169 | 170 | client.socket.write(Buffer.concat([Buffer.from(upgradeRequest), text ... (truncated) release without fix: 4 failed, 3 skipped bun test v1.4.3-canary.1 (becf408) test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts: 78 | // Only observed through the races below. 79 | failed.promise.catch(() => {}); 80 | socket.on("error", error => failed.reject(error)); 81 | socket.on("close", () => { 82 | closed.resolve(); 83 | failed.reject(new Error("the server closed the socket")); ^ error: the server closed the socket at <anonymous> (/workspace/bun/test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts:83:25) at emit (node:events:100:22) at <anonymous> (node:net:2350:20) 78 | // Only observed through the races below. 79 | failed.promise.catch(() => {}); 80 | socket.on("error", error => failed.reject(error)); 81 | socket.on("close", () => { 82 | closed.resolve(); 83 | failed.reject(new Error("the server closed the socket")); ^ error: the server closed the socket at <anonymous> (/workspace/bun/test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts:83:25) at emit (node:events:100:22) at <anonymous> (node:net:2350:2 ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: 3 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts test/js/first_party/ws/ws.test.ts bun test v1.4.3 (c6b7fcb) test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts: (pass) frames in the same read as the upgrade request > are delivered in order, and a ping gets its pong (tls: false) [738.45ms] (pass) frames in the same read as the upgrade request > are delivered when server.upgrade() runs after `await Promise.resolve()` [381.05ms] (pass) frames in the same read as the upgrade request > are delivered when server.upgrade() runs after `await req.text()` [380.86ms] (pass) frames in the same read as the upgrade request > a frame that the read cuts short is completed by the next read [397.03ms] (pass) frames in the same read as the upgrade request > are delivered in order, and a ping gets its pong (tls: true) [680.67ms] (pass) frames in the same read as the upgrade request > a Content-Length body in the same read as the upgrade request is not parsed as frames [131.43ms] (pass) frames in the same read as the u ... (truncated) release with fix: 3 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 666ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [0/27] cargo bun_runtime → libbun_runtime.a �[1m�[33mwarning�[0m�[1m: binary `bun_shim_impl` should have a kebab-case name�[0m �[1m�[94m|�[0m �[1m�[94m 1�[0m �[1m�[94m|�[0m /workspace/bun/build/release/rust-target/.../bun_shim_impl �[1m�[94m|�[0m �[1m�[33m^^^^^^^^^^^^^�[0m �[1m�[94m|�[0m �[1m�[94m= �[0m�[1mnote�[0m: `cargo::non_kebab_case_bins` is set to `warn` by default �[1m�[96mhelp�[0m: to change the binary name to `bun-shim-impl`, convert `bin.name` �[1m�[94m--> �[0msrc/install/windows-shim/Cargo.toml:41:8 �[1m�[94m|�[0m �[1m�[94m41�[0m �[91m- �[0mname = �[91m"bun_shim_impl"�[0m �[1m�[94m41�[0m �[92m+ �[0mname = �[92m"bun-shim-impl"�[0m �[1m�[94m|�[0m �[1m�[33mwarning�[0m: `bun_shim_impl` (manifest) generated 1 warning �[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 ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` packages/bun-usockets/src/internal/internal.h | 6 + packages/bun-uws/src/HttpContext.h | 16 +- packages/bun-uws/src/HttpParser.h | 26 +- .../websocket-server-upgrade-early-frames.test.ts | 344 +++++++++++++++++++++ test/js/first_party/ws/ws.test.ts | 133 +++++++- 5 files changed, 518 insertions(+), 7 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests packages/bun-usockets/src/internal/internal.h 1 2 45 packages/bun-uws/src/HttpContext.h 6 5 46 packages/bun-uws/src/HttpParser.h 3 4 45 …websocket/websocket-server-upgrade-early-frames.test.ts 2 5 30 test/js/first_party/ws/ws.test.ts 2 3 31 ``` </details> <!-- robobun:evidence:end -->
This branch has not been deployed
2 participants
Rewritten after the first review: no immediates, the close goes through the drain path.
Problem
wsshim is CLOSED whenclose()returns, and a'close'listener added after it never fires. npm ws is CLOSING and emits'close'later, with aBufferreason. Cause: uws runs the close callback insideServerWebSocket.close()(packages/bun-uws/src/WebSocket.h:270) and#close(ws.js:1056on main) emitted right there.close()with messages still queued fordrainlost them.send()after close dropped its callback,bufferedAmountignoredgetBufferedAmount(), and#drainresent messages uws had accepted.close()issued inside thedrainhandler. The connection only went away through theend()timer, 16 s by default. PlainBun.servereproduces it.Fix
close()records the pending close and runs the drain step: queued messages go out first, the native close follows oncegetBufferedAmount()is 0. The close callback stays in CLOSING, fails the callbacks of undelivered messages, and emits'close'on the next tick with aBufferreason.send()on a closing or closed socket calls back with the "not open" error and counts the bytes.bufferedAmountadds what uws has buffered. A native0is a drop only for a non-empty payload.onWritableshuts the socket down after the drain handler whenend()was called in it, asonDataalready does (WebSocketContext.h:339). Bun runs the handler corked, soend()leaves the FIN to the caller.test/js/first_party/ws/ws.test.ts(block "server socket close lifecycle") andtest/js/bun/websocket/websocket-server-backpressure-buffer.test.ts. All 9 new tests fail on the released build. More in the notes.Background
"ws"always resolves to the shim. Its server socket is a JS state machine over theBun.servewebsocket callbacks installed bynode:http(_http_server.ts:648).send()returns the byte count,-1if uws buffered the message,0if it dropped it (after 16 MiB). The shim queues dropped messages and retries them fromdrain.'close'carries the code passed toclose(), which a compliant peer echoes anyway.Notes
Reference behavior was taken from node v26 with ws 8.18.3 (
test/node_modules/ws) using a raw RFC 6455 client, so both implementations saw identical bytes. The shim now matches on: readyState afterclose()andterminate(), the send and ping callbacks after close,'close'args(4000, Buffer("bye"))for a local close with an echoing peer,(1006, Buffer(0))afterterminate(),(4444, Buffer("other"))for a peer close,bufferedAmount === 4after a latesend("late"), andsend("", cb)calling back with one empty frame on the wire.Known remaining differences: the close code after a local
close()is the local one (native limitation above). The binary frame shape forbinaryType = "arraybuffer"was fixed on main by #39808 (now in the base of this branch), the text frame shape is #36061. The deferred'close'relies on request handlers running with the event loop entered (#39826, in the base of this branch): before that, anextTickqueued insideclose()ran beforeclose()returned when called from the'connection'handler. The test "close() inside the 'connection' handler" pins that case.Why the close waits for
getBufferedAmount() === 0and not only for the shim queue: uwsend()sends the Close frame withsend(), and when the socket is over its backpressure limit that returns DROPPED, whichend()treats as sent and shuts the socket down at once. The released build delivers 2.5 MiB of 26 MiB in the new drain test for that reason. That is a separate uws issue, reported separately. Waiting for an empty buffer also matches when npm ws emits'close'.Probe for the FIN bug with plain
Bun.serve: pause a raw client,ws.send()1 MiB until it returns-1, indraincallws.close()oncegetBufferedAmount()is 0, resume the client. Released build: the client gets the Close frame and no FIN for 4 to 16 s (theend()timer). This branch: FIN right after the Close frame. The new test relies on that timer being 16 s at the default idleTimeout, so it times out on the released build instead of passing late.The backpressure tests are skipped on Windows like the ones already in that file: Winsock's loopback takes the whole payload into the kernel. On Linux and macOS the kernel takes under 8 MiB from a paused reader (the existing 8 MiB test depends on the same), so the queue test sends 26 x 1 MiB to get past the 16 MiB limit. The two blocks take about 1.5 s on a debug ASAN build and were stable over 6 runs.
#draincallbacks moved fromqueueMicrotasktoprocess.nextTick, which is whatsend()uses for the direct path.From the second review round (1607583):
payloadByteLengthsizes a payload that is neither a string nor buffer-like as the text frame of its string form, which is what the nativesend()sends, so a drop of such a message is not mistaken for a delivered empty frame. The close callback subtracts the entries it discards instead of zeroing the counter, so bytes counted by asend()during CLOSING stay inbufferedAmountas in npm ws. The test "terminate() with messages still queued fails their callbacks" pins that and the error callbacks of undelivered messages.Suites run on the debug build:
test/js/first_party/ws/(4 files),node-http-with-ws.test.ts, the ws regression tests (012040,03844,26358,29684,32734,3613),test/js/bun/websocket/(the 8 failures inwebsocket-server.test.tsare the debug-build fixture timeouts that #39370 addresses, the file passes on the released build here too),test/js/web/websocket/(2 failures need the public internet).ws-proxy.test.tsneedsHTTP_PROXYunset in this container, on the released build as well.Overlap with #39843: that PR fixes both uws findings from this one in
end()itself (Close frame past the backpressure limit, FIN sent byend()after uncork), which covers the drain handler case too. TheonWritablehunk here then becomes redundant and can be dropped when #39843 lands first, or #39843 can remove it on rebase. Both can coexist at runtime:shutdown()on an already shut down socket is a no-op in usockets (raw and SSL). The two PRs also both add tests towebsocket-server-backpressure-buffer.test.ts, so whichever lands second needs a small rebase.Rebased onto main after #39642 and #39808 landed in
ws.js. The only conflicts were adjacent additions (thecontrolPayloadhelper next topayloadByteLength/wasDropped, and theEventEmitter/isWindowsimports in the test file), kept both sides.no test proof · iteration 5 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/first_party/ws/ws.test.ts, test/js/bun/websocket/websocket-server-backpressure-buffer.test.ts