Code review comment for lp:~nikwen/account-polld/authenticate-again-fix

Niklas Wenzel (nikwen) wrote :

Hi,

Of course, I can explain what this fix does. Let's have a look at what the current code in trunk does.

In the Poll() function it checks whether the account should be polled based on whether auth data is given, based on whether the application is blacklisted and based on the current penalty count. (http://bazaar.launchpad.net/~ubuntu-push-hackers/account-polld/trunk/view/99/cmd/account-polld/account_manager.go#L64)

If those tests pass (which is what they do in my log file), the poll() function is called. (http://bazaar.launchpad.net/~ubuntu-push-hackers/account-polld/trunk/view/99/cmd/account-polld/account_manager.go#L109)
It outputs "Polling account <account-number>" and checks whether the appropriate click package for the account type (e.g. the Gmail webapp) is installed and if it's not, it aborts polling.
The next thing it does is checking whether authentication failed and if it did, it will abort.
And here comes the problem. If an account fails to authenticate, it won't try to login again as what happens is this: Since a.authData.Error is still set to something else than nil, poll() will always abort. Therefore, this account won't be polled again.

However, I'm seeing the problem you described. It's that if auth fails in the following "else if", it will still try to poll the account, right? http://bazaar.launchpad.net/~ubuntu-push-hackers/account-polld/trunk/view/99/cmd/account-polld/main.go#L115

So you are right and this should be fixed somewhere else. I'll look into where to fix it.

Looking at an example with Tokens which expired, I also found the following issue:

It does not reset the a.authData.Error field to nil when it grabs new tokens.
If the token expires, it calls the account_watcher_refresh() function (http://bazaar.launchpad.net/~ubuntu-push-hackers/account-polld/trunk/view/99/accounts/account-watcher.c#L298) from the poll() function (http://bazaar.launchpad.net/~ubuntu-push-hackers/account-polld/trunk/view/99/cmd/account-polld/account_manager.go#L129). This will soon call the account_info_clear_login() function (http://bazaar.launchpad.net/~ubuntu-push-hackers/account-polld/trunk/view/99/accounts/account-watcher.c#L60), which is in my opinion the proper place to reset the Error field.

I added a workaround for this in the following fix, so don't merge this one either. https://code.launchpad.net/~nikwen/account-polld/directly-poll-with-new-account-data-fix/+merge/238901

I'll try to fix both bugs properly next weekend. Therefore, you don't need to look into merging any of my merge proposals until then.

« Back to merge proposal