MM-23805: Refactor web_hub (#14277)

* MM-23800: remove goroutineID and stack printing

Each hub has a goroutineID which is calculated with a known hack.
The FAQ clearly explains why goroutines don't have an id:
https://golang.org/doc/faq#no_goroutine_id.

We only added that because sometimes the hub would be deadlocked and
having the goroutineID would be useful when getting the stack trace.
This is also problematic in stress tests because the hubs would
frequently get overloaded and the logs would unnecessarily have stack traces.

But that was in the past, and we have done extensive testing with
load tests and fuzz testing to smooth any rough edges remaining.
Including adding additional metrics for hub buffer size.

Monitoring the metrics is a better way to approach this problem.
Therefore, we remove these kludges from the code.

* Also remove deadlock checking code

There is no need for that anymore since
we are getting rid of the stack printing anyways.

Let's do a wholesale refactor and clean up the codebase.

* MM-23805: Refactor web_hub

This is a beginning of the refactoring of the websocket code.

To start off with, we unexport some methods and constants which did not
need to be exported. There are more remaining but some are out of scope for this PR.

The main chunk of refactor is to unexport the webconn send channel
which was the main cause of panics. Since we were directly sending
to the connection from various parts of the codebase, it would be possible
that the send channel would be closed and we could still send a message.
This would crash the server.

To fix this, we refactor the code to centralize all sending from the main
hub goroutine. This means we can leverage the connections map to check
if the connection exists or not, and only then send the message.

We also move the cluster calls to cluster.go.

* bring back cluster code inside hub

* Incorporate review comments

* Address review comments

* rename index

* MM-23807: Refactor web_conn

- Unexport some struct fields and constants which are not necessary
to be accessed from outside the package. This will help us moving
the entire websocket handling code to a separate package later.

- Change some empty string checks to check for empty string rather
than doing a len check which is more idiomatic. Both of them compile
to the same code. So it doesn't make a difference performance-wise.

- Remove redundant ToJson calls to get the length.

- Incorporate review comments

- Unexport some more methods

* Fix field name

* Run make app-layers

* Add note on hub check

app/web_conn.go

@@ -9,44 +9,49 @@ import (
 	"sync/atomic"
 	"time"
 
-	"github.com/gorilla/websocket"
-	goi18n "github.com/mattermost/go-i18n/i18n"
 	"github.com/mattermost/mattermost-server/v5/mlog"
 	"github.com/mattermost/mattermost-server/v5/model"
+
+	"github.com/gorilla/websocket"
+	goi18n "github.com/mattermost/go-i18n/i18n"
 )
 
 const (
-	SEND_QUEUE_SIZE           = 256
-	SEND_SLOW_WARN            = (SEND_QUEUE_SIZE * 50) / 100
-	SEND_DEADLOCK_WARN        = (SEND_QUEUE_SIZE * 95) / 100
-	WRITE_WAIT                = 30 * time.Second
-	PONG_WAIT                 = 100 * time.Second
-	PING_PERIOD               = (PONG_WAIT * 6) / 10
-	AUTH_TIMEOUT              = 5 * time.Second
-	WEBCONN_MEMBER_CACHE_TIME = 1000 * 60 * 30 // 30 minutes
+	sendQueueSize          = 256
+	sendSlowWarn           = (sendQueueSize * 50) / 100
+	sendFullWarn           = (sendQueueSize * 95) / 100
+	writeWaitTime          = 30 * time.Second
+	pongWaitTime           = 100 * time.Second
+	pingInterval           = (pongWaitTime * 6) / 10
+	authCheckInterval      = 5 * time.Second
+	webConnMemberCacheTime = 1000 * 60 * 30 // 30 minutes
 )
 
+// WebConn represents a single websocket connection to a user.
+// It contains all the necesarry state to manage sending/receiving data to/from
+// a websocket.
 type WebConn struct {
-	sessionExpiresAt          int64 // This should stay at the top for 64-bit alignment of 64-bit words accessed atomically
-	App                       *App
-	WebSocket                 *websocket.Conn
-	Send                      chan model.WebSocketMessage
+	sessionExpiresAt int64 // This should stay at the top for 64-bit alignment of 64-bit words accessed atomically
+	App              *App
+	WebSocket        *websocket.Conn
+	T                goi18n.TranslateFunc
+	Locale           string
+	Sequence         int64
+	UserId           string
+
+	allChannelMembers         map[string]string
+	lastAllChannelMembersTime int64
+	lastUserActivityAt        int64
+	send                      chan model.WebSocketMessage
 	sessionToken              atomic.Value
 	session                   atomic.Value
-	LastUserActivityAt        int64
-	UserId                    string
-	T                         goi18n.TranslateFunc
-	Locale                    string
-	AllChannelMembers         map[string]string
-	LastAllChannelMembersTime int64
-	Sequence                  int64
-	closeOnce                 sync.Once
 	endWritePump              chan struct{}
 	pumpFinished              chan struct{}
 }
 
+// NewWebConn returns a new WebConn instance.
 func (a *App) NewWebConn(ws *websocket.Conn, session model.Session, t goi18n.TranslateFunc, locale string) *WebConn {
-	if len(session.UserId) > 0 {
+	if session.UserId != "" {
 		a.Srv().Go(func() {
 			a.SetStatusOnline(session.UserId, false)
 			a.UpdateLastActivityAtIfNeeded(session)
@@ -55,9 +60,9 @@ func (a *App) NewWebConn(ws *websocket.Conn, session model.Session, t goi18n.Tra
 
 	wc := &WebConn{
 		App:                a,
-		Send:               make(chan model.WebSocketMessage, SEND_QUEUE_SIZE),
+		send:               make(chan model.WebSocketMessage, sendQueueSize),
 		WebSocket:          ws,
-		LastUserActivityAt: model.GetMillis(),
+		lastUserActivityAt: model.GetMillis(),
 		UserId:             session.UserId,
 		T:                  t,
 		Locale:             locale,
@@ -72,34 +77,38 @@ func (a *App) NewWebConn(ws *websocket.Conn, session model.Session, t goi18n.Tra
 	return wc
 }
 
+// Close closes the WebConn.
 func (wc *WebConn) Close() {
 	wc.WebSocket.Close()
-	wc.closeOnce.Do(func() {
-		close(wc.endWritePump)
-	})
 	<-wc.pumpFinished
 }
 
+// GetSessionExpiresAt returns the time at which the session expires.
 func (wc *WebConn) GetSessionExpiresAt() int64 {
 	return atomic.LoadInt64(&wc.sessionExpiresAt)
 }
 
+// SetSessionExpiresAt sets the time at which the session expires.
 func (wc *WebConn) SetSessionExpiresAt(v int64) {
 	atomic.StoreInt64(&wc.sessionExpiresAt, v)
 }
 
+// GetSessionToken returns the session token of the connection.
 func (wc *WebConn) GetSessionToken() string {
 	return wc.sessionToken.Load().(string)
 }
 
+// SetSessionToken sets the session token of the connection.
 func (wc *WebConn) SetSessionToken(v string) {
 	wc.sessionToken.Store(v)
 }
 
+// GetSession returns the session of the connection.
 func (wc *WebConn) GetSession() *model.Session {
 	return wc.session.Load().(*model.Session)
 }
 
+// SetSession sets the session of the connection.
 func (wc *WebConn) SetSession(v *model.Session) {
 	if v != nil {
 		v = v.DeepCopy()
@@ -108,17 +117,18 @@ func (wc *WebConn) SetSession(v *model.Session) {
 	wc.session.Store(v)
 }
 
+// Pump starts the WebConn instance. After this, the websocket
+// is ready to send/receive messages.
 func (wc *WebConn) Pump() {
-	ch := make(chan struct{})
+	var wg sync.WaitGroup
+	wg.Add(1)
 	go func() {
+		defer wg.Done()
 		wc.writePump()
-		close(ch)
 	}()
 	wc.readPump()
-	wc.closeOnce.Do(func() {
-		close(wc.endWritePump)
-	})
-	<-ch
+	close(wc.endWritePump)
+	wg.Wait()
 	wc.App.HubUnregister(wc)
 	close(wc.pumpFinished)
 }
@@ -128,9 +138,9 @@ func (wc *WebConn) readPump() {
 		wc.WebSocket.Close()
 	}()
 	wc.WebSocket.SetReadLimit(model.SOCKET_MAX_MESSAGE_SIZE_KB)
-	wc.WebSocket.SetReadDeadline(time.Now().Add(PONG_WAIT))
+	wc.WebSocket.SetReadDeadline(time.Now().Add(pongWaitTime))
 	wc.WebSocket.SetPongHandler(func(string) error {
-		wc.WebSocket.SetReadDeadline(time.Now().Add(PONG_WAIT))
+		wc.WebSocket.SetReadDeadline(time.Now().Add(pongWaitTime))
 		if wc.IsAuthenticated() {
 			wc.App.Srv().Go(func() {
 				wc.App.SetStatusAwayIfNeeded(wc.UserId, false)
@@ -142,12 +152,7 @@ func (wc *WebConn) readPump() {
 	for {
 		var req model.WebSocketRequest
 		if err := wc.WebSocket.ReadJSON(&req); err != nil {
-			// browsers will appear as CloseNoStatusReceived
-			if websocket.IsCloseError(err, websocket.CloseNormalClosure, websocket.CloseNoStatusReceived) {
-				mlog.Debug("websocket.read: client side closed socket", mlog.String("user_id", wc.UserId))
-			} else {
-				mlog.Debug("websocket.read: closing websocket", mlog.String("user_id", wc.UserId), mlog.Err(err))
-			}
+			wc.logSocketErr("websocket.read", err)
 			return
 		}
 		wc.App.Srv().WebSocketRouter.ServeWebSocket(wc, &req)
@@ -155,8 +160,8 @@ func (wc *WebConn) readPump() {
 }
 
 func (wc *WebConn) writePump() {
-	ticker := time.NewTicker(PING_PERIOD)
-	authTicker := time.NewTicker(AUTH_TIMEOUT)
+	ticker := time.NewTicker(pingInterval)
+	authTicker := time.NewTicker(authCheckInterval)
 
 	defer func() {
 		ticker.Stop()
@@ -166,9 +171,9 @@ func (wc *WebConn) writePump() {
 
 	for {
 		select {
-		case msg, ok := <-wc.Send:
+		case msg, ok := <-wc.send:
 			if !ok {
-				wc.WebSocket.SetWriteDeadline(time.Now().Add(WRITE_WAIT))
+				wc.WebSocket.SetWriteDeadline(time.Now().Add(writeWaitTime))
 				wc.WebSocket.WriteMessage(websocket.CloseMessage, []byte{})
 				return
 			}
@@ -176,11 +181,12 @@ func (wc *WebConn) writePump() {
 			evt, evtOk := msg.(*model.WebSocketEvent)
 
 			skipSend := false
-			if len(wc.Send) >= SEND_SLOW_WARN {
+			if len(wc.send) >= sendSlowWarn {
 				// When the pump starts to get slow we'll drop non-critical messages
-				if msg.EventType() == model.WEBSOCKET_EVENT_TYPING ||
-					msg.EventType() == model.WEBSOCKET_EVENT_STATUS_CHANGE ||
-					msg.EventType() == model.WEBSOCKET_EVENT_CHANNEL_VIEWED {
+				switch msg.EventType() {
+				case model.WEBSOCKET_EVENT_TYPING,
+					model.WEBSOCKET_EVENT_STATUS_CHANGE,
+					model.WEBSOCKET_EVENT_CHANNEL_VIEWED:
 					mlog.Info(
 						"websocket.slow: dropping message",
 						mlog.String("user_id", wc.UserId),
@@ -201,33 +207,22 @@ func (wc *WebConn) writePump() {
 					msgBytes = []byte(msg.ToJson())
 				}
 
-				if len(wc.Send) >= SEND_DEADLOCK_WARN {
-					if evtOk {
-						mlog.Warn(
-							"websocket.full",
-							mlog.String("user_id", wc.UserId),
-							mlog.String("type", msg.EventType()),
-							mlog.String("channel_id", evt.GetBroadcast().ChannelId),
-							mlog.Int("size", len(msg.ToJson())),
-						)
-					} else {
-						mlog.Warn(
-							"websocket.full",
-							mlog.String("user_id", wc.UserId),
-							mlog.String("type", msg.EventType()),
-							mlog.Int("size", len(msg.ToJson())),
-						)
+				if len(wc.send) >= sendFullWarn {
+					logData := []mlog.Field{
+						mlog.String("user_id", wc.UserId),
+						mlog.String("type", msg.EventType()),
+						mlog.Int("size", len(msgBytes)),
 					}
+					if evtOk {
+						logData = append(logData, mlog.String("channel_id", evt.GetBroadcast().ChannelId))
+					}
+
+					mlog.Warn("websocket.full", logData...)
 				}
 
-				wc.WebSocket.SetWriteDeadline(time.Now().Add(WRITE_WAIT))
+				wc.WebSocket.SetWriteDeadline(time.Now().Add(writeWaitTime))
 				if err := wc.WebSocket.WriteMessage(websocket.TextMessage, msgBytes); err != nil {
-					// browsers will appear as CloseNoStatusReceived
-					if websocket.IsCloseError(err, websocket.CloseNormalClosure, websocket.CloseNoStatusReceived) {
-						mlog.Debug("websocket.send: client side closed socket", mlog.String("user_id", wc.UserId))
-					} else {
-						mlog.Debug("websocket.send: closing websocket", mlog.String("user_id", wc.UserId), mlog.Err(err))
-					}
+					wc.logSocketErr("websocket.send", err)
 					return
 				}
 
@@ -237,14 +232,9 @@ func (wc *WebConn) writePump() {
 			}
 
 		case <-ticker.C:
-			wc.WebSocket.SetWriteDeadline(time.Now().Add(WRITE_WAIT))
+			wc.WebSocket.SetWriteDeadline(time.Now().Add(writeWaitTime))
 			if err := wc.WebSocket.WriteMessage(websocket.PingMessage, []byte{}); err != nil {
-				// browsers will appear as CloseNoStatusReceived
-				if websocket.IsCloseError(err, websocket.CloseNormalClosure, websocket.CloseNoStatusReceived) {
-					mlog.Debug("websocket.ticker: client side closed socket", mlog.String("user_id", wc.UserId))
-				} else {
-					mlog.Debug("websocket.ticker: closing websocket", mlog.String("user_id", wc.UserId), mlog.Err(err))
-				}
+				wc.logSocketErr("websocket.ticker", err)
 				return
 			}
 
@@ -261,13 +251,15 @@ func (wc *WebConn) writePump() {
 	}
 }
 
+// InvalidateCache resets all internal data of the WebConn.
 func (wc *WebConn) InvalidateCache() {
-	wc.AllChannelMembers = nil
-	wc.LastAllChannelMembersTime = 0
+	wc.allChannelMembers = nil
+	wc.lastAllChannelMembersTime = 0
 	wc.SetSession(nil)
 	wc.SetSessionExpiresAt(0)
 }
 
+// IsAuthenticated returns whether the given WebConn is authenticated or not.
 func (wc *WebConn) IsAuthenticated() bool {
 	// Check the expiry to see if we need to check for a new session
 	if wc.GetSessionExpiresAt() < model.GetMillis() {
@@ -324,7 +316,8 @@ func (wc *WebConn) shouldSendEventToGuest(msg *model.WebSocketEvent) bool {
 	return canSee
 }
 
-func (wc *WebConn) ShouldSendEvent(msg *model.WebSocketEvent) bool {
+// shouldSendEvent returns whether the message should be sent or not.
+func (wc *WebConn) shouldSendEvent(msg *model.WebSocketEvent) bool {
 	// IMPORTANT: Do not send event if WebConn does not have a session
 	if !wc.IsAuthenticated() {
 		return false
@@ -353,7 +346,7 @@ func (wc *WebConn) ShouldSendEvent(msg *model.WebSocketEvent) bool {
 	}
 
 	// If the event is destined to a specific user
-	if len(msg.GetBroadcast().UserId) > 0 {
+	if msg.GetBroadcast().UserId != "" {
 		return wc.UserId == msg.GetBroadcast().UserId
 	}
 
@@ -365,31 +358,31 @@ func (wc *WebConn) ShouldSendEvent(msg *model.WebSocketEvent) bool {
 	}
 
 	// Only report events to users who are in the channel for the event
-	if len(msg.GetBroadcast().ChannelId) > 0 {
-		if model.GetMillis()-wc.LastAllChannelMembersTime > WEBCONN_MEMBER_CACHE_TIME {
-			wc.AllChannelMembers = nil
-			wc.LastAllChannelMembersTime = 0
+	if msg.GetBroadcast().ChannelId != "" {
+		if model.GetMillis()-wc.lastAllChannelMembersTime > webConnMemberCacheTime {
+			wc.allChannelMembers = nil
+			wc.lastAllChannelMembersTime = 0
 		}
 
-		if wc.AllChannelMembers == nil {
+		if wc.allChannelMembers == nil {
 			result, err := wc.App.Srv().Store.Channel().GetAllChannelMembersForUser(wc.UserId, true, false)
 			if err != nil {
 				mlog.Error("webhub.shouldSendEvent.", mlog.Err(err))
 				return false
 			}
-			wc.AllChannelMembers = result
-			wc.LastAllChannelMembersTime = model.GetMillis()
+			wc.allChannelMembers = result
+			wc.lastAllChannelMembersTime = model.GetMillis()
 		}
 
-		if _, ok := wc.AllChannelMembers[msg.GetBroadcast().ChannelId]; ok {
+		if _, ok := wc.allChannelMembers[msg.GetBroadcast().ChannelId]; ok {
 			return true
 		}
 		return false
 	}
 
 	// Only report events to users who are in the team for the event
-	if len(msg.GetBroadcast().TeamId) > 0 {
-		return wc.IsMemberOfTeam(msg.GetBroadcast().TeamId)
+	if msg.GetBroadcast().TeamId != "" {
+		return wc.isMemberOfTeam(msg.GetBroadcast().TeamId)
 	}
 
 	if wc.GetSession().Props[model.SESSION_PROP_IS_GUEST] == "true" {
@@ -399,10 +392,12 @@ func (wc *WebConn) ShouldSendEvent(msg *model.WebSocketEvent) bool {
 	return true
 }
 
-func (wc *WebConn) IsMemberOfTeam(teamId string) bool {
+// IsMemberOfTeam returns whether the user of the WebConn
+// is a member of the given teamId or not.
+func (wc *WebConn) isMemberOfTeam(teamId string) bool {
 	currentSession := wc.GetSession()
 
-	if currentSession == nil || len(currentSession.Token) == 0 {
+	if currentSession == nil || currentSession.Token == "" {
 		session, err := wc.App.GetSession(wc.GetSessionToken())
 		if err != nil {
 			mlog.Error("Invalid session.", mlog.Err(err))
@@ -414,3 +409,12 @@ func (wc *WebConn) IsMemberOfTeam(teamId string) bool {
 
 	return currentSession.GetTeamByTeamId(teamId) != nil
 }
+
+func (wc *WebConn) logSocketErr(source string, err error) {
+	// browsers will appear as CloseNoStatusReceived
+	if websocket.IsCloseError(err, websocket.CloseNormalClosure, websocket.CloseNoStatusReceived) {
+		mlog.Debug(source+": client side closed socket", mlog.String("user_id", wc.UserId))
+	} else {
+		mlog.Debug(source+": closing websocket", mlog.String("user_id", wc.UserId), mlog.Err(err))
+	}
+}