From d9713792ba4f38d3f7a23502da701a691668fbca Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Wed, 22 Apr 2020 00:20:44 +0530 Subject: [PATCH] MM-23800: remove goroutineID and stack printing (#14268) * 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. Co-authored-by: mattermod --- app/server.go | 5 ++-- app/web_hub.go | 64 +------------------------------------------------- 2 files changed, 3 insertions(+), 66 deletions(-) diff --git a/app/server.go b/app/server.go index 0e2cd9c44a..385795133c 100644 --- a/app/server.go +++ b/app/server.go @@ -74,9 +74,8 @@ type Server struct { EmailBatching *EmailBatchingJob EmailRateLimiter *throttled.GCRARateLimiter - hubsLock sync.RWMutex - hubs []*Hub - HubsStopCheckingForDeadlock chan bool + hubsLock sync.RWMutex + hubs []*Hub PushNotificationsHub PushNotificationsHub diff --git a/app/web_hub.go b/app/web_hub.go index 38c5b475f0..f7623b2ebe 100644 --- a/app/web_hub.go +++ b/app/web_hub.go @@ -4,14 +4,11 @@ package app import ( - "fmt" "hash/fnv" "runtime" "runtime/debug" "strconv" - "strings" "sync/atomic" - "time" "github.com/mattermost/mattermost-server/v5/mlog" "github.com/mattermost/mattermost-server/v5/model" @@ -19,8 +16,6 @@ import ( const ( BROADCAST_QUEUE_SIZE = 4096 - DEADLOCK_TICKER = 15 * time.Second // check every 15 seconds - DEADLOCK_WARN = (BROADCAST_QUEUE_SIZE * 99) / 100 // number of buffered messages before printing stack trace ) type WebConnActivityMessage struct { @@ -43,7 +38,6 @@ type Hub struct { invalidateUser chan string activity chan *WebConnActivityMessage ExplicitStop bool - goroutineId int } func (a *App) NewWebHub() *Hub { @@ -70,7 +64,6 @@ func (a *App) HubStart() { mlog.Info("Starting websocket hubs", mlog.Int("number_of_hubs", numberOfHubs)) a.Srv().SetHubs(make([]*Hub, numberOfHubs)) - a.Srv().HubsStopCheckingForDeadlock = make(chan bool, 1) for i := 0; i < len(a.Srv().GetHubs()); i++ { newHub := a.NewWebHub() @@ -82,54 +75,11 @@ func (a *App) HubStart() { } newHub.Start() } - - go func() { - ticker := time.NewTicker(DEADLOCK_TICKER) - - defer func() { - ticker.Stop() - }() - - for { - select { - case <-ticker.C: - for _, hub := range a.Srv().GetHubs() { - if len(hub.broadcast) >= DEADLOCK_WARN { - mlog.Error( - "Websocket hub queue is filled to 99% of its capacity", - mlog.Int("hub", hub.connectionIndex), - mlog.Int("goroutine", hub.goroutineId), - mlog.Int("events", len(hub.broadcast)), - ) - buf := make([]byte, 1<<16) - runtime.Stack(buf, true) - output := fmt.Sprintf("%s", buf) - splits := strings.Split(output, "goroutine ") - - for _, part := range splits { - if strings.Contains(part, fmt.Sprintf("%v", hub.goroutineId)) { - mlog.Error("Trace for possible deadlock goroutine", mlog.String("trace", part)) - } - } - } - } - - case <-a.Srv().HubsStopCheckingForDeadlock: - return - } - } - }() } func (a *App) HubStop() { mlog.Info("stopping websocket hub connections") - select { - case a.Srv().HubsStopCheckingForDeadlock <- true: - default: - mlog.Warn("We appear to have already sent the stop checking for deadlocks command") - } - for _, hub := range a.Srv().GetHubs() { hub.Stop() } @@ -374,17 +324,6 @@ func (h *Hub) UpdateActivity(userId, sessionToken string, activityAt int64) { } } -func getGoroutineId() int { - var buf [64]byte - n := runtime.Stack(buf[:], false) - idField := strings.Fields(strings.TrimPrefix(string(buf[:n]), "goroutine "))[0] - id, err := strconv.Atoi(idField) - if err != nil { - id = -1 - } - return id -} - func (h *Hub) Stop() { close(h.stop) <-h.didStop @@ -396,8 +335,7 @@ func (h *Hub) Start() { var doRecover func() doStart = func() { - h.goroutineId = getGoroutineId() - mlog.Debug("Hub for index is starting with goroutine", mlog.Int("index", h.connectionIndex), mlog.Int("goroutine", h.goroutineId)) + mlog.Debug("Hub is starting", mlog.Int("index", h.connectionIndex)) connections := newHubConnectionIndex()