Skip to content

Commit

Permalink
Websocket: Prevent panic on Disconnect/Connect
Browse files Browse the repository at this point in the history
Previously we'd set the websocket to disconnected when *either* of the
Conn/AuthConn got a disconnect message.

This meant that:
* The other connection was still functioning
* A user would be free to call Connect()
* wsReadData() may not have exited

Any call to Connect would be acceptable, and we'd probably get a panic
from the underlying shared/re-used gorilla websocket:
`panic: runtime error: slice bounds out of range [:13501] with capacity 8192`
when a new wsReadData goro is started and the old tries to ReadMessage
as well, overallocating the buffer.

This wouldn't normally occur because trafficMonitor would see traffic
(on either connection) and then set the websocket to being connected
again.

The solution is to treat a Disconnect on either websocket as a call to
Shutdown the whole websocket, and then trafficMonitor can reconnect it
properly.

Unit Testing for this has been difficult. So far I've had to rely on a
disruption inside websocket's connectionMonitor itself to reproduce the
panic, but from there it's been very consistent.

Depends on thrasher-corp#1466
  • Loading branch information
gbjk committed Feb 6, 2024
1 parent b65f102 commit 8ddb7cf
Showing 1 changed file with 6 additions and 3 deletions.
9 changes: 6 additions & 3 deletions exchanges/stream/websocket.go
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,11 @@ func (w *Websocket) connectionMonitor() error {
case err := <-w.ReadMessageErrors:
if IsDisconnectionError(err) {
log.Warnf(log.WebsocketMgr, "%v websocket has been disconnected. Reason: %v", w.exchangeName, err)
w.setState(disconnected)
if w.IsConnected() {
if err := w.Shutdown(); err != nil {

Check failure on line 418 in exchanges/stream/websocket.go

View workflow job for this annotation

GitHub Actions / lint

shadow: declaration of "err" shadows declaration at line 414 (govet)
log.Errorf(log.WebsocketMgr, "%v websocket: connectionMonitor shutdown err: %s", w.exchangeName, err)
}
}
}

w.DataHandler <- err
Expand Down Expand Up @@ -582,13 +586,12 @@ func (w *Websocket) trafficMonitor() {
trafficTimer.Stop()
w.setTrafficMonitorRunning(false)
w.Wg.Done() // without this the w.Shutdown() call below will deadlock
if !w.IsConnecting() && w.IsConnected() {
if w.IsConnected() {
err := w.Shutdown()
if err != nil {
log.Errorf(log.WebsocketMgr, "%v websocket: trafficMonitor shutdown err: %s", w.exchangeName, err)
}
}

return
}

Expand Down

0 comments on commit 8ddb7cf

Please sign in to comment.