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
1=== modified file 'cmd/account-polld/account_manager.go'
2--- cmd/account-polld/account_manager.go 2014-10-02 13:58:49 +0000
3+++ cmd/account-polld/account_manager.go 2014-12-02 20:03:06 +0000
4@@ -1,6 +1,7 @@
5 /*
6 Copyright 2014 Canonical Ltd.
7 Authors: Sergio Schvezov <sergio.schvezov@canonical.com>
8+ Niklas Wenzel <nikwen.developer@gmail.com>
9
10 This program is free software: you can redistribute it and/or modify it
11 under the terms of the GNU General Public License version 3, as published
12@@ -20,6 +21,7 @@
13 import (
14 "log"
15 "time"
16+ "errors"
17
18 "launchpad.net/account-polld/accounts"
19 "launchpad.net/account-polld/plugins"
20@@ -40,10 +42,15 @@
21
22 var (
23 pollTimeout = time.Duration(30 * time.Second)
24- bootstrapPollTimeout = time.Duration(5 * time.Minute)
25+ bootstrapPollTimeout = time.Duration(4 * time.Minute)
26 maxCounter = 4
27 )
28
29+var (
30+ authError = errors.New("Skipped account")
31+ clickNotInstalledError = errors.New("Click not installed")
32+)
33+
34 var isBlacklisted = cblacklist.IsBlacklisted
35
36 func NewAccountManager(watcher *accounts.Watcher, postWatch chan *PostWatch, plugin plugins.Plugin) *AccountManager {
37@@ -97,7 +104,9 @@
38 log.Println("Poll for account", a.authData.AccountId, "was successful")
39 a.penaltyCount = 0
40 } else {
41- log.Println("Poll for account", a.authData.AccountId, "has failed:", err)
42+ if (err != clickNotInstalledError && err != authError) { // Do not log the error twice
43+ log.Println("Poll for account", a.authData.AccountId, "has failed:", err)
44+ }
45 if a.penaltyCount < maxCounter {
46 a.penaltyCount++
47 }
48@@ -112,11 +121,13 @@
49 log.Println(
50 "Skipping account", a.authData.AccountId, "as target click",
51 a.plugin.ApplicationId(), "is not installed")
52+ a.doneChan <- clickNotInstalledError
53 return
54 }
55
56 if a.authData.Error != nil {
57 log.Println("Account", a.authData.AccountId, "failed to authenticate:", a.authData.Error)
58+ a.doneChan <- authError
59 return
60 }
61

Subscribers

People subscribed via source and target branches