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

[Niklas Wenzel]

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

[John Lenton (chipaca)]

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)]

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

[Niklas Wenzel (nikwen)]

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)]

> 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

=== 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 @@
 /*
  Copyright 2014 Canonical Ltd.
  Authors: Sergio Schvezov <[e-mail]>
+          Niklas Wenzel <[e-mail]>
 
  This program is free software: you can redistribute it and/or modify it
  under the terms of the GNU General Public License version 3, as published
@@ -20,6 +21,7 @@
 import (
 	"log"
 	"time"
+	"errors"
 
 	"launchpad.net/account-polld/accounts"
 	"launchpad.net/account-polld/plugins"
@@ -40,10 +42,15 @@
 
 var (
 	pollTimeout          = time.Duration(30 * time.Second)
-	bootstrapPollTimeout = time.Duration(5 * time.Minute)
+	bootstrapPollTimeout = time.Duration(4 * time.Minute)
 	maxCounter           = 4
 )
 
+var (
+	authError              = errors.New("Skipped account")
+	clickNotInstalledError = errors.New("Click not installed")
+)
+
 var isBlacklisted = cblacklist.IsBlacklisted
 
 func NewAccountManager(watcher *accounts.Watcher, postWatch chan *PostWatch, plugin plugins.Plugin) *AccountManager {
@@ -97,7 +104,9 @@
 			log.Println("Poll for account", a.authData.AccountId, "was successful")
 			a.penaltyCount = 0
 		} else {
-			log.Println("Poll for account", a.authData.AccountId, "has failed:", err)
+			if (err != clickNotInstalledError && err != authError) { // Do not log the error twice
+				log.Println("Poll for account", a.authData.AccountId, "has failed:", err)
+			}
 			if a.penaltyCount < maxCounter {
 				a.penaltyCount++
 			}
@@ -112,11 +121,13 @@
 		log.Println(
 			"Skipping account", a.authData.AccountId, "as target click",
 			a.plugin.ApplicationId(), "is not installed")
+		a.doneChan <- clickNotInstalledError
 		return
 	}
 
 	if a.authData.Error != nil {
 		log.Println("Account", a.authData.AccountId, "failed to authenticate:", a.authData.Error)
+		a.doneChan <- authError
 		return
 	}