tmc / grpc-websocket-proxy

A proxy to transparently upgrade grpc-gateway streaming endpoints to use websockets
MIT License
553 stars 72 forks source link

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

Open leventeliu opened 1 year ago

leventeliu commented 1 year ago

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.