diff --git a/server/channels/app/platform/session.go b/server/channels/app/platform/session.go index 0c4d46b244..889d26c9f4 100644 --- a/server/channels/app/platform/session.go +++ b/server/channels/app/platform/session.go @@ -17,6 +17,11 @@ import ( func (ps *PlatformService) ReturnSessionToPool(session *model.Session) { if session != nil { session.Id = "" + // Once the session is retrieved from the pool, all existing prop fields are cleared. + // To avoid a race between clearing the props and accessing it, clear the props maps before returning it to the pool. + clear(session.Props) + // Also clear the team members slice to avoid a similar race condition. + clear(session.TeamMembers) ps.sessionPool.Put(session) } } diff --git a/server/channels/app/platform/web_conn.go b/server/channels/app/platform/web_conn.go index 9c2bd3e149..caa27dd663 100644 --- a/server/channels/app/platform/web_conn.go +++ b/server/channels/app/platform/web_conn.go @@ -194,15 +194,6 @@ func (ps *PlatformService) PopulateWebConnConfig(s *model.Session, cfg *WebConnC // NewWebConn returns a new WebConn instance. func (ps *PlatformService) NewWebConn(cfg *WebConnConfig, suite SuiteIFace, runner HookRunner) *WebConn { - userID := cfg.Session.UserId - session := cfg.Session - if cfg.Session.UserId != "" { - ps.Go(func() { - ps.SetStatusOnline(userID, false) - ps.UpdateLastActivityAtIfNeeded(session) - }) - } - // Disable TCP_NO_DELAY for higher throughput var tcpConn *net.TCPConn switch conn := cfg.WebSocket.UnderlyingConn().(type) { @@ -254,9 +245,20 @@ func (ps *PlatformService) NewWebConn(cfg *WebConnConfig, suite SuiteIFace, runn remoteAddress: cfg.RemoteAddress, xForwardedFor: cfg.XForwardedFor, } - wc.Active.Store(cfg.Active) wc.SetSession(&cfg.Session) + userID := cfg.Session.UserId + if userID != "" { + // UpdateLastActivityAtIfNeeded might block if the Hub is busy. + // Create a goroutine to avoid blocking the creation of the websocket connection. + ps.Go(func() { + ps.SetStatusOnline(userID, false) + ps.UpdateLastActivityAtIfNeeded(*wc.GetSession()) + }) + } + + wc.Active.Store(cfg.Active) + wc.SetSessionToken(cfg.Session.Token) wc.SetSessionExpiresAt(cfg.Session.ExpiresAt) wc.SetConnectionID(cfg.ConnectionID) @@ -388,6 +390,8 @@ func (wc *WebConn) GetSession() *model.Session { // SetSession sets the session of the connection. func (wc *WebConn) SetSession(v *model.Session) { + // Clone the session first as WebConn takes ownership of the object + // and the web.Hub will return it to the [sync.Pool] once the WebConn gets removed. if v != nil { v = v.DeepCopy() } diff --git a/server/channels/app/platform/web_conn_test.go b/server/channels/app/platform/web_conn_test.go index 1943aeed76..fb947e12b3 100644 --- a/server/channels/app/platform/web_conn_test.go +++ b/server/channels/app/platform/web_conn_test.go @@ -10,6 +10,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/gorilla/websocket" "github.com/stretchr/testify/assert" @@ -17,6 +18,7 @@ import ( "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/plugin" + "github.com/mattermost/mattermost/server/public/shared/i18n" ) type hookRunner struct { @@ -241,3 +243,38 @@ func TestWebConnDrainDeadQueue(t *testing.T) { t.Run("Overwritten First", func(t *testing.T) { run(int64(128), deadQueueSize+10) }) }) } + +// TestWebConnSessionRace guards against https://mattermost.atlassian.net/browse/MM-60307. It need to be run with the -race flag. +func TestWebConnSessionRace(t *testing.T) { + th := Setup(t).InitBasic() + t.Cleanup(th.TearDown) + + s := httptest.NewServer(dummyWebsocketHandler(t)) + t.Cleanup(s.Close) + d := websocket.Dialer{} + c, _, err := d.Dial("ws://"+s.Listener.Addr().String()+"/ws", nil) + require.NoError(t, err) + + err = th.Service.Start(nil) + require.NoError(t, err) + + session, err := th.Service.CreateSession(th.Context, &model.Session{ + UserId: th.BasicUser.Id, + }) + require.NoError(t, err) + // Ensure LastActivityAt needs to get updated in the session store + session.LastActivityAt = session.LastActivityAt - model.SessionActivityTimeout - 1 + + cfg := &WebConnConfig{ + WebSocket: c, + Session: *session, + TFunc: i18n.IdentityTfunc(), + Locale: "en", + } + _ = th.Service.NewWebConn(cfg, th.Suite, &hookRunner{}) + + session.AddProp(model.SessionPropPlatform, "chrome") + + // Wait a bit of the race checker to catch any + time.Sleep(100 * time.Millisecond) +}