poll-daemon

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

  1. donechan-fix
  2. Merge into trunk
Proposed by Niklas Wenzel
Status: Merged
Approved by: John Lenton
Approved revision: 104
Merged at revision: 113
Proposed branch: lp://staging/~nikwen/account-polld/donechan-fix
Merge into: lp://staging/~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://staging/~nikwen/account-polld/donechan-fix
Reviewer Review Type Date Requested Status
John Lenton (community) Approve
Review via email: mp+238898@code.staging.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.
Revision history for this message
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
Revision history for this message
Niklas Wenzel (nikwen) wrote :

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

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
John Lenton (chipaca) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches