fix: websocket keepalive goroutine leak

[mdawar]

The current WebSocket keepAlive function does not exit on connection close or error.

This pull request fixes the goroutine leak caused by waiting to receive on the ticker channel and not exiting on connection close.

I made the same change to all instances of the keepAlive function as it's being duplicated in multiple packages (Obviously this needs to be refactored).

The keepAlive method in the websocket package required a new done channel that is closed on ReadMessage errors which are considered fatal and will cause a reconnect.

[carlosmiei]

Hello @mdawar,

Thanks for your contribution, we will check it and get back to you!

[Copilot]

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

v2/common/websocket/client.go:385

• Closing c.done directly on an error may lead to a panic if ReadMessage is called concurrently or if multiple errors occur. Consider guarding the close with a sync.Once or check if the channel is already closed.

if err != nil { close(c.done) }

[vigodsky]

Hello @mdawar ,
It seems like keepalive handler will exit after some delay (ticker.C) and check that last pong message outdated.
See example vigodsky/go-binance@master...vigodsky:go-binance:test-keepalive-leak

See keepalive log messages like worker <id>: ...

=== RUN   TestClient
=== RUN   TestClient/TestReadWriteSync
2025/04/14 11:55:37 WebSocket server started on :8080
2025/04/14 11:55:37 worker 982472000: ping
2025/04/14 11:55:37 Received ping: 
=== RUN   TestClient/TestReadWriteSync/WriteSync_success
Binance-golang 2025/04/14 11:55:37 read: waiting for message
2025/04/14 11:55:37 Received message: {"id":"c0992ee8-c588-49a7-a379-9c80b6efcbbe","method":"some-method","params":{}}
Binance-golang 2025/04/14 11:55:37 read: got new message
Binance-golang 2025/04/14 11:55:37 read: sending message into read channel '{c0992ee8-c588-49a7-a379-9c80b6efcbbe}'
Binance-golang 2025/04/14 11:55:37 read: remove message from request list '{c0992ee8-c588-49a7-a379-9c80b6efcbbe}'
Binance-golang 2025/04/14 11:55:37 read: waiting for message
2025/04/14 11:55:39 worker 982472000: ticker chan
Binance-golang 2025/04/14 11:55:39 read: error reading message 'read tcp [::1]:52269->[::1]:8080: use of closed network connection'
Binance-golang 2025/04/14 11:55:39 read: wait to get connected
Binance-golang 2025/04/14 11:55:39 reconnect: received signal
2025/04/14 11:55:39 Error reading message: websocket: close 1006 (abnormal closure): unexpected EOF
2025/04/14 11:55:39 worker 982472000: pong outdated, exit
Binance-golang 2025/04/14 11:55:39 reconnect: connected
Binance-golang 2025/04/14 11:55:39 read: connection established
Binance-golang 2025/04/14 11:55:39 read: waiting for message
2025/04/14 11:55:39 worker 992354000: ping
2025/04/14 11:55:39 Received ping: 
2025/04/14 11:55:41 worker 992354000: ticker chan
Binance-golang 2025/04/14 11:55:41 read: error reading message 'read tcp [::1]:52270->[::1]:8080: use of closed network connection'
Binance-golang 2025/04/14 11:55:41 reconnect: received signal
2025/04/14 11:55:41 worker 992354000: pong outdated, exit
2025/04/14 11:55:41 Error reading message: websocket: close 1006 (abnormal closure): unexpected EOF
Binance-golang 2025/04/14 11:55:41 reconnect: connected
2025/04/14 11:55:41 worker 996330000: ping
2025/04/14 11:55:41 Received ping: 
2025/04/14 11:55:43 worker 996330000: ticker chan
2025/04/14 11:55:43 worker 996330000: pong outdated, exit
2025/04/14 11:55:43 Error reading message: websocket: close 1006 (abnormal closure): unexpected EOF

The same thing you will get if you emulate disconnect by server. KeepAlive handler will exit.

[mdawar]

@vigodsky yes sure I know that these goroutines will eventually exit on the next tick, but it's usually a best practice to exit immediately, I had a lot of errors from the goleak package in my tests after upgrading to the latest version of this package, that's why I tried to fix them with this pull request.
Anyway if the current behavior is acceptable we can dismiss this pull request.

[vigodsky]

@mdawar I agree, but in such case I would recommend to use context. It's safer then closing channel (when you lost control which channel and when was closed)

[mdawar]

@vigodsky You're totally right, closing the channel carries the risk of a panic in the WebSocket client.
In the other cases the doneC channel is already there and is closed on exit, so it's totally fine.
I've made the change to use a context, which can also be used later for other cancellations.
Please feel free to suggest any other changes if you want or even re-close the pull request if it's not acceptable, I don't have any issues with that.