rpc: send clean close frame on websocket disconnect#20788
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to make RPC server–initiated WebSocket disconnects appear as a clean, normal WebSocket closure (1000) to peers, instead of an abnormal closure (1006), by explicitly sending a Close control frame before tearing down the connection.
Changes:
- Send a WebSocket Close frame (
StatusNormalClosure) inwebsocketCodec.Close()prior to shutting down the JSON codec and ping loop.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d6fb707 to
a054ecf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c4925f2 to
d0e6dcb
Compare
|
@Sahil-4555 fix lint plz |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e2cb486 to
e686d6d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I thought there was a helper in the websocket library for this so you don't need to spin up a bunch of timers and goroutines? |
Replace the manual goroutine + timer pattern in wsConnAdapter.Close with a direct call to coder/websocket Conn.Close(StatusNormalClosure, ""), which already performs the close handshake with bounded internal timeouts (5s write, 5s read). Follow-up to #20788. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Good point. I've raise follow-up PR #20913 |
I have done first commit as per it changes you have done in the followup PR. As due to copilot commets i have to do following commit changes as per it to resolve it |
|
Some times copilot commets feels too overcomplex |
) ## Summary Follow-up to erigontech#20788. Address review feedback from @anacrolix: > I thought there was a helper in the websocket library for this so you don't need to spin up a bunch of timers and goroutines? The `coder/websocket` `Conn.Close(code, reason)` method already performs the close handshake with bounded internal timeouts (5s write, 5s read), so the manual goroutine + timer plumbing in `wsConnAdapter.Close` is redundant. Replace it with a direct call. The existing `TestWebsocketServerGracefulClose` test still passes — clients see a clean `StatusNormalClosure` (1000) instead of `StatusAbnormalClosure` (1006). ## Test plan - [x] `go test ./rpc/ -run Websocket` — all pass - [x] `make lint` — clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: info@weblogix.biz <admin@10gbps.weblogix.it>
Currently, when a WebSocket connection is closed by the RPC server, the underlying TCP connection gets torn down immediately without sending a final WebSocket Close frame. Because of this abrupt teardown, clients connected to the node experience an abnormal closure (error code 1006) instead of a clean, normal disconnect (error code 1000).
This PR fixes that behavior by explicitly sending a clean 1000 normal closure frame right before terminating the connection. It uses the
Close()method from thecoder/websocketlibrary to safely transmit this control frame.