Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: ping option may cause data race and break the connection #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leventeliu
Copy link

According to Gorilla's websocket doc, #concurrency section:

<https://pkg.go.dev/github.com/gorilla/websocket#hdr-Concurrency>

> Connections support one concurrent reader and one concurrent writer.
>
> Applications are responsible for ensuring that no more than one goroutine calls the write methods (NextWriter, SetWriteDeadline, WriteMessage, WriteJSON, EnableWriteCompression, SetCompressionLevel) concurrently and that no more than one goroutine calls the read methods (NextReader, SetReadDeadline, ReadMessage, ReadJSON, SetPongHandler, SetPingHandler) concurrently.
>
> The Close and WriteControl methods can be called concurrently with all other methods.

The ping write loop here runs in another goroutine and use WriteMessage to write Ping is obviously against the pattern. And also SetWriteDeadline/SetReadDeadline will produce side effect for normal text reading/writing.

So I will remove the use of SetWriteDeadline/SetReadDeadline for Ping/Pong message, and use WriteControl to send ping, which is allowed for concurrent call and also has a dependent deadline counter.

According to Gorilla's websocket doc, #concurrency section:

    <https://pkg.go.dev/github.com/gorilla/websocket#hdr-Concurrency>

    > Connections support one concurrent reader and one concurrent writer.
    >
    > Applications are responsible for ensuring that no more than one goroutine calls the write methods (NextWriter, SetWriteDeadline, WriteMessage, WriteJSON, EnableWriteCompression, SetCompressionLevel) concurrently and that no more than one goroutine calls the read methods (NextReader, SetReadDeadline, ReadMessage, ReadJSON, SetPongHandler, SetPingHandler) concurrently.
    >
    > The Close and WriteControl methods can be called concurrently with all other methods.

The `ping write loop` here runs in another goroutine and use WriteMessage
to write Ping is obviously against the pattern. And also
SetWriteDeadline/SetReadDeadline will produce side effect for normal
text reading/writing.

So I will remove the use of SetWriteDeadline/SetReadDeadline for
Ping/Pong message, and use WriteControl to send ping, which is allowed
for concurrent call and also has a dependent deadline counter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant