From dce061630530c467966378ae3c5adbcf4a09e34f Mon Sep 17 00:00:00 2001 From: Joram Wilander Date: Wed, 17 Jan 2018 08:50:49 -0500 Subject: ABC-73 Move session clean-up to daily task (#8095) * Move session clean-up to daily task * Split delete query into batches --- cmd/platform/server.go | 17 ++++++++++- store/sqlstore/session_store.go | 54 ++++++++++++++++++++++------------- store/store.go | 1 + store/storetest/mocks/SessionStore.go | 5 ++++ store/storetest/session_store.go | 48 ++++++++++++++++++++++++++++++- 5 files changed, 103 insertions(+), 22 deletions(-) diff --git a/cmd/platform/server.go b/cmd/platform/server.go index e36abdaf9..0c23b5f27 100644 --- a/cmd/platform/server.go +++ b/cmd/platform/server.go @@ -21,6 +21,10 @@ import ( "github.com/spf13/cobra" ) +const ( + SESSIONS_CLEANUP_BATCH_SIZE = 1000 +) + var MaxNotificationsPerChannelDefault int64 = 1000000 var serverCmd = &cobra.Command{ @@ -120,7 +124,7 @@ func runServer(configFileLocation string, disableConfigWatch bool) { go runSecurityJob(a) go runDiagnosticsJob(a) - + go runSessionCleanupJob(a) go runTokenCleanupJob(a) go runCommandWebhookCleanupJob(a) @@ -198,6 +202,13 @@ func runCommandWebhookCleanupJob(a *app.App) { }, time.Hour*1) } +func runSessionCleanupJob(a *app.App) { + doSessionCleanup(a) + model.CreateRecurringTask("Session Cleanup", func() { + doSessionCleanup(a) + }, time.Hour*24) +} + func resetStatuses(a *app.App) { if result := <-a.Srv.Store.Status().ResetAll(); result.Err != nil { l4g.Error(utils.T("mattermost.reset_status.error"), result.Err.Error()) @@ -221,3 +232,7 @@ func doTokenCleanup(a *app.App) { func doCommandWebhookCleanup(a *app.App) { a.Srv.Store.CommandWebhook().Cleanup() } + +func doSessionCleanup(a *app.App) { + a.Srv.Store.Session().Cleanup(model.GetMillis(), SESSIONS_CLEANUP_BATCH_SIZE) +} diff --git a/store/sqlstore/session_store.go b/store/sqlstore/session_store.go index 57b7bbffe..221603865 100644 --- a/store/sqlstore/session_store.go +++ b/store/sqlstore/session_store.go @@ -5,12 +5,15 @@ package sqlstore import ( "net/http" + "time" l4g "github.com/alecthomas/log4go" - "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" - "github.com/mattermost/mattermost-server/utils" +) + +const ( + SESSIONS_CLEANUP_DELAY_MILLISECONDS = 100 ) type SqlSessionStore struct { @@ -50,10 +53,6 @@ func (me SqlSessionStore) Save(session *model.Session) store.StoreChannel { session.PreSave() - if cur := <-me.CleanUpExpiredSessions(session.UserId); cur.Err != nil { - l4g.Error(utils.T("store.sql_session.save.cleanup.error"), cur.Err) - } - tcs := me.Team().GetTeamsForUser(session.UserId) if err := me.GetMaster().Insert(session); err != nil { @@ -108,10 +107,6 @@ func (me SqlSessionStore) Get(sessionIdOrToken string) store.StoreChannel { func (me SqlSessionStore) GetSessions(userId string) store.StoreChannel { return store.Do(func(result *store.StoreResult) { - if cur := <-me.CleanUpExpiredSessions(userId); cur.Err != nil { - l4g.Error(utils.T("store.sql_session.get_sessions.error"), cur.Err) - } - var sessions []*model.Session tcs := me.Team().GetTeamsForUser(userId) @@ -180,16 +175,6 @@ func (me SqlSessionStore) PermanentDeleteSessionsByUser(userId string) store.Sto }) } -func (me SqlSessionStore) CleanUpExpiredSessions(userId string) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - if _, err := me.GetMaster().Exec("DELETE FROM Sessions WHERE UserId = :UserId AND ExpiresAt != 0 AND :ExpiresAt > ExpiresAt", map[string]interface{}{"UserId": userId, "ExpiresAt": model.GetMillis()}); err != nil { - result.Err = model.NewAppError("SqlSessionStore.CleanUpExpiredSessions", "store.sql_session.cleanup_expired_sessions.app_error", nil, err.Error(), http.StatusInternalServerError) - } else { - result.Data = userId - } - }) -} - func (me SqlSessionStore) UpdateLastActivityAt(sessionId string, time int64) store.StoreChannel { return store.Do(func(result *store.StoreResult) { if _, err := me.GetMaster().Exec("UPDATE Sessions SET LastActivityAt = :LastActivityAt WHERE Id = :Id", map[string]interface{}{"LastActivityAt": time, "Id": sessionId}); err != nil { @@ -236,3 +221,32 @@ func (me SqlSessionStore) AnalyticsSessionCount() store.StoreChannel { } }) } + +func (me SqlSessionStore) Cleanup(expiryTime int64, batchSize int64) { + l4g.Debug("Cleaning up session store.") + + var query string + if me.DriverName() == model.DATABASE_DRIVER_POSTGRES { + query = "DELETE FROM Sessions WHERE Id = any (array (SELECT Id FROM Sessions WHERE ExpiresAt != 0 AND :ExpiresAt > ExpiresAt LIMIT :Limit))" + } else { + query = "DELETE FROM Sessions WHERE ExpiresAt != 0 AND :ExpiresAt > ExpiresAt LIMIT :Limit" + } + + var rowsAffected int64 = 1 + + for rowsAffected > 0 { + if sqlResult, err := me.GetMaster().Exec(query, map[string]interface{}{"ExpiresAt": expiryTime, "Limit": batchSize}); err != nil { + l4g.Error("Unable to cleanup session store. err=%v", err.Error()) + return + } else { + var rowErr error + rowsAffected, rowErr = sqlResult.RowsAffected() + if rowErr != nil { + l4g.Error("Unable to cleanup session store. err=%v", err.Error()) + return + } + } + + time.Sleep(SESSIONS_CLEANUP_DELAY_MILLISECONDS * time.Millisecond) + } +} diff --git a/store/store.go b/store/store.go index dc140edd4..c66daec7f 100644 --- a/store/store.go +++ b/store/store.go @@ -260,6 +260,7 @@ type SessionStore interface { UpdateRoles(userId string, roles string) StoreChannel UpdateDeviceId(id string, deviceId string, expiresAt int64) StoreChannel AnalyticsSessionCount() StoreChannel + Cleanup(expiryTime int64, batchSize int64) } type AuditStore interface { diff --git a/store/storetest/mocks/SessionStore.go b/store/storetest/mocks/SessionStore.go index 1bec89e11..70b2bd945 100644 --- a/store/storetest/mocks/SessionStore.go +++ b/store/storetest/mocks/SessionStore.go @@ -29,6 +29,11 @@ func (_m *SessionStore) AnalyticsSessionCount() store.StoreChannel { return r0 } +// Cleanup provides a mock function with given fields: expiryTime, batchSize +func (_m *SessionStore) Cleanup(expiryTime int64, batchSize int64) { + _m.Called(expiryTime, batchSize) +} + // Get provides a mock function with given fields: sessionIdOrToken func (_m *SessionStore) Get(sessionIdOrToken string) store.StoreChannel { ret := _m.Called(sessionIdOrToken) diff --git a/store/storetest/session_store.go b/store/storetest/session_store.go index 3d35b91ad..cf1db177d 100644 --- a/store/storetest/session_store.go +++ b/store/storetest/session_store.go @@ -8,9 +8,14 @@ import ( "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" + + "github.com/stretchr/testify/assert" ) func TestSessionStore(t *testing.T, ss store.Store) { + // Run serially to prevent interfering with other tests + testSessionCleanup(t, ss) + t.Run("Save", func(t *testing.T) { testSessionStoreSave(t, ss) }) t.Run("SessionGet", func(t *testing.T) { testSessionGet(t, ss) }) t.Run("SessionGetWithDeviceId", func(t *testing.T) { testSessionGetWithDeviceId(t, ss) }) @@ -58,7 +63,7 @@ func testSessionGet(t *testing.T, ss store.Store) { if rs2 := (<-ss.Session().GetSessions(s1.UserId)); rs2.Err != nil { t.Fatal(rs2.Err) } else { - if len(rs2.Data.([]*model.Session)) != 2 { + if len(rs2.Data.([]*model.Session)) != 3 { t.Fatal("should match len") } } @@ -248,3 +253,44 @@ func testSessionCount(t *testing.T, ss store.Store) { } } } + +func testSessionCleanup(t *testing.T, ss store.Store) { + now := model.GetMillis() + + s1 := model.Session{} + s1.UserId = model.NewId() + s1.ExpiresAt = 0 // never expires + store.Must(ss.Session().Save(&s1)) + + s2 := model.Session{} + s2.UserId = s1.UserId + s2.ExpiresAt = now + 1000000 // expires in the future + store.Must(ss.Session().Save(&s2)) + + s3 := model.Session{} + s3.UserId = model.NewId() + s3.ExpiresAt = 1 // expired + store.Must(ss.Session().Save(&s3)) + + s4 := model.Session{} + s4.UserId = model.NewId() + s4.ExpiresAt = 2 // expired + store.Must(ss.Session().Save(&s4)) + + ss.Session().Cleanup(now, 1) + + err := (<-ss.Session().Get(s1.Id)).Err + assert.Nil(t, err) + + err = (<-ss.Session().Get(s2.Id)).Err + assert.Nil(t, err) + + err = (<-ss.Session().Get(s3.Id)).Err + assert.NotNil(t, err) + + err = (<-ss.Session().Get(s4.Id)).Err + assert.NotNil(t, err) + + store.Must(ss.Session().Remove(s1.Id)) + store.Must(ss.Session().Remove(s2.Id)) +} -- cgit v1.2.3-1-g7c22