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
=== modified file 'client/service/postal.go'
--- client/service/postal.go 2015-03-05 14:09:54 +0000
+++ client/service/postal.go 2015-08-13 19:59:59 +0000
@@ -53,7 +53,6 @@
53 Presenter53 Presenter
54 GetCh() chan *reply.MMActionReply54 GetCh() chan *reply.MMActionReply
55 RemoveNotification(string, bool)55 RemoveNotification(string, bool)
56 StartCleanupLoop()
57 Tags(*click.AppId) []string56 Tags(*click.AppId) []string
58 Clear(*click.AppId, ...string) int57 Clear(*click.AppId, ...string) int
59}58}
@@ -187,7 +186,6 @@
187186
188 go svc.consumeHelperResults(svc.HelperPool.Start())187 go svc.consumeHelperResults(svc.HelperPool.Start())
189 go svc.handleActions(actionsCh, svc.messagingMenu.GetCh())188 go svc.handleActions(actionsCh, svc.messagingMenu.GetCh())
190 svc.messagingMenu.StartCleanupLoop()
191 return nil189 return nil
192}190}
193191
194192
=== modified file 'debian/rules'
--- debian/rules 2014-07-19 00:18:18 +0000
+++ debian/rules 2015-08-13 19:59:59 +0000
@@ -12,10 +12,10 @@
12# (should go away once we ship go 1.3)12# (should go away once we ship go 1.3)
13override_dh_auto_test:13override_dh_auto_test:
14 cd $$( find ./ -type d -regex '\./[^/]*/src/launchpad.net' -printf "%h\n" | head -n1) && \14 cd $$( find ./ -type d -regex '\./[^/]*/src/launchpad.net' -printf "%h\n" | head -n1) && \
15 env GOPATH=$$(cd ..; pwd) go test -v $$(env GOPATH=$$(cd ..; pwd) go list $(DH_GOPKG)/... | grep -v acceptance | grep -v http13client )15 env GOPATH=$$(cd ..; pwd) go test -race -v $$(env GOPATH=$$(cd ..; pwd) go list $(DH_GOPKG)/... | grep -v acceptance | grep -v http13client )
1616
17override_dh_install:17override_dh_install:
18 dh_install -Xusr/bin/cmd --fail-missing18 dh_install -Xusr/bin/cmd --fail-missing
1919
20%:20%:
21 dh $@ --buildsystem=golang --with=golang21 dh $@ --buildsystem=golang --with=golang --parallel
2222
=== modified file 'messaging/cmessaging/cmessaging.go'
--- messaging/cmessaging/cmessaging.go 2014-08-07 10:20:35 +0000
+++ messaging/cmessaging/cmessaging.go 2015-08-13 19:59:59 +0000
@@ -44,7 +44,6 @@
44 Actions []string44 Actions []string
45 App *click.AppId45 App *click.AppId
46 Tag string46 Tag string
47 Gone bool
48}47}
4948
50func gchar(s string) *C.gchar {49func gchar(s string) *C.gchar {
5150
=== modified file 'messaging/messaging.go'
--- messaging/messaging.go 2015-07-15 20:22:55 +0000
+++ messaging/messaging.go 2015-08-13 19:59:59 +0000
@@ -31,23 +31,19 @@
31 "launchpad.net/ubuntu-push/messaging/reply"31 "launchpad.net/ubuntu-push/messaging/reply"
32)32)
3333
34var cleanupLoopDuration = 5 * time.Minute
35
36type MessagingMenu struct {34type MessagingMenu struct {
37 Log logger.Logger35 Log logger.Logger
38 Ch chan *reply.MMActionReply36 Ch chan *reply.MMActionReply
39 notifications map[string]*cmessaging.Payload // keep a ref to the Payload used in the MMU callback37 notifications map[string]*cmessaging.Payload // keep a ref to the Payload used in the MMU callback
40 lock sync.RWMutex38 lock sync.RWMutex
41 stopCleanupLoopCh chan bool39 lastCleanupTime time.Time
42 ticker *time.Ticker
43 tickerCh <-chan time.Time
44}40}
4541
42type cleanUp func()
43
46// New returns a new MessagingMenu44// New returns a new MessagingMenu
47func New(log logger.Logger) *MessagingMenu {45func New(log logger.Logger) *MessagingMenu {
48 ticker := time.NewTicker(cleanupLoopDuration)46 return &MessagingMenu{Log: log, Ch: make(chan *reply.MMActionReply), notifications: make(map[string]*cmessaging.Payload)}
49 stopCh := make(chan bool)
50 return &MessagingMenu{Log: log, Ch: make(chan *reply.MMActionReply), notifications: make(map[string]*cmessaging.Payload), ticker: ticker, tickerCh: ticker.C, stopCleanupLoopCh: stopCh}
51}47}
5248
53var cAddNotification = cmessaging.AddNotification49var cAddNotification = cmessaging.AddNotification
@@ -59,12 +55,23 @@
59 return mmu.Ch55 return mmu.Ch
60}56}
6157
62func (mmu *MessagingMenu) addNotification(app *click.AppId, notificationId string, tag string, card *launch_helper.Card, actions []string) {58func (mmu *MessagingMenu) addNotification(app *click.AppId, notificationId string, tag string, card *launch_helper.Card, actions []string, testingCleanUpFunction cleanUp) {
63 mmu.lock.Lock()59 mmu.lock.Lock()
64 defer mmu.lock.Unlock()60 defer mmu.lock.Unlock()
65 payload := &cmessaging.Payload{Ch: mmu.Ch, Actions: actions, App: app, Tag: tag}61 payload := &cmessaging.Payload{Ch: mmu.Ch, Actions: actions, App: app, Tag: tag}
66 mmu.notifications[notificationId] = payload62 mmu.notifications[notificationId] = payload
67 cAddNotification(app.DesktopId(), notificationId, card, payload)63 cAddNotification(app.DesktopId(), notificationId, card, payload)
64
65 // Clean up our internal notifications store if it holds more than 20 messages (and apparently nobody ever calls Tags())
66 if len(mmu.notifications) > 20 && time.Since(mmu.lastCleanupTime).Minutes() > 10 {
67 mmu.lastCleanupTime = time.Now()
68 if testingCleanUpFunction == nil {
69 go mmu.cleanUpNotifications()
70 } else {
71 testingCleanUpFunction() // Has to implement the asynchronous part itself
72 }
73
74 }
68}75}
6976
70func (mmu *MessagingMenu) RemoveNotification(notificationId string, fromUI bool) {77func (mmu *MessagingMenu) RemoveNotification(notificationId string, fromUI bool) {
@@ -77,53 +84,28 @@
77 }84 }
78}85}
7986
80// cleanupNotifications remove notifications that were cleared from the messaging menu
81func (mmu *MessagingMenu) cleanUpNotifications() {87func (mmu *MessagingMenu) cleanUpNotifications() {
82 mmu.lock.Lock()88 mmu.lock.Lock()
83 defer mmu.lock.Unlock()89 defer mmu.lock.Unlock()
90 mmu.doCleanUpNotifications()
91}
92
93// doCleanupNotifications removes notifications that were cleared from the messaging menu
94func (mmu *MessagingMenu) doCleanUpNotifications() {
84 for nid, payload := range mmu.notifications {95 for nid, payload := range mmu.notifications {
85 if payload.Gone {96 if !cNotificationExists(payload.App.DesktopId(), nid) {
86 // sweep
87 delete(mmu.notifications, nid)97 delete(mmu.notifications, nid)
88 // don't check the mmu for this nid
89 continue
90 }
91 exists := cNotificationExists(payload.App.DesktopId(), nid)
92 if !exists {
93 // mark
94 payload.Gone = true
95 }98 }
96 }99 }
97}100}
98101
99func (mmu *MessagingMenu) StartCleanupLoop() {
100 mmu.doStartCleanupLoop(mmu.cleanUpNotifications)
101}
102
103func (mmu *MessagingMenu) doStartCleanupLoop(cleanupFunc func()) {
104 go func() {
105 for {
106 select {
107 case <-mmu.tickerCh:
108 cleanupFunc()
109 case <-mmu.stopCleanupLoopCh:
110 mmu.ticker.Stop()
111 mmu.Log.Debugf("CleanupLoop stopped.")
112 return
113 }
114 }
115 }()
116}
117
118func (mmu *MessagingMenu) StopCleanupLoop() {
119 mmu.stopCleanupLoopCh <- true
120}
121
122func (mmu *MessagingMenu) Tags(app *click.AppId) []string {102func (mmu *MessagingMenu) Tags(app *click.AppId) []string {
123 orig := app.Original()103 orig := app.Original()
124 tags := []string(nil)104 tags := []string(nil)
125 mmu.lock.RLock()105 mmu.lock.Lock()
126 defer mmu.lock.RUnlock()106 defer mmu.lock.Unlock()
107 mmu.lastCleanupTime = time.Now()
108 mmu.doCleanUpNotifications()
127 for _, payload := range mmu.notifications {109 for _, payload := range mmu.notifications {
128 if payload.App.Original() == orig {110 if payload.App.Original() == orig {
129 tags = append(tags, payload.Tag)111 tags = append(tags, payload.Tag)
@@ -156,6 +138,7 @@
156 for _, nid := range nids {138 for _, nid := range nids {
157 mmu.RemoveNotification(nid, true)139 mmu.RemoveNotification(nid, true)
158 }140 }
141 mmu.cleanUpNotifications()
159142
160 return len(nids)143 return len(nids)
161}144}
@@ -190,7 +173,7 @@
190173
191 mmu.Log.Debugf("[%s] creating notification centre entry for %s (summary: %s)", nid, app.Base(), card.Summary)174 mmu.Log.Debugf("[%s] creating notification centre entry for %s (summary: %s)", nid, app.Base(), card.Summary)
192175
193 mmu.addNotification(app, nid, notification.Tag, card, actions)176 mmu.addNotification(app, nid, notification.Tag, card, actions, nil)
194177
195 return true178 return true
196}179}
197180
=== modified file 'messaging/messaging_test.go'
--- messaging/messaging_test.go 2015-07-15 20:22:55 +0000
+++ messaging/messaging_test.go 2015-08-13 19:59:59 +0000
@@ -18,6 +18,8 @@
1818
19import (19import (
20 "sort"20 "sort"
21 "strconv"
22 "sync"
21 "time"23 "time"
2224
23 . "launchpad.net/gocheck"25 . "launchpad.net/gocheck"
@@ -48,8 +50,6 @@
48 cRemoveNotification = func(a, n string) {50 cRemoveNotification = func(a, n string) {
49 ms.log.Debugf("REMOVE: app: %s, not: %s", a, n)51 ms.log.Debugf("REMOVE: app: %s, not: %s", a, n)
50 }52 }
51 // just in case
52 cNotificationExists = nil
53}53}
5454
55func (ms *MessagingSuite) TearDownSuite(c *C) {55func (ms *MessagingSuite) TearDownSuite(c *C) {
@@ -61,6 +61,8 @@
61func (ms *MessagingSuite) SetUpTest(c *C) {61func (ms *MessagingSuite) SetUpTest(c *C) {
62 ms.log = helpers.NewTestLogger(c, "debug")62 ms.log = helpers.NewTestLogger(c, "debug")
63 ms.app = clickhelp.MustParseAppId("com.example.test_test_0")63 ms.app = clickhelp.MustParseAppId("com.example.test_test_0")
64 // just in case
65 cNotificationExists = nil
64}66}
6567
66func (ms *MessagingSuite) TestPresentPresents(c *C) {68func (ms *MessagingSuite) TestPresentPresents(c *C) {
@@ -135,11 +137,23 @@
135 return &launch_helper.Notification{Card: &card, Tag: s}137 return &launch_helper.Notification{Card: &card, Tag: s}
136 }138 }
137139
140 existsCount := 0
141 // patch cNotificationExists to return true
142 cNotificationExists = func(did string, nid string) bool {
143 existsCount++
144 return true
145 }
146
138 c.Check(mmu.Tags(ms.app), IsNil)147 c.Check(mmu.Tags(ms.app), IsNil)
139 c.Assert(mmu.Present(ms.app, "notif1", f("one")), Equals, true)148 c.Assert(mmu.Present(ms.app, "notif1", f("one")), Equals, true)
140 ms.checkTags(c, mmu.Tags(ms.app), []string{"one"})149 ms.checkTags(c, mmu.Tags(ms.app), []string{"one"})
150 c.Check(existsCount, Equals, 1)
151 existsCount = 0
152
141 c.Assert(mmu.Present(ms.app, "notif2", f("")), Equals, true)153 c.Assert(mmu.Present(ms.app, "notif2", f("")), Equals, true)
142 ms.checkTags(c, mmu.Tags(ms.app), []string{"one", ""})154 ms.checkTags(c, mmu.Tags(ms.app), []string{"one", ""})
155 c.Check(existsCount, Equals, 2)
156
143 // and an empty notification doesn't count157 // and an empty notification doesn't count
144 c.Assert(mmu.Present(ms.app, "notif3", &launch_helper.Notification{Tag: "X"}), Equals, false)158 c.Assert(mmu.Present(ms.app, "notif3", &launch_helper.Notification{Tag: "X"}), Equals, false)
145 ms.checkTags(c, mmu.Tags(ms.app), []string{"one", ""})159 ms.checkTags(c, mmu.Tags(ms.app), []string{"one", ""})
@@ -172,6 +186,12 @@
172 c.Assert(f(app3, "notif6", "one", true), Equals, true)186 c.Assert(f(app3, "notif6", "one", true), Equals, true)
173 c.Assert(f(app3, "notif7", "", true), Equals, true)187 c.Assert(f(app3, "notif7", "", true), Equals, true)
174188
189 // patch cNotificationExists to return true in order to make sure that messages
190 // do not get deleted by the doCleanUpTags() call in the Tags() function
191 cNotificationExists = func(did string, nid string) bool {
192 return true
193 }
194
175 // that is:195 // that is:
176 // app 1: "one", "two", "";196 // app 1: "one", "two", "";
177 // app 2: "one", "two";197 // app 2: "one", "two";
@@ -223,7 +243,7 @@
223 mmu := New(ms.log)243 mmu := New(ms.log)
224 card := launch_helper.Card{Summary: "ehlo", Persist: true, Actions: []string{"action-1"}}244 card := launch_helper.Card{Summary: "ehlo", Persist: true, Actions: []string{"action-1"}}
225 actions := []string{"{\"app\":\"com.example.test_test_0\",\"act\":\"action-1\",\"nid\":\"notif-id\"}", "action-1"}245 actions := []string{"{\"app\":\"com.example.test_test_0\",\"act\":\"action-1\",\"nid\":\"notif-id\"}", "action-1"}
226 mmu.addNotification(ms.app, "notif-id", "a-tag", &card, actions)246 mmu.addNotification(ms.app, "notif-id", "a-tag", &card, actions, nil)
227247
228 // check it's there248 // check it's there
229 payload, ok := mmu.notifications["notif-id"]249 payload, ok := mmu.notifications["notif-id"]
@@ -242,7 +262,7 @@
242 mmu := New(ms.log)262 mmu := New(ms.log)
243 card := launch_helper.Card{Summary: "ehlo", Persist: true, Actions: []string{"action-1"}}263 card := launch_helper.Card{Summary: "ehlo", Persist: true, Actions: []string{"action-1"}}
244 actions := []string{"{\"app\":\"com.example.test_test_0\",\"act\":\"action-1\",\"nid\":\"notif-id\"}", "action-1"}264 actions := []string{"{\"app\":\"com.example.test_test_0\",\"act\":\"action-1\",\"nid\":\"notif-id\"}", "action-1"}
245 mmu.addNotification(ms.app, "notif-id", "a-tag", &card, actions)265 mmu.addNotification(ms.app, "notif-id", "a-tag", &card, actions, nil)
246266
247 // check it's there267 // check it's there
248 _, ok := mmu.notifications["notif-id"]268 _, ok := mmu.notifications["notif-id"]
@@ -261,13 +281,13 @@
261 mmu := New(ms.log)281 mmu := New(ms.log)
262 card := launch_helper.Card{Summary: "ehlo", Persist: true, Actions: []string{"action-1"}}282 card := launch_helper.Card{Summary: "ehlo", Persist: true, Actions: []string{"action-1"}}
263 actions := []string{"{\"app\":\"com.example.test_test_0\",\"act\":\"action-1\",\"nid\":\"notif-id\"}", "action-1"}283 actions := []string{"{\"app\":\"com.example.test_test_0\",\"act\":\"action-1\",\"nid\":\"notif-id\"}", "action-1"}
264 mmu.addNotification(ms.app, "notif-id", "", &card, actions)284 mmu.addNotification(ms.app, "notif-id", "", &card, actions, nil)
265285
266 // check it's there286 // check it's there
267 _, ok := mmu.notifications["notif-id"]287 _, ok := mmu.notifications["notif-id"]
268 c.Check(ok, Equals, true)288 c.Check(ok, Equals, true)
269289
270 // patch cnotificationexists to return true290 // patch cNotificationExists to return true
271 cNotificationExists = func(did string, nid string) bool {291 cNotificationExists = func(did string, nid string) bool {
272 return true292 return true
273 }293 }
@@ -276,62 +296,94 @@
276 // check it's still there296 // check it's still there
277 _, ok = mmu.notifications["notif-id"]297 _, ok = mmu.notifications["notif-id"]
278 c.Check(ok, Equals, true)298 c.Check(ok, Equals, true)
279 // patch cnotificationexists to return false299
300 // patch cNotificationExists to return false
280 cNotificationExists = func(did string, nid string) bool {301 cNotificationExists = func(did string, nid string) bool {
281 return false302 return false
282 }303 }
283 // mark the notification304 // remove the notification
284 mmu.cleanUpNotifications()
285 // check it's gone
286 _, ok = mmu.notifications["notif-id"]
287 c.Check(ok, Equals, true)
288 // sweep the notification
289 mmu.cleanUpNotifications()305 mmu.cleanUpNotifications()
290 // check it's gone306 // check it's gone
291 _, ok = mmu.notifications["notif-id"]307 _, ok = mmu.notifications["notif-id"]
292 c.Check(ok, Equals, false)308 c.Check(ok, Equals, false)
293}309}
294310
295func (ms *MessagingSuite) TestCleanupLoop(c *C) {311func (ms *MessagingSuite) TestCleanupInAddNotification(c *C) {
296 mmu := New(ms.log)312 mmu := New(ms.log)
297 tickerCh := make(chan time.Time)313
298 mmu.tickerCh = tickerCh314 var wg sync.WaitGroup
299 cleanupCh := make(chan bool)315
300 cleanupFunc := func() {316 var cleanUpAsynchronously = func() {
301 cleanupCh <- true317 wg.Add(1)
302 }318 go func() {
303 // start the cleanup loop319 defer wg.Done()
304 mmu.doStartCleanupLoop(cleanupFunc)320 mmu.cleanUpNotifications()
305 // mark321 }()
306 tickerCh <- time.Now()322 }
307 // check it was called323
308 <-cleanupCh324 showNotification := func(number int) {
309 // stop the loop and check that it's actually stopped.325 action := "action-" + strconv.Itoa(number)
310 mmu.StopCleanupLoop()326 notificationId := "notif-id-" + strconv.Itoa(number)
311 c.Check(ms.log.Captured(), Matches, "(?s).*DEBUG CleanupLoop stopped.*")327 card := launch_helper.Card{Summary: "ehlo", Persist: true, Actions: []string{action}}
312}328 actions := []string{"{\"app\":\"com.example.test_test_0\",\"act\":\"" + action + "\",\"nid\":\"" + notificationId + "\"}", action}
313329 mmu.addNotification(ms.app, notificationId, "", &card, actions, cleanUpAsynchronously)
314func (ms *MessagingSuite) TestStartCleanupLoop(c *C) {330 }
315 mmu := New(ms.log)331
316 tickerCh := make(chan time.Time)332 // Add 20 notifications
317 mmu.tickerCh = tickerCh333 for i := 0; i < 20; i++ {
318 card := launch_helper.Card{Summary: "ehlo", Persist: true, Actions: []string{"action-1"}}334 showNotification(i)
319 actions := []string{"{\"app\":\"com.example.test_test_0\",\"act\":\"action-1\",\"nid\":\"notif-id\"}", "action-1"}335 }
320 mmu.addNotification(ms.app, "notif-id", "", &card, actions)336
321 // patch cnotificationexists to return true and signal when it's called337 // wait for the cleanup goroutine in addNotification to finish in case it gets called (which it shouldn't!)
322 notifExistsCh := make(chan bool)338 wg.Wait()
339
340 // check that we have got 20 notifications
341 c.Check(mmu.notifications, HasLen, 20)
342
343 // patch cNotificationExists to return true
323 cNotificationExists = func(did string, nid string) bool {344 cNotificationExists = func(did string, nid string) bool {
324 notifExistsCh <- true
325 return true345 return true
326 }346 }
327 // statr the cleanup loop347
328 mmu.StartCleanupLoop()348 // adding another notification should not remove the current ones
329 // mark349 showNotification(21)
330 tickerCh <- time.Now()350
331 // check it's there, and marked351 // wait for the cleanup goroutine in addNotification to finish in case it gets called (which it shouldn't!)
332 <-notifExistsCh352 wg.Wait()
333 // stop the loop353
334 mmu.StopCleanupLoop()354 // check we that have 21 notifications now
355 c.Check(mmu.notifications, HasLen, 21)
356
357 // patch cNotificationExists to return false for all but the next one we are going to add
358 cNotificationExists = func(did string, nid string) bool {
359 return nid == "notif-id-22"
360 }
361
362 // adding another notification should not remove the current ones as mmu.lastCleanupTime is too recent
363 showNotification(22)
364
365 // wait for the cleanup goroutine in addNotification to finish in case it gets called (which it shouldn't!)
366 wg.Wait()
367
368 // check we that have got 22 notifications now
369 c.Check(mmu.notifications, HasLen, 22)
370
371 // set back the lastCleanupTime to 11 minutes ago
372 mmu.lastCleanupTime = mmu.lastCleanupTime.Add(-11 * time.Minute)
373
374 // patch cNotificationExists to return false for all but the next one we are going to add
375 cNotificationExists = func(did string, nid string) bool {
376 return nid == "notif-id-23"
377 }
378
379 // adding another notification should remove all previous ones now
380 showNotification(23)
381
382 // wait for the cleanup goroutine in addNotification to finish
383 wg.Wait()
384
385 // check that all notifications except the last one have been removed
386 c.Check(mmu.notifications, HasLen, 1)
335}387}
336388
337func (ms *MessagingSuite) TestGetCh(c *C) {389func (ms *MessagingSuite) TestGetCh(c *C) {

Subscribers

People subscribed via source and target branches