From 7ce68e89d7effff65e64d8ce711452e431165333 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Mon, 30 Mar 2020 18:12:40 +0530 Subject: [PATCH] MM-22044: Fix panic on web_conn send hello (#13799) * MM-22044: Fix panic on web_conn send hello (*Hub).Start is the central place for sending all web connection related traffic. However, there was this one call to (*WebConn).Hello which tried to send a message to a webconn separately. This was a rare case, but it did occur under stress conditions generated from a load test. When the websocket send SEND_QUEUE_SIZE would get filled up and we would attempt to make a broadcast, the non-blocking send would close the Send channel of the web connection. During that time, if a web connection would try to perform a broadcast, it would try to send to a closed channel and cause a panic. The solution is to bring back the sending of hello into the same goroutine inside (*Hub).Start so that all state is centralised and we avoid sending to a closed channel by sending the hello message inside the registering code itself. * Adding non-blocking send * Simplify things * Remove test * Bring sendHello back * Improve code further Co-authored-by: mattermod --- app/web_conn.go | 4 ++-- app/web_hub.go | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/app/web_conn.go b/app/web_conn.go index c4919a6e9b..99a93267d2 100644 --- a/app/web_conn.go +++ b/app/web_conn.go @@ -291,10 +291,10 @@ func (wc *WebConn) IsAuthenticated() bool { return true } -func (wc *WebConn) SendHello() { +func (wc *WebConn) createHelloMessage() *model.WebSocketEvent { msg := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_HELLO, "", "", wc.UserId, nil) msg.Add("server_version", fmt.Sprintf("%v.%v.%v.%v", model.CurrentVersion, model.BuildNumber, wc.App.ClientConfigHash(), wc.App.License() != nil)) - wc.Send <- msg + return msg } func (wc *WebConn) shouldSendEventToGuest(msg *model.WebSocketEvent) bool { diff --git a/app/web_hub.go b/app/web_hub.go index 53ecc64965..38c5b475f0 100644 --- a/app/web_hub.go +++ b/app/web_hub.go @@ -339,10 +339,6 @@ func (h *Hub) Register(webConn *WebConn) { case h.register <- webConn: case <-h.didStop: } - - if webConn.IsAuthenticated() { - webConn.SendHello() - } } func (h *Hub) Unregister(webConn *WebConn) { @@ -410,6 +406,9 @@ func (h *Hub) Start() { case webCon := <-h.register: connections.Add(webCon) atomic.StoreInt64(&h.connectionCount, int64(len(connections.All()))) + if webCon.IsAuthenticated() { + webCon.Send <- webCon.createHelloMessage() + } case webCon := <-h.unregister: connections.Remove(webCon) atomic.StoreInt64(&h.connectionCount, int64(len(connections.All())))