Conversation
Connect stored the new connection in ws.conn and started a reader that
re-read that field on every iteration. A second Connect on a live
WebSocket therefore left the first reader running and pointed both
readers at the same *websocket.Conn.
Two readers on one gorilla connection share its buffered reader. Their
frames interleave, the connection fails with errors such as "RSV2 set",
"bad opcode 4" or "continuation after FIN", and the shared buffer can be
corrupted badly enough to panic:
panic: runtime error: slice bounds out of range [:7168] with capacity 4096
bufio.(*Reader).Read(...)
github.com/gorilla/websocket.(*messageReader).Read(...)
github.com/krakenfx/api-go/v2/pkg/kraken.(*WebSocket).read(...)
Reaching that state is easy, because NewWebSocket registers an
OnDisconnected handler that reconnects and never deregisters it: a
caller that adds its own reconnecting handler gets two Connect calls per
drop. With one such handler the readers accumulate on every disconnect,
so a server that accepted 2 connections after the first drop accepted 3,
then 5, then 7.
read now takes its connection as a parameter, so a reader can never
drift onto a connection that replaced the one it was started for, and
Connect refuses to open a second connection while one is live or while
another dial is in flight, returning the new ErrAlreadyConnected. The
built-in Reconnect treats that error as success, since it means another
caller has already done the work.
The connection state is moved under a mutex. conn was written by Connect
while a reader read it, and active was written by an incoming reader
while the reader it replaced cleared it in a defer; both are reported by
the race detector on every reconnect.
Two smaller fixes in the same area:
- Disconnect no longer panics with "send on closed channel". It closed
its done channel in a defer while the handler that sends on it stays
registered until the function returns, so a disconnect arriving after
the one second timeout sent on a closed channel. The channel is now
buffered and the send is non-blocking, which also stops the handler
from blocking the reader goroutine.
- Disconnect and WriteMessage no longer dereference a nil connection
when nothing has been connected.
Adds tests for the reconnect, duplicate-handler, concurrent-Connect and
Disconnect paths. They pass under -race.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Connect stored the new connection in ws.conn and started a reader that re-read that field on every iteration. A second Connect on a live WebSocket therefore left the first reader running and pointed both readers at the same *websocket.Conn.
Two readers on one gorilla connection share its buffered reader. Their frames interleave, the connection fails with errors such as "RSV2 set", "bad opcode 4" or "continuation after FIN", and the shared buffer can be corrupted badly enough to panic:
Reaching that state is easy, because NewWebSocket registers an OnDisconnected handler that reconnects and never deregisters it: a caller that adds its own reconnecting handler gets two Connect calls per drop. With one such handler the readers accumulate on every disconnect, so a server that accepted 2 connections after the first drop accepted 3, then 5, then 7.
read now takes its connection as a parameter, so a reader can never drift onto a connection that replaced the one it was started for, and Connect refuses to open a second connection while one is live or while another dial is in flight, returning the new ErrAlreadyConnected. The built-in Reconnect treats that error as success, since it means another caller has already done the work.
The connection state is moved under a mutex. conn was written by Connect while a reader read it, and active was written by an incoming reader while the reader it replaced cleared it in a defer; both are reported by the race detector on every reconnect.
Two smaller fixes in the same area:
Adds tests for the reconnect, duplicate-handler, concurrent-Connect and Disconnect paths. They pass under -race.