Merge lp:~nikwen/account-polld/donechan-fix into lp:~ubuntu-push-hackers/account-polld/trunk

Proposed by Niklas Wenzel on 2014-10-20
Status: Merged
Approved by: John Lenton on 2015-03-20
Approved revision: 104
Merged at revision: 113
Proposed branch: lp:~nikwen/account-polld/donechan-fix
Merge into: lp:~ubuntu-push-hackers/account-polld/trunk
Diff against target: 60 lines (+13/-2)
1 file modified
cmd/account-polld/account_manager.go (+13/-2)
To merge this branch: bzr merge lp:~nikwen/account-polld/donechan-fix
Reviewer Review Type Date Requested Status
John Lenton 2014-10-20 Approve on 2015-03-20
Review via email: mp+238898@code.launchpad.net

Description of the change

The poll() function in "cmd/account-polld/account_manager.go" always needs to provide an output to a.doneChan. Otherwise the select in Poll() does not know poll() has finished and produces a timeout.

Excerpt from the logs:

2014/10/19 17:23:15 Starting poll for account 4
2014/10/19 17:23:15 Polling account 4
2014/10/19 17:23:15 Account 4 failed to authenticate: GDBus.Error:com.google.code.AccountsSSO.SingleSignOn.Error.NoConnection: Host accounts.google.com not found
2014/10/19 17:23:45 Poll for account 4 has timed out out after 30s
2014/10/19 17:23:45 Ending poll for account 4

To post a comment you must log in.
John Lenton (chipaca) wrote :

Finally have time to review this! Sorry for the delay.

While the bug is real, I have nits with your fix:

the doneChan is a channel for errors, but you're sending nil (no error) for at least two cases that are clearly errors. This needs fixing; I suggest returning instead ad-hoc errors and logging them in a single place (so e.g. returning fmt.Errorf("failed to authenticate: %v", a.authData.Error) instead of log-and-return-nil; then the "has failed:", err in the error path should suffice).

This will have the side effect of bumping penaltyCount for auth errors and click-not-installed, which is not necessarily a bad thing.

review: Needs Fixing
Niklas Wenzel (nikwen) wrote :

Ok, I'll try to fix this next weekend. Thanks for your review. :)

Niklas Wenzel (nikwen) wrote :

I finally had some time to submit my fix.
In addition to fixing it the way you suggested, I reduced the bootstrapPollTimeout. Here's why (correct me if I'm wrong):

When the ubuntu-push-client polls the account-polld package for the first time, it will wait for a reply for five minutes. Afterwards, if it doesn't get an answer, it will not poll account-polld again. However, it takes a few (milli-)seconds until the commands in account-polld are actually run. So even if it aborts after 5 minutes, ubuntu-push-client's 5 minutes will already be over.
The logical consequence was to reduce the bootstrapPollTimeout.

Niklas Wenzel (nikwen) wrote :

> When the ubuntu-push-client polls the account-polld package for the first
> time, it will wait for a reply for five minutes. Afterwards, if it doesn't get
> an answer, it will not poll account-polld again. However, it takes a few
> (milli-)seconds until the commands in account-polld are actually run. So even
> if it aborts after 5 minutes, ubuntu-push-client's 5 minutes will already be
> over.
> The logical consequence was to reduce the bootstrapPollTimeout.

Ok, I found out that the first poll is done by account-polld itself, so we don't have this issue. However, I'd still keep the reduced timeout as it will then poll the remaining accounts instantly when the first poll initiated by the ubuntu-push-client is done. Otherwise, it would have to wait for the previous query to be finished and I seriously don't believe that an account will be able to authenticate after that extra 1 minute if it hasn't done previously.

John Lenton (chipaca) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'cmd/account-polld/account_manager.go'
--- cmd/account-polld/account_manager.go 2014-10-02 13:58:49 +0000
+++ cmd/account-polld/account_manager.go 2014-12-02 20:03:06 +0000
@@ -1,6 +1,7 @@
1/*1/*
2 Copyright 2014 Canonical Ltd.2 Copyright 2014 Canonical Ltd.
3 Authors: Sergio Schvezov <sergio.schvezov@canonical.com>3 Authors: Sergio Schvezov <sergio.schvezov@canonical.com>
4 Niklas Wenzel <nikwen.developer@gmail.com>
45
5 This program is free software: you can redistribute it and/or modify it6 This program is free software: you can redistribute it and/or modify it
6 under the terms of the GNU General Public License version 3, as published7 under the terms of the GNU General Public License version 3, as published
@@ -20,6 +21,7 @@
20import (21import (
21 "log"22 "log"
22 "time"23 "time"
24 "errors"
2325
24 "launchpad.net/account-polld/accounts"26 "launchpad.net/account-polld/accounts"
25 "launchpad.net/account-polld/plugins"27 "launchpad.net/account-polld/plugins"
@@ -40,10 +42,15 @@
4042
41var (43var (
42 pollTimeout = time.Duration(30 * time.Second)44 pollTimeout = time.Duration(30 * time.Second)
43 bootstrapPollTimeout = time.Duration(5 * time.Minute)45 bootstrapPollTimeout = time.Duration(4 * time.Minute)
44 maxCounter = 446 maxCounter = 4
45)47)
4648
49var (
50 authError = errors.New("Skipped account")
51 clickNotInstalledError = errors.New("Click not installed")
52)
53
47var isBlacklisted = cblacklist.IsBlacklisted54var isBlacklisted = cblacklist.IsBlacklisted
4855
49func NewAccountManager(watcher *accounts.Watcher, postWatch chan *PostWatch, plugin plugins.Plugin) *AccountManager {56func NewAccountManager(watcher *accounts.Watcher, postWatch chan *PostWatch, plugin plugins.Plugin) *AccountManager {
@@ -97,7 +104,9 @@
97 log.Println("Poll for account", a.authData.AccountId, "was successful")104 log.Println("Poll for account", a.authData.AccountId, "was successful")
98 a.penaltyCount = 0105 a.penaltyCount = 0
99 } else {106 } else {
100 log.Println("Poll for account", a.authData.AccountId, "has failed:", err)107 if (err != clickNotInstalledError && err != authError) { // Do not log the error twice
108 log.Println("Poll for account", a.authData.AccountId, "has failed:", err)
109 }
101 if a.penaltyCount < maxCounter {110 if a.penaltyCount < maxCounter {
102 a.penaltyCount++111 a.penaltyCount++
103 }112 }
@@ -112,11 +121,13 @@
112 log.Println(121 log.Println(
113 "Skipping account", a.authData.AccountId, "as target click",122 "Skipping account", a.authData.AccountId, "as target click",
114 a.plugin.ApplicationId(), "is not installed")123 a.plugin.ApplicationId(), "is not installed")
124 a.doneChan <- clickNotInstalledError
115 return125 return
116 }126 }
117127
118 if a.authData.Error != nil {128 if a.authData.Error != nil {
119 log.Println("Account", a.authData.AccountId, "failed to authenticate:", a.authData.Error)129 log.Println("Account", a.authData.AccountId, "failed to authenticate:", a.authData.Error)
130 a.doneChan <- authError
120 return131 return
121 }132 }
122133

Subscribers

People subscribed via source and target branches