From 5ec94d6ce05ad3a8d521f37f8b5f1f6ababfe807 Mon Sep 17 00:00:00 2001 From: Levente Liu Date: Tue, 4 Jul 2023 16:23:53 +0800 Subject: [PATCH] Fix: ping option may cause data race and break the connection According to Gorilla's websocket doc, #concurrency section: > 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. --- go.sum | 1 - wsproxy/websocket_proxy.go | 14 ++++++-------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/go.sum b/go.sum index f247863..e3ed4b5 100644 --- a/go.sum +++ b/go.sum @@ -19,7 +19,6 @@ golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20210510120138-977fb7262007 h1:gG67DSER+11cZvqIMb8S8bt0vZtiN6xWYARwirrOSfE= golang.org/x/sys v0.0.0-20210510120138-977fb7262007/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= -golang.org/x/text v0.3.6 h1:aRYxNxv6iGQlyVaZmk6ZgYEDa+Jg18DxebPSrd6bg1M= golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= diff --git a/wsproxy/websocket_proxy.go b/wsproxy/websocket_proxy.go index 7092162..1006e44 100644 --- a/wsproxy/websocket_proxy.go +++ b/wsproxy/websocket_proxy.go @@ -130,9 +130,12 @@ func defaultHeaderForwarder(header string) bool { // The cookie name is specified by the TokenCookieName value. // // example: -// Sec-Websocket-Protocol: Bearer, foobar +// +// Sec-Websocket-Protocol: Bearer, foobar +// // is converted to: -// Authorization: Bearer foobar +// +// Authorization: Bearer foobar // // Method can be overwritten with the MethodOverrideParam get parameter in the requested URL func WebsocketProxy(h http.Handler, opts ...Option) http.Handler { @@ -226,10 +229,6 @@ func (p *Proxy) proxy(w http.ResponseWriter, r *http.Request) { // read loop -- take messages from websocket and write to http request go func() { - if p.pingInterval > 0 && p.pingWait > 0 && p.pongWait > 0 { - conn.SetReadDeadline(time.Now().Add(p.pongWait)) - conn.SetPongHandler(func(string) error { conn.SetReadDeadline(time.Now().Add(p.pongWait)); return nil }) - } defer func() { cancelFn() }() @@ -275,8 +274,7 @@ func (p *Proxy) proxy(w http.ResponseWriter, r *http.Request) { p.logger.Debugln("ping loop done") return case <-ticker.C: - conn.SetWriteDeadline(time.Now().Add(p.pingWait)) - if err := conn.WriteMessage(websocket.PingMessage, nil); err != nil { + if err := conn.WriteControl(websocket.PingMessage, nil, time.Now().Add(p.pingWait)); err != nil { return } }