From f79b7567b136f18b0f59ec9fb9113beebf2c06bd Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Tue, 28 Apr 2020 00:59:48 +0530 Subject: [PATCH] MM-23896: Fix clearing of batched emails on user activity (#14340) On user activity, we were clearing the job.pendingNotifications map. But we had already created a copy of the notifications slice while iterating the map. Therefore, if we pass the copied slice, it would still have the old notifications which were originally deleted. The unit tests would not catch this because it was testing the job.pendingNotifications map and not actually checking if the email handler was being called or not. We fix that now. Co-authored-by: mattermod --- app/email_batching.go | 6 +++--- app/email_batching_test.go | 24 +++++++++++++++++++++++- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/app/email_batching.go b/app/email_batching.go index c3f2a2bd83..56c3889ec5 100644 --- a/app/email_batching.go +++ b/app/email_batching.go @@ -182,13 +182,13 @@ func (job *EmailBatchingJob) checkPendingNotifications(now time.Time, handler fu } } - // send the email notification if it's been long enough - if now.Sub(time.Unix(batchStartTime/1000, 0)) > time.Duration(interval)*time.Second { + // send the email notification if there are notifications to send AND it's been long enough + if len(job.pendingNotifications[userId]) > 0 && now.Sub(time.Unix(batchStartTime/1000, 0)) > time.Duration(interval)*time.Second { job.server.Go(func(userId string, notifications []*batchedNotification) func() { return func() { handler(userId, notifications) } - }(userId, notifications)) + }(userId, job.pendingNotifications[userId])) delete(job.pendingNotifications, userId) } } diff --git a/app/email_batching_test.go b/app/email_batching_test.go index 4f6cdf96d8..a4ca0f9dad 100644 --- a/app/email_batching_test.go +++ b/app/email_batching_test.go @@ -4,6 +4,7 @@ package app import ( + "sync/atomic" "testing" "time" @@ -115,7 +116,28 @@ func TestCheckPendingNotifications(t *testing.T) { _, err = th.App.Srv().Store.Channel().UpdateMember(channelMember) require.Nil(t, err) - job.checkPendingNotifications(time.Unix(10002, 0), func(string, []*batchedNotification) {}) + // We reset the interval to something shorter + err = th.App.Srv().Store.Preference().Save(&model.Preferences{{ + UserId: th.BasicUser.Id, + Category: model.PREFERENCE_CATEGORY_NOTIFICATIONS, + Name: model.PREFERENCE_NAME_EMAIL_INTERVAL, + Value: "10", + }}) + require.Nil(t, err) + + var wasCalled int32 + job.checkPendingNotifications(time.Unix(10050, 0), func(string, []*batchedNotification) { + atomic.StoreInt32(&wasCalled, int32(1)) + }) + + // A hack to check whether the handler was called. + // It's not straightforward to just wait for it using a channel because the test should + // NOT call the handler, and it will be called only if the test fails. + time.Sleep(1 * time.Second) + // We do a check outside the email handler, because otherwise, failing from + // inside the handler doesn't let the .Go() function exit cleanly, and it gets + // stuck during server shutdown, trying to wait for the goroutine to exit + require.Equal(t, atomic.LoadInt32(&wasCalled), int32(0), "email handler should not have been called") require.Nil(t, job.pendingNotifications[th.BasicUser.Id]) require.Empty(t, job.pendingNotifications[th.BasicUser.Id], "should've remove queued post since user acted")