Repository navigation
Fix WebSocket normal close status handling - #102
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Use RFC 6455 normal closure status 1000 consistently across WebSocket acceptors and connectors. Normalize omitted close codes and add regression coverage for normal, abnormal, and rejected connections.
0fb6fe5 to
bbf412e
Compare
Replace the literal readyState 3 with client.CLOSED in WebSocketServer shutdown, document that server shutdown sends 1001 and waits for each closing handshake, and note the browser-side close code restriction on IWebSocketCommunicator.close(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
samchon
left a comment
There was a problem hiding this comment.
Review
Verdict: the core fix is correct and the regression test is real. Approving after two small follow-ups pushed in fd29137 (details below).
Verified
test_web_close_statuspasses on this branch and fails against the pre-fix source (Client normal close(default) was treated as an error.), so it genuinely covers #99.- Full
pnpm run test:nodepasses; CI (NodeJS + Browser) is green. - The fix commit applies cleanly onto current
masterwithout the toolchain commits.
Core fix
| Change | Assessment |
|---|---|
_Handle_close: code !== 100 → !== NORMAL_CLOSURE |
Obvious typo fix, exactly #99. |
socket.close(code ?? NORMAL_CLOSURE, reason) |
Correct. ws's Sender.close sends an empty frame when code === undefined and silently drops reason; the peer then sees 1005. Normalizing to 1000 unifies Node and browser behavior and makes reason actually reach the peer. |
Connector !event.code || event.code !== 1000 → !== NORMAL_CLOSURE |
Equivalent simplification. |
Impact scope for the record: destructor(error) only uses error to reject in-flight RFC promises; join() is unaffected. So the bug was "pending RPCs during a normal close got WebSocketError(1005) instead of Error("Connection has been closed.")" — narrow, but worth fixing.
Findings
-
PR description was missing the amended
WebSocketServer._Close()change — the0fb6fe5 → bbf412eamend added graceful shutdown with1001,GOING_AWAY, andtest_server_shutdown, none of which were in the body. That is an observable behavior change (terminate()→ clients saw1006; nowclose(1001, …)→ clients see1001). Fixed: PR body updated with a "Behavior changes" section. -
server.close()can now block up to 30 s — the graceful path waits for each client's closing handshake, andwsonly destroys the socket aftercloseTimeout = 30 * 1000. Previouslyterminate()was immediate. This is consistent withacceptor.close()/connector.close(), which already have the same exposure, so I left the behavior as designed and documented it onWebSocketServer.close(). If shutdown latency under zombie connections matters (e.g. k8s grace periods), a bounded grace period followed byterminate()would be a reasonable follow-up. -
reject()defaulting to1000— RFC 6455 defines1000as "purpose fulfilled", which reads oddly for a rejection;1008(Policy Violation) would be more idiomatic. Functionally identical (_Handshake'sonclosethrows for any code), and it is strictly better than the old default (1005,reasonlost), so leaving it. Noted in the body so consumers checkingerror.statusknow. -
Scope — the PR carries three toolchain commits (TS 7 /
ttsx/ pnpm, +5.9k-line lockfile, CI). Unrelated to #99 and the fix is independent of them. Not blocking since CI validates them, but noted in the body. -
Nits (fixed in
fd29137)client.readyState === 3→client.CLOSED.IWebSocketCommunicator.close()JSDoc now notes that browsers only allow1000or3000–4999from the client side (InvalidAccessErrorotherwise). Pre-existing:connector.close(1008)in a browser throws afterstate_ = CLOSINGis already set, leaving the connector stuck — worth a follow-up guard.
-
Follow-up, not in this PR —
WebSocketAcceptor.upgrade()still callssocket.close()without a code on malformed headers (:100,:108), so the peer sees1005. The existing@todocovers it;1003/1007would fit.
🤖 Generated with Claude Code
Problem
WebSocketAcceptorcompared close code100instead of RFC 6455 normal closure code1000. In addition, omitting a close code passedundefinedto Node'swsimplementation, which sent an empty close frame (dropping anyreason), exposed1005to the peer, and made browser and Node behavior diverge.WebSocketServer.close()also terminated every client socket abruptly, so remote connectors observed1006instead of a proper going-away signal.Changes
1000normal closure,1001going away) ininternal/WebSocketCloseCode.ts.1000as a normal close on both acceptors and connectors.1000.1000codes asWebSocketErrorstatuses.WebSocketServer.close()now sends1001("WebSocketServer is going away.") to every client and waits for each closing handshake before shutting the server down.1000/3000–4999) and add regression coverage for both directions, default closes, explicit normal closes, abnormal closes, default rejection, and server shutdown.Behavior changes
acceptor.close()/connector.close()without a code: in-flight RFCs now reject with the genericError("Connection has been closed.")instead ofWebSocketError(1005).acceptor.reject()without a status:connect()throwsWebSocketError(1000, reason)instead ofWebSocketError(1005, ""); thereasonis now delivered.server.close(): connectors receiveWebSocketError(1001, "WebSocketServer is going away.")instead ofWebSocketError(1006). Shutdown waits for the close handshake, so an unresponsive peer can delay it by up tows's 30-second close timeout.Toolchain
This branch also carries the TypeScript 7 /
ttsx/ pnpm migration commits (a910a2e,384afd3,94a00dc). The fix commit itself applies cleanly tomasterwithout them.Closes #99.
Validation
pnpm buildpnpm testpnpm exec tsc --noEmit --project test/tsconfig.jsontest_web_close_statusfails against the pre-fix source and passes with the fix.🤖 Generated with Claude Code