Bun.serve websocket: deliver the Close frame over backpressureLimit and send the FIN right after a close() made inside a handler

Closed
RO
1mo ago
+257-32

Problem

  • ws.close() over backpressureLimit delivers neither the buffered frames nor the Close frame, only a FIN. Same for the echo of a peer's Close frame. end() does bool ok = send(closePayload, CLOSE) (packages/bun-uws/src/WebSocket.h:242), and DROPPED is 2, so it shuts down on the full buffer.
  • close() inside a handler sends the Close frame but no FIN until the end() timer fires, 16 s at the default idleTimeout. end() left the FIN to whoever uncorks the socket. Only onData and the synchronous upgrade path did that, not WebSocket::cork(), which runs the open handler of an asynchronous upgrade, the drain handler and ws.cork().

Fix

  • send() keeps the maxBackpressure check and delegates to a new private sendFrame(), which end() uses for the Close frame. That frame is at most 127 bytes, so the limit protects nothing there. It can only go behind the buffer: the buffer usually starts mid-frame, so a Close frame sent instead would land inside that frame.
  • end() now uncorks (the Close frame is the last frame, nothing is left to batch) and sends the FIN itself when nothing is buffered. Otherwise onWritable (WebSocketContext.h:379) sends it after the drain, as before. The copies of this check in onData and HttpContext.h are dead now and removed.
  • Verified: test/js/bun/websocket/websocket-server-backpressure-buffer.test.ts, block close(). 6 of 8 tests fail on the released build, the other 2 guard the removed checks. Other suites: notes.

Background

  • uws buffers unsent bytes per socket in user space. Above maxBackpressure (backpressureLimit in Bun.serve) send() drops the message. SendStatus is BACKPRESSURE = 0, SUCCESS = 1, DROPPED = 2, so only BACKPRESSURE is false.
  • Corking: while a handler runs, writes go to a cork buffer that uncork() writes out when the handler returns. A synchronous upgrade hands the cork of the 101 response to the new socket, and the HTTP parser uncorks it.
  • shutdown() is shutdown(fd, SHUT_WR). The kernel sends what it holds, then the FIN. uws also drops the writable poll, so its own buffer is never flushed.
Notes

Measured on the released build with a raw client that reads nothing until the server is done, backpressureLimit: 2 MiB, 1 MiB frames until sendBinary() returns 0 (5 accepted on Linux, 3 on Windows). ws.close(1000, "bye"): 2 frames plus 524268 bytes of the third, then FIN. Peer Close frame: all frames, then FIN, no Close frame (end() ran corked inside onData, so the FIN waited, but the frame was dropped). close() in open() after a synchronous upgrade: 2 frames plus part of the third. The three corked cases with an empty buffer (asynchronous upgrade, drain, ws.cork()) deliver the Close frame within 100 ms and the FIN at 16 s. With this change all of them end with the accepted frames, the Close frame (880503e8627965 or 88060fa170656572) and an immediate FIN. A debug build with the headers stashed fails like the released build.

History of this PR: the first revision fixed only the over-limit case, the second added a buffer check to the synchronous upgrade path in HttpContext.h (the old check keyed on uncork() not failing, which is also true for a socket that a large send left uncorked with a full buffer). The self-review pointed out that this was one more copy of the same check while WebSocket::cork() had none, so the third revision moves the FIN into end() and deletes the copies. #39802 adds the same check after the drain handler and waits for getBufferedAmount() === 0 in the ws shim before closing. With this change neither is needed, noted there.

end() uncorking inside a handler: the cork buffer holds this socket's frames from the same handler plus the Close frame, which are written out in one write() either way. Nothing writes to the socket after end(): Bun marks the ServerWebSocket closed before calling it, end() frees the pub/sub subscriber, and onData ignores further input. The outer uncork() of the handler finds no cork and is a no-op. uncork() on a closed socket is also a no-op, and both the raw and the TLS shutdown() check the closed and already shut down states themselves.

The timeout after end() is idleTimeoutComponents.second, 16 s at the default idleTimeout. It already bounded a close under the limit with a non-empty buffer. An over-limit close now gets the same bound instead of an immediate FIN. ws.terminate() remains the immediate path. The three FIN tests rely on that timer being longer than the test timeout, so they do not lower idleTimeout.

Where the partial frame comes from: a frame of 16 KiB or more goes to us_socket_write2() and the unwritten tail is appended to the buffer (WebSocket.h:161-169); smaller frames take AsyncSocket::write(), which does the same (AsyncSocket.h:368). closeOnBackpressureLimit: end() no longer passes the branch in send() that calls us_socket_shutdown_read(). A dropped user send has already done that when it applies, and end() is itself the close. Queued publishes still go through the public send() and are still dropped over the limit. Upstream uWebSockets has the old code.

Windows: the tests run there. The released build fails them the same way on a Windows x64 machine, and the second revision's tests passed on the Windows x64 and aarch64 lanes.

Suites on the debug build after the third revision: 26 websocket files (test/js/bun/websocket/ except the big file, test/js/first_party/ws/ except the proxy file, node-http-with-ws, the close, upgrade, deflate, fault and unix tests in test/js/web/websocket/, regression tests 012040, 26358, 29684, 32734, 3613), 216 pass. websocket-server.test.ts: the same 8 load timeouts next to the send() benchmark as on main (#39370), 80 close, publish, upgrade and stop tests pass with a filter. test/bake/dev/hot.test.ts and deinitialization.test.ts pass. test/cli/inspect/inspect.test.ts fails in this container with the released build and without my changes as well.