Update AttachmentServiceImpl to retry attachment uploads.

Update AttachmentServiceImpl to retry attachment uploads.

Add TaskQueue, a class that provides retry and backoff semantics for
tasks.  Used by AttachmentServiceImpl.

AttachmentUploaderImpl and AttachmentDownloaderImpl now differentiate
between transient and non-transient errors.  Almost all errors are
assumed to be transient.  "403 Forbidden" is the exception and is 
returned by the sync server if attachment are disabled for the user.

Transient errors encountered during attachment upload will be retried
with exponential backoff (using TaskQueue and BackoffEntry).  The idea
is to consolidate retry logic at the AttachmentServiceImpl level.

In a future CL, changes in network connectivity will affect retry and
backoff.

BUG=372622, 380437
Committed: https://crrev.com/c51cb78e34848822057a669f3ed1b69b7b2bd266
Cr-Commit-Position: refs/heads/master@{#294195}

[maniscalco, 2014-09-08 22:02:08]

maniscalco@chromium.org changed reviewers:
+ pavely@chromium.org

[maniscalco, 2014-09-08 22:02:09]

Pavel, would you please review this change?

I plan to integration with network connectivity monitoring in a future CL.

[pavely, 2014-09-09 19:42:24]

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_service_impl.cc (right):

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_service_impl.cc:247: break;
You have to call exactly one of three functions exactly once. You only call
Mark... or Retry and sometimes don't call anything at all. What will happen with
task_queue if task gets unspecified error?

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_service_impl.cc:264: }
Add default: NOTREACHED(). It will show that you are not missing cases.

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl.cc (right):

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:152: }
We discussed that all 4xx except "forbidden" are persistent errors. will you
make corresponding change in the future?

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:190:
ReportResult(UPLOAD_TRANSIENT_ERROR, attachment_.GetId());
Could you put a TODO that errors should be treated differently here as well?
Let's discuss in person what the behavior should be.

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/public...
File sync/internal_api/public/attachments/task_queue.h (right):

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/public...
sync/internal_api/public/attachments/task_queue.h:28: template <typename T>
It would be useful to put comment with example of usage before class in header.
Also it would be a great place to mention that for every invoked task you have
to call exactly one on the three functions exactly once.

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/public...
sync/internal_api/public/attachments/task_queue.h:219: queue_.pop_front();
task variable doesn't own task. pop_front will delete it.
You should PostTask first and then pop_front.

[pavely, 2014-09-09 19:47:42]

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl.cc (right):

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:152: }
On 2014/09/09 19:42:23, pavely wrote:
> We discussed that all 4xx except "forbidden" are persistent errors. will you
> make corresponding change in the future?

Sorry, I meant "unauthorized"

[maniscalco, 2014-09-09 21:15:44]

Thanks for the feedback!  PTAL.

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_service_impl.cc (right):

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_service_impl.cc:247: break;
On 2014/09/09 19:42:23, pavely wrote:
> You have to call exactly one of three functions exactly once. You only call
> Mark... or Retry and sometimes don't call anything at all. What will happen
with
> task_queue if task gets unspecified error?

Good catch.  There is a bug here.  There is no "completion" method called when
UploadDone gets AttachmentUploader::UPLOAD_UNSPECIFIED_ERROR...

OK, I've (hopefully) now simplified TaskQueue by replacing the Retry method with
MarkAsFailed.  PTAL and LMK WYT.

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_service_impl.cc:264: }
On 2014/09/09 19:42:23, pavely wrote:
> Add default: NOTREACHED(). It will show that you are not missing cases.

I'm not sure that's preferable to no default.  Without default, I get a compile
time error if I forget to handle a case.  With default+NOTREACHED, I'll only get
a runtime error if I hit that case.

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl.cc (right):

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:152: }
On 2014/09/09 19:47:42, pavely wrote:
> On 2014/09/09 19:42:23, pavely wrote:
> > We discussed that all 4xx except "forbidden" are persistent errors. will you
> > make corresponding change in the future?
> 
> Sorry, I meant "unauthorized"

Acknowledged.

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:152: }
On 2014/09/09 19:42:23, pavely wrote:
> We discussed that all 4xx except "forbidden" are persistent errors. will you
> make corresponding change in the future?

I think you mean all are transient except for forbidden (forbidden is returned
when the user is not on the whitelist for attachments).  Right?  My goal is to
return UPLOAD_TRANSIENT_ERROR for all 4XXs except forbidden.  I believe that's
what this code does, or did I misunderstand?

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:190:
ReportResult(UPLOAD_TRANSIENT_ERROR, attachment_.GetId());
On 2014/09/09 19:42:23, pavely wrote:
> Could you put a TODO that errors should be treated differently here as well?
> Let's discuss in person what the behavior should be.

Happy to chat more in person.  TODO added.

Were you thinking we should examine the cause of failing to get a token?  That
sometimes you don't want to retry here?

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/public...
File sync/internal_api/public/attachments/task_queue.h (right):

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/public...
sync/internal_api/public/attachments/task_queue.h:28: template <typename T>
On 2014/09/09 19:42:23, pavely wrote:
> It would be useful to put comment with example of usage before class in
header.
> Also it would be a great place to mention that for every invoked task you have
> to call exactly one on the three functions exactly once. 

Good idea.  Done.

https://codereview.chromium.org/554743004/diff/80001/sync/internal_api/public...
sync/internal_api/public/attachments/task_queue.h:219: queue_.pop_front();
On 2014/09/09 19:42:23, pavely wrote:
> task variable doesn't own task. pop_front will delete it.
> You should PostTask first and then pop_front.

Yikes.  Good catch.  Fixed.  I mistakenly thought I was getting temporary
lifetime extension by const reference
(http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-c...).

[pavely, 2014-09-10 00:11:43]

lgtm

[maniscalco, 2014-09-10 16:08:25]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-09-10 16:09:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/maniscalco@chromium.org/554743004/120001

[commit-bot: I haz the power, 2014-09-10 16:14:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 16:14:10]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54080)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/59032)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[maniscalco, 2014-09-10 17:27:00]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-09-10 17:30:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/maniscalco@chromium.org/554743004/140001

[commit-bot: I haz the power, 2014-09-10 18:31:22]

Committed patchset #8 (id:140001) as 81d9236b2364168ac2c427bd1b135a6d933bf52a

[commit-bot: I haz the power, 2014-09-10 18:37:59]

Patchset 8 (id:??) landed as
https://crrev.com/c51cb78e34848822057a669f3ed1b69b7b2bd266
Cr-Commit-Position: refs/heads/master@{#294195}