Merge lp:~nikwen/ubuntu-push/gmail-missed-messages-cleanup-fix-new into lp:ubuntu-push/automatic

Proposed by Niklas Wenzel on 2015-07-15
Status: Merged
Approved by: Samuele Pedroni on 2015-08-13
Approved revision: 416
Merged at revision: 408
Proposed branch: lp:~nikwen/ubuntu-push/gmail-missed-messages-cleanup-fix-new
Merge into: lp:ubuntu-push/automatic
Prerequisite: lp:~nikwen/ubuntu-push/old-cleanup-fix-revert
Diff against target: 431 lines (+136/-104)
5 files modified
client/service/postal.go (+0/-2)
debian/rules (+2/-2)
messaging/cmessaging/cmessaging.go (+0/-1)
messaging/messaging.go (+32/-49)
messaging/messaging_test.go (+102/-50)
To merge this branch: bzr merge lp:~nikwen/ubuntu-push/gmail-missed-messages-cleanup-fix-new
Reviewer Review Type Date Requested Status
Samuele Pedroni 2015-07-15 Approve on 2015-08-13
Review via email: mp+264919@code.launchpad.net

Commit Message

Remove the clean up loop and instead clean up whenever it is needed. This ensures that the notification list is always up to date and fixes an issue where Gmail notifications might be missed due to outdated information about the current state (LP: #1451510). Also includes tests.

Description of the Change

Remove the clean up loop and instead clean up whenever it is needed. This ensures that the notification list is always up to date and fixes an issue where Gmail notifications might be missed due to outdated information about the current state (LP: #1451510). Also includes tests.

Sadly, this MP is still against trunk instead of automatic as there are conflicts with automatic's r403. Therefore, I vote for reverting that revision and then merging the new branch.
It has a few performance/battery life advantages over the old one, even though it uses the same approach.

As the old MP did for some reasons (which I sadly don't know much about as I was not involved) not pass QA testing (afaik it was rejected because "one mail did not show up"), I would like to request that the QA team contacts me directly using my public email from launchpad.net/~nikwen when running into any issues. That way I can actually respond to their feedback!

To post a comment you must log in.
Samuele Pedroni (pedronis) wrote :

looks good,

the idea of the loop was to avoid consuming more and more memory if nobody calls Tags,

review: Needs Information
Niklas Wenzel (nikwen) wrote :

Thank you for your review, Samuele. :)
I'll check if that "memory leak" can still happen and how we can prevent it.

Niklas Wenzel (nikwen) wrote :

Let me still think about it.

Niklas Wenzel (nikwen) wrote :

I just tested it on my device and it works fine.

What I did here is I added a check if we store more than 20 message to the addNotification() method. If we do, we call the cleanUpNotifications() method asynchronously if it is not running already. That way we can get rid of that aweful loop while still preventing too much memory being occupied by messages. After all, anything up to 20 messages should be perfectly fine.

Note that I also added a test for that.

Samuele Pedroni (pedronis) wrote :

I get a go test -race failure in messaging/

I also don't understand the reason for the flag mmu.cleaningUp I think

review: Needs Fixing
Niklas Wenzel (nikwen) wrote :

Yes, you were right. The flag didn't fulfil the purpose I wanted it to fulfil. Thanks!

What I wanted to do is make sure that the cleanup function doesn't get called too often when a user receives a lot of notifications. Hence, I have now adjusted the code to save the last time at which the cleanup function was called and to only call it from the addMessage() function when the last invocation has been at least 10 minutes back.

Samuele Pedroni (pedronis) wrote :
Download full text (6.9 KiB)

I'm still getting -race failures, this is on vivid

$ go version
go version go1.3.3 linux/amd64
$ go test -race
==================
WARNING: DATA RACE
Write by goroutine 10:
  launchpad.net/ubuntu-push/messaging.(*MessagingSuite).TestCleanupInAddNotification()
      /home/pedronis/canonical/go-ws/src/launchpad.net/ubuntu-push/messaging/messaging_test.go:349 +0x4a3
  runtime.call16()
      /usr/lib/go/src/pkg/runtime/asm_amd64.s:360 +0x31
  reflect.Value.Call()
      /usr/lib/go/src/pkg/reflect/value.go:411 +0xed
  launchpad.net/gocheck.func·006()
      /home/pedronis/canonical/go-ws/src/launchpad.net/gocheck/gocheck.go:733 +0x51b
  launchpad.net/gocheck.func·004()
      /home/pedronis/canonical/go-ws/src/launchpad.net/gocheck/gocheck.go:628 +0xf4

Previous read by goroutine 13:
  launchpad.net/ubuntu-push/messaging.(*MessagingMenu).doCleanUpNotifications()
      /home/pedronis/canonical/go-ws/src/launchpad.net/ubuntu-push/messaging/messaging.go:89 +0x125
  launchpad.net/ubuntu-push/messaging.(*MessagingMenu).cleanUpNotifications()
      /home/pedronis/canonical/go-ws/src/launchpad.net/ubuntu-push/messaging/messaging.go:83 +0x77

Goroutine 10 (running) created at:
  launchpad.net/gocheck.(*suiteRunner).forkCall()
      /home/pedronis/canonical/go-ws/src/launchpad.net/gocheck/gocheck.go:629 +0x42e
  launchpad.net/gocheck.(*suiteRunner).forkTest()
      /home/pedronis/canonical/go-ws/src/launchpad.net/gocheck/gocheck.go:765 +0xce
  launchpad.net/gocheck.(*suiteRunner).runTest()
      /home/pedronis/canonical/go-ws/src/launchpad.net/gocheck/gocheck.go:770 +0x3e
  launchpad.net/gocheck.(*suiteRunner).run()
      /home/pedronis/canonical/go-ws/src/launchpad.net/gocheck/gocheck.go:584 +0x3fd
  launchpad.net/gocheck.Run()
      /home/pedronis/canonical/go-ws/src/launchpad.net/gocheck/run.go:76 +0x56
  launchpad.net/gocheck.RunAll()
      /home/pedronis/canonical/go-ws/src/launchpad.net/gocheck/run.go:68 +0x12d
  launchpad.net/gocheck.TestingT()
      /home/pedronis/canonical/go-ws/src/launchpad.net/gocheck/run.go:56 +0x53c
  launchpad.net/ubuntu-push/messaging.Test()
      /home/pedronis/canonical/go-ws/src/launchpad.net/ubuntu-push/messaging/messaging_test.go:36 +0x34
  testing.tRunner()
      /usr/lib/go/src/pkg/testing/testing.go:422 +0x10f

Goroutine 13 (finished) created at:
  launchpad.net/ubuntu-push/messaging.(*MessagingMenu).addNotification()
      /home/pedronis/canonical/go-ws/src/launchpad.net/ubuntu-push/messaging/messaging.go:66 +0x404
  launchpad.net/ubuntu-push/messaging.func·010()
      /home/pedronis/canonical/go-ws/src/launchpad.net/ubuntu-push/messaging/messaging_test.go:318 +0x3ed
  launchpad.net/ubuntu-push/messaging.(*MessagingSuite).TestCleanupInAddNotification()
      /home/pedronis/canonical/go-ws/src/launchpad.net/ubuntu-push/messaging/messaging_test.go:338 +0x36b
  runtime.call16()
      /usr/lib/go/src/pkg/runtime/asm_amd64.s:360 +0x31
  reflect.Value.Call()
      /usr/lib/go/src/pkg/reflect/value.go:411 +0xed
  launchpad.net/gocheck.func·006()
      /home/pedronis/canonical/go-ws/src/launchpad.net/gocheck/gocheck.go:733 +0x51b
  launchpad.net/gocheck.func·004()
      /home/pedronis/canonical/go-ws/src/launchpad.net/gochec...

Read more...

Niklas Wenzel (nikwen) wrote :

Thank you for that information. I'll look into this. :)

Niklas Wenzel (nikwen) wrote :

That was a tough one!

Figuring out what was wrong was pretty easy. It simply didn't like my sleep instructions in the unit test (and neither did I). However, finding a good fix wasn't.

Anyway, here you go. (Ignore my first attempt, the second one is much better.)

Samuele Pedroni (pedronis) wrote :

thanks, much better now

review: Approve
Niklas Wenzel (nikwen) wrote :

Thank you for approving this. :)

Would you mind creating a landing now? The corresponding bug has been scheduled for OTA-6 for which the final freeze will be on Tuesday if I'm not mistaken.

Ubuntu One Auto Pilot (otto-pilot) wrote :
Download full text (14.6 KiB)

The attempt to merge lp:~nikwen/ubuntu-push/gmail-missed-messages-cleanup-fix-new into lp:ubuntu-push/automatic failed. Below is the output from the failed tests.

scripts/deps.sh server/dev/server.go
scripts/deps.sh server/acceptance/cmd/acceptanceclient.go
scripts/deps.sh ubuntu-push-client.go
/mnt/tarmac/cache/ubuntu-push-automatic/go-ws/bin/godeps -t launchpad.net/ubuntu-push launchpad.net/ubuntu-push/accounts launchpad.net/ubuntu-push/bus launchpad.net/ubuntu-push/bus/accounts launchpad.net/ubuntu-push/bus/connectivity launchpad.net/ubuntu-push/bus/emblemcounter launchpad.net/ubuntu-push/bus/haptic launchpad.net/ubuntu-push/bus/networkmanager launchpad.net/ubuntu-push/bus/notifications launchpad.net/ubuntu-push/bus/polld launchpad.net/ubuntu-push/bus/powerd launchpad.net/ubuntu-push/bus/systemimage launchpad.net/ubuntu-push/bus/testing launchpad.net/ubuntu-push/bus/unitygreeter launchpad.net/ubuntu-push/bus/urfkill launchpad.net/ubuntu-push/bus/windowstack launchpad.net/ubuntu-push/click launchpad.net/ubuntu-push/click/cappinfo launchpad.net/ubuntu-push/click/cblacklist launchpad.net/ubuntu-push/click/cclick launchpad.net/ubuntu-push/click/testing launchpad.net/ubuntu-push/client launchpad.net/ubuntu-push/client/gethosts launchpad.net/ubuntu-push/client/service launchpad.net/ubuntu-push/client/session launchpad.net/ubuntu-push/client/session/seenstate launchpad.net/ubuntu-push/config launchpad.net/ubuntu-push/external/murmur3 launchpad.net/ubuntu-push/identifier launchpad.net/ubuntu-push/identifier/testing launchpad.net/ubuntu-push/launch_helper launchpad.net/ubuntu-push/launch_helper/cual launchpad.net/ubuntu-push/launch_helper/helper_finder launchpad.net/ubuntu-push/launch_helper/legacy launchpad.net/ubuntu-push/logger launchpad.net/ubuntu-push/messaging launchpad.net/ubuntu-push/messaging/cmessaging launchpad.net/ubuntu-push/messaging/reply launchpad.net/ubuntu-push/nih launchpad.net/ubuntu-push/nih/cnih launchpad.net/ubuntu-push/poller launchpad.net/ubuntu-push/protocol launchpad.net/ubuntu-push/server launchpad.net/ubuntu-push/server/api launchpad.net/ubuntu-push/server/broker launchpad.net/ubuntu-push/server/broker/simple launchpad.net/ubuntu-push/server/broker/testing launchpad.net/ubuntu-push/server/broker/testsuite launchpad.net/ubuntu-push/server/dev launchpad.net/ubuntu-push/server/listener launchpad.net/ubuntu-push/server/session launchpad.net/ubuntu-push/server/store launchpad.net/ubuntu-push/sounds launchpad.net/ubuntu-push/testing launchpad.net/ubuntu-push/testing/condition launchpad.net/ubuntu-push/urldispatcher launchpad.net/ubuntu-push/urldispatcher/curldispatcher launchpad.net/ubuntu-push/util launchpad.net/ubuntu-push/ launchpad.net/ubuntu-push/server/acceptance/cmd/ launchpad.net/ubuntu-push/server/dev/ 2>/dev/null | cat > dependencies.tsv
rm -f -r /mnt/tarmac/cache/ubuntu-push-automatic/go-ws/pkg
mkdir -p /mnt/tarmac/cache/ubuntu-push-automatic/go-ws/bin
mkdir -p /mnt/tarmac/cache/ubuntu-push-automatic/go-ws/pkg
go get -u launchpad.net/godeps
go get -d -u launchpad.net/gocheck launchpad.net/go-dbus/v1 launchpad.net/go-xdg/v0 code.google.com/p/gosqlite/sqlite3 code.google.com/p/go-uuid/uuid
/mnt/tarmac/cache/ubuntu-p...

Niklas Wenzel (nikwen) wrote :

I'll fix that now.

416. By Niklas Wenzel on 2015-08-13

Run gofmt

Niklas Wenzel (nikwen) wrote :

Fixed. :)

My ubuntu-push and account-polld related work has actually been the first time I coded in go, so please bear with me if things like that happen as I had never heard of gofmt before. :)

If should be fine now though.

Niklas Wenzel (nikwen) wrote :

Finally! Thanks for merging. :)

Would you mind pinging the right person regarding a citrain landing now?

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'client/service/postal.go'
2--- client/service/postal.go 2015-03-05 14:09:54 +0000
3+++ client/service/postal.go 2015-08-13 19:59:59 +0000
4@@ -53,7 +53,6 @@
5 Presenter
6 GetCh() chan *reply.MMActionReply
7 RemoveNotification(string, bool)
8- StartCleanupLoop()
9 Tags(*click.AppId) []string
10 Clear(*click.AppId, ...string) int
11 }
12@@ -187,7 +186,6 @@
13
14 go svc.consumeHelperResults(svc.HelperPool.Start())
15 go svc.handleActions(actionsCh, svc.messagingMenu.GetCh())
16- svc.messagingMenu.StartCleanupLoop()
17 return nil
18 }
19
20
21=== modified file 'debian/rules'
22--- debian/rules 2014-07-19 00:18:18 +0000
23+++ debian/rules 2015-08-13 19:59:59 +0000
24@@ -12,10 +12,10 @@
25 # (should go away once we ship go 1.3)
26 override_dh_auto_test:
27 cd $$( find ./ -type d -regex '\./[^/]*/src/launchpad.net' -printf "%h\n" | head -n1) && \
28- env GOPATH=$$(cd ..; pwd) go test -v $$(env GOPATH=$$(cd ..; pwd) go list $(DH_GOPKG)/... | grep -v acceptance | grep -v http13client )
29+ env GOPATH=$$(cd ..; pwd) go test -race -v $$(env GOPATH=$$(cd ..; pwd) go list $(DH_GOPKG)/... | grep -v acceptance | grep -v http13client )
30
31 override_dh_install:
32 dh_install -Xusr/bin/cmd --fail-missing
33
34 %:
35- dh $@ --buildsystem=golang --with=golang
36+ dh $@ --buildsystem=golang --with=golang --parallel
37
38=== modified file 'messaging/cmessaging/cmessaging.go'
39--- messaging/cmessaging/cmessaging.go 2014-08-07 10:20:35 +0000
40+++ messaging/cmessaging/cmessaging.go 2015-08-13 19:59:59 +0000
41@@ -44,7 +44,6 @@
42 Actions []string
43 App *click.AppId
44 Tag string
45- Gone bool
46 }
47
48 func gchar(s string) *C.gchar {
49
50=== modified file 'messaging/messaging.go'
51--- messaging/messaging.go 2015-07-15 20:22:55 +0000
52+++ messaging/messaging.go 2015-08-13 19:59:59 +0000
53@@ -31,23 +31,19 @@
54 "launchpad.net/ubuntu-push/messaging/reply"
55 )
56
57-var cleanupLoopDuration = 5 * time.Minute
58-
59 type MessagingMenu struct {
60- Log logger.Logger
61- Ch chan *reply.MMActionReply
62- notifications map[string]*cmessaging.Payload // keep a ref to the Payload used in the MMU callback
63- lock sync.RWMutex
64- stopCleanupLoopCh chan bool
65- ticker *time.Ticker
66- tickerCh <-chan time.Time
67+ Log logger.Logger
68+ Ch chan *reply.MMActionReply
69+ notifications map[string]*cmessaging.Payload // keep a ref to the Payload used in the MMU callback
70+ lock sync.RWMutex
71+ lastCleanupTime time.Time
72 }
73
74+type cleanUp func()
75+
76 // New returns a new MessagingMenu
77 func New(log logger.Logger) *MessagingMenu {
78- ticker := time.NewTicker(cleanupLoopDuration)
79- stopCh := make(chan bool)
80- return &MessagingMenu{Log: log, Ch: make(chan *reply.MMActionReply), notifications: make(map[string]*cmessaging.Payload), ticker: ticker, tickerCh: ticker.C, stopCleanupLoopCh: stopCh}
81+ return &MessagingMenu{Log: log, Ch: make(chan *reply.MMActionReply), notifications: make(map[string]*cmessaging.Payload)}
82 }
83
84 var cAddNotification = cmessaging.AddNotification
85@@ -59,12 +55,23 @@
86 return mmu.Ch
87 }
88
89-func (mmu *MessagingMenu) addNotification(app *click.AppId, notificationId string, tag string, card *launch_helper.Card, actions []string) {
90+func (mmu *MessagingMenu) addNotification(app *click.AppId, notificationId string, tag string, card *launch_helper.Card, actions []string, testingCleanUpFunction cleanUp) {
91 mmu.lock.Lock()
92 defer mmu.lock.Unlock()
93 payload := &cmessaging.Payload{Ch: mmu.Ch, Actions: actions, App: app, Tag: tag}
94 mmu.notifications[notificationId] = payload
95 cAddNotification(app.DesktopId(), notificationId, card, payload)
96+
97+ // Clean up our internal notifications store if it holds more than 20 messages (and apparently nobody ever calls Tags())
98+ if len(mmu.notifications) > 20 && time.Since(mmu.lastCleanupTime).Minutes() > 10 {
99+ mmu.lastCleanupTime = time.Now()
100+ if testingCleanUpFunction == nil {
101+ go mmu.cleanUpNotifications()
102+ } else {
103+ testingCleanUpFunction() // Has to implement the asynchronous part itself
104+ }
105+
106+ }
107 }
108
109 func (mmu *MessagingMenu) RemoveNotification(notificationId string, fromUI bool) {
110@@ -77,53 +84,28 @@
111 }
112 }
113
114-// cleanupNotifications remove notifications that were cleared from the messaging menu
115 func (mmu *MessagingMenu) cleanUpNotifications() {
116 mmu.lock.Lock()
117 defer mmu.lock.Unlock()
118+ mmu.doCleanUpNotifications()
119+}
120+
121+// doCleanupNotifications removes notifications that were cleared from the messaging menu
122+func (mmu *MessagingMenu) doCleanUpNotifications() {
123 for nid, payload := range mmu.notifications {
124- if payload.Gone {
125- // sweep
126+ if !cNotificationExists(payload.App.DesktopId(), nid) {
127 delete(mmu.notifications, nid)
128- // don't check the mmu for this nid
129- continue
130- }
131- exists := cNotificationExists(payload.App.DesktopId(), nid)
132- if !exists {
133- // mark
134- payload.Gone = true
135 }
136 }
137 }
138
139-func (mmu *MessagingMenu) StartCleanupLoop() {
140- mmu.doStartCleanupLoop(mmu.cleanUpNotifications)
141-}
142-
143-func (mmu *MessagingMenu) doStartCleanupLoop(cleanupFunc func()) {
144- go func() {
145- for {
146- select {
147- case <-mmu.tickerCh:
148- cleanupFunc()
149- case <-mmu.stopCleanupLoopCh:
150- mmu.ticker.Stop()
151- mmu.Log.Debugf("CleanupLoop stopped.")
152- return
153- }
154- }
155- }()
156-}
157-
158-func (mmu *MessagingMenu) StopCleanupLoop() {
159- mmu.stopCleanupLoopCh <- true
160-}
161-
162 func (mmu *MessagingMenu) Tags(app *click.AppId) []string {
163 orig := app.Original()
164 tags := []string(nil)
165- mmu.lock.RLock()
166- defer mmu.lock.RUnlock()
167+ mmu.lock.Lock()
168+ defer mmu.lock.Unlock()
169+ mmu.lastCleanupTime = time.Now()
170+ mmu.doCleanUpNotifications()
171 for _, payload := range mmu.notifications {
172 if payload.App.Original() == orig {
173 tags = append(tags, payload.Tag)
174@@ -156,6 +138,7 @@
175 for _, nid := range nids {
176 mmu.RemoveNotification(nid, true)
177 }
178+ mmu.cleanUpNotifications()
179
180 return len(nids)
181 }
182@@ -190,7 +173,7 @@
183
184 mmu.Log.Debugf("[%s] creating notification centre entry for %s (summary: %s)", nid, app.Base(), card.Summary)
185
186- mmu.addNotification(app, nid, notification.Tag, card, actions)
187+ mmu.addNotification(app, nid, notification.Tag, card, actions, nil)
188
189 return true
190 }
191
192=== modified file 'messaging/messaging_test.go'
193--- messaging/messaging_test.go 2015-07-15 20:22:55 +0000
194+++ messaging/messaging_test.go 2015-08-13 19:59:59 +0000
195@@ -18,6 +18,8 @@
196
197 import (
198 "sort"
199+ "strconv"
200+ "sync"
201 "time"
202
203 . "launchpad.net/gocheck"
204@@ -48,8 +50,6 @@
205 cRemoveNotification = func(a, n string) {
206 ms.log.Debugf("REMOVE: app: %s, not: %s", a, n)
207 }
208- // just in case
209- cNotificationExists = nil
210 }
211
212 func (ms *MessagingSuite) TearDownSuite(c *C) {
213@@ -61,6 +61,8 @@
214 func (ms *MessagingSuite) SetUpTest(c *C) {
215 ms.log = helpers.NewTestLogger(c, "debug")
216 ms.app = clickhelp.MustParseAppId("com.example.test_test_0")
217+ // just in case
218+ cNotificationExists = nil
219 }
220
221 func (ms *MessagingSuite) TestPresentPresents(c *C) {
222@@ -135,11 +137,23 @@
223 return &launch_helper.Notification{Card: &card, Tag: s}
224 }
225
226+ existsCount := 0
227+ // patch cNotificationExists to return true
228+ cNotificationExists = func(did string, nid string) bool {
229+ existsCount++
230+ return true
231+ }
232+
233 c.Check(mmu.Tags(ms.app), IsNil)
234 c.Assert(mmu.Present(ms.app, "notif1", f("one")), Equals, true)
235 ms.checkTags(c, mmu.Tags(ms.app), []string{"one"})
236+ c.Check(existsCount, Equals, 1)
237+ existsCount = 0
238+
239 c.Assert(mmu.Present(ms.app, "notif2", f("")), Equals, true)
240 ms.checkTags(c, mmu.Tags(ms.app), []string{"one", ""})
241+ c.Check(existsCount, Equals, 2)
242+
243 // and an empty notification doesn't count
244 c.Assert(mmu.Present(ms.app, "notif3", &launch_helper.Notification{Tag: "X"}), Equals, false)
245 ms.checkTags(c, mmu.Tags(ms.app), []string{"one", ""})
246@@ -172,6 +186,12 @@
247 c.Assert(f(app3, "notif6", "one", true), Equals, true)
248 c.Assert(f(app3, "notif7", "", true), Equals, true)
249
250+ // patch cNotificationExists to return true in order to make sure that messages
251+ // do not get deleted by the doCleanUpTags() call in the Tags() function
252+ cNotificationExists = func(did string, nid string) bool {
253+ return true
254+ }
255+
256 // that is:
257 // app 1: "one", "two", "";
258 // app 2: "one", "two";
259@@ -223,7 +243,7 @@
260 mmu := New(ms.log)
261 card := launch_helper.Card{Summary: "ehlo", Persist: true, Actions: []string{"action-1"}}
262 actions := []string{"{\"app\":\"com.example.test_test_0\",\"act\":\"action-1\",\"nid\":\"notif-id\"}", "action-1"}
263- mmu.addNotification(ms.app, "notif-id", "a-tag", &card, actions)
264+ mmu.addNotification(ms.app, "notif-id", "a-tag", &card, actions, nil)
265
266 // check it's there
267 payload, ok := mmu.notifications["notif-id"]
268@@ -242,7 +262,7 @@
269 mmu := New(ms.log)
270 card := launch_helper.Card{Summary: "ehlo", Persist: true, Actions: []string{"action-1"}}
271 actions := []string{"{\"app\":\"com.example.test_test_0\",\"act\":\"action-1\",\"nid\":\"notif-id\"}", "action-1"}
272- mmu.addNotification(ms.app, "notif-id", "a-tag", &card, actions)
273+ mmu.addNotification(ms.app, "notif-id", "a-tag", &card, actions, nil)
274
275 // check it's there
276 _, ok := mmu.notifications["notif-id"]
277@@ -261,13 +281,13 @@
278 mmu := New(ms.log)
279 card := launch_helper.Card{Summary: "ehlo", Persist: true, Actions: []string{"action-1"}}
280 actions := []string{"{\"app\":\"com.example.test_test_0\",\"act\":\"action-1\",\"nid\":\"notif-id\"}", "action-1"}
281- mmu.addNotification(ms.app, "notif-id", "", &card, actions)
282+ mmu.addNotification(ms.app, "notif-id", "", &card, actions, nil)
283
284 // check it's there
285 _, ok := mmu.notifications["notif-id"]
286 c.Check(ok, Equals, true)
287
288- // patch cnotificationexists to return true
289+ // patch cNotificationExists to return true
290 cNotificationExists = func(did string, nid string) bool {
291 return true
292 }
293@@ -276,62 +296,94 @@
294 // check it's still there
295 _, ok = mmu.notifications["notif-id"]
296 c.Check(ok, Equals, true)
297- // patch cnotificationexists to return false
298+
299+ // patch cNotificationExists to return false
300 cNotificationExists = func(did string, nid string) bool {
301 return false
302 }
303- // mark the notification
304- mmu.cleanUpNotifications()
305- // check it's gone
306- _, ok = mmu.notifications["notif-id"]
307- c.Check(ok, Equals, true)
308- // sweep the notification
309+ // remove the notification
310 mmu.cleanUpNotifications()
311 // check it's gone
312 _, ok = mmu.notifications["notif-id"]
313 c.Check(ok, Equals, false)
314 }
315
316-func (ms *MessagingSuite) TestCleanupLoop(c *C) {
317- mmu := New(ms.log)
318- tickerCh := make(chan time.Time)
319- mmu.tickerCh = tickerCh
320- cleanupCh := make(chan bool)
321- cleanupFunc := func() {
322- cleanupCh <- true
323- }
324- // start the cleanup loop
325- mmu.doStartCleanupLoop(cleanupFunc)
326- // mark
327- tickerCh <- time.Now()
328- // check it was called
329- <-cleanupCh
330- // stop the loop and check that it's actually stopped.
331- mmu.StopCleanupLoop()
332- c.Check(ms.log.Captured(), Matches, "(?s).*DEBUG CleanupLoop stopped.*")
333-}
334-
335-func (ms *MessagingSuite) TestStartCleanupLoop(c *C) {
336- mmu := New(ms.log)
337- tickerCh := make(chan time.Time)
338- mmu.tickerCh = tickerCh
339- card := launch_helper.Card{Summary: "ehlo", Persist: true, Actions: []string{"action-1"}}
340- actions := []string{"{\"app\":\"com.example.test_test_0\",\"act\":\"action-1\",\"nid\":\"notif-id\"}", "action-1"}
341- mmu.addNotification(ms.app, "notif-id", "", &card, actions)
342- // patch cnotificationexists to return true and signal when it's called
343- notifExistsCh := make(chan bool)
344+func (ms *MessagingSuite) TestCleanupInAddNotification(c *C) {
345+ mmu := New(ms.log)
346+
347+ var wg sync.WaitGroup
348+
349+ var cleanUpAsynchronously = func() {
350+ wg.Add(1)
351+ go func() {
352+ defer wg.Done()
353+ mmu.cleanUpNotifications()
354+ }()
355+ }
356+
357+ showNotification := func(number int) {
358+ action := "action-" + strconv.Itoa(number)
359+ notificationId := "notif-id-" + strconv.Itoa(number)
360+ card := launch_helper.Card{Summary: "ehlo", Persist: true, Actions: []string{action}}
361+ actions := []string{"{\"app\":\"com.example.test_test_0\",\"act\":\"" + action + "\",\"nid\":\"" + notificationId + "\"}", action}
362+ mmu.addNotification(ms.app, notificationId, "", &card, actions, cleanUpAsynchronously)
363+ }
364+
365+ // Add 20 notifications
366+ for i := 0; i < 20; i++ {
367+ showNotification(i)
368+ }
369+
370+ // wait for the cleanup goroutine in addNotification to finish in case it gets called (which it shouldn't!)
371+ wg.Wait()
372+
373+ // check that we have got 20 notifications
374+ c.Check(mmu.notifications, HasLen, 20)
375+
376+ // patch cNotificationExists to return true
377 cNotificationExists = func(did string, nid string) bool {
378- notifExistsCh <- true
379 return true
380 }
381- // statr the cleanup loop
382- mmu.StartCleanupLoop()
383- // mark
384- tickerCh <- time.Now()
385- // check it's there, and marked
386- <-notifExistsCh
387- // stop the loop
388- mmu.StopCleanupLoop()
389+
390+ // adding another notification should not remove the current ones
391+ showNotification(21)
392+
393+ // wait for the cleanup goroutine in addNotification to finish in case it gets called (which it shouldn't!)
394+ wg.Wait()
395+
396+ // check we that have 21 notifications now
397+ c.Check(mmu.notifications, HasLen, 21)
398+
399+ // patch cNotificationExists to return false for all but the next one we are going to add
400+ cNotificationExists = func(did string, nid string) bool {
401+ return nid == "notif-id-22"
402+ }
403+
404+ // adding another notification should not remove the current ones as mmu.lastCleanupTime is too recent
405+ showNotification(22)
406+
407+ // wait for the cleanup goroutine in addNotification to finish in case it gets called (which it shouldn't!)
408+ wg.Wait()
409+
410+ // check we that have got 22 notifications now
411+ c.Check(mmu.notifications, HasLen, 22)
412+
413+ // set back the lastCleanupTime to 11 minutes ago
414+ mmu.lastCleanupTime = mmu.lastCleanupTime.Add(-11 * time.Minute)
415+
416+ // patch cNotificationExists to return false for all but the next one we are going to add
417+ cNotificationExists = func(did string, nid string) bool {
418+ return nid == "notif-id-23"
419+ }
420+
421+ // adding another notification should remove all previous ones now
422+ showNotification(23)
423+
424+ // wait for the cleanup goroutine in addNotification to finish
425+ wg.Wait()
426+
427+ // check that all notifications except the last one have been removed
428+ c.Check(mmu.notifications, HasLen, 1)
429 }
430
431 func (ms *MessagingSuite) TestGetCh(c *C) {

Subscribers

People subscribed via source and target branches