Retry uploading in UploadJobImpl when error occurs

chrome/browser/chromeos/policy/upload_job_impl.cc

 // Copyright (c) 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/policy/upload_job_impl.h"
 
 #include <stddef.h>
 #include <set>
 #include <utility>
 
+#include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/strings/stringprintf.h"
 #include "google_apis/gaia/gaia_constants.h"
 #include "google_apis/gaia/google_service_auth_error.h"
 #include "net/base/mime_util.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_request_status.h"
 
 namespace policy {
 
 namespace {
 
 // Format for bearer tokens in HTTP requests to access OAuth 2.0 protected
 // resources.
 const char kAuthorizationHeaderFormat[] = "Authorization: Bearer %s";
 
 // Value the "Content-Type" field will be set to in the POST request.
 const char kUploadContentType[] = "multipart/form-data";
 
-// Number of upload retries.
-const int kMaxRetries = 1;
+// Number of upload attempts.
+const int kMaxAttempts = 4;
 
 // Max size of MIME boundary according to RFC 1341, section 7.2.1.
 const size_t kMaxMimeBoundarySize = 70;
 
+// Delay after first unsuccessful upload attempt. After each additional failure,
+// the delay increases with this value.

[Andrew T Wilson (Slow), 2016/05/02 09:15:49]

Consider just having a simple fixed 25s delay - the important thing is that we
try a few times before the server times out, actually increasing the retry
interval isn't important so I'd err towards simplicity.

[Marton Hunyady, 2016/05/04 11:26:41]

Done.

+// Value chosen because the server is waiting 2 minutes for a screenshot.
+// 1st attempt is 0s after request, 2nd: 18s, 3rd: 54s, 4th: 108s.
+const long kRetryDelayMs = 18000;
+
 }  // namespace
 
 UploadJobImpl::Delegate::~Delegate() {
 }
 
 UploadJobImpl::MimeBoundaryGenerator::~MimeBoundaryGenerator() {
 }
 
 UploadJobImpl::RandomMimeBoundaryGenerator::~RandomMimeBoundaryGenerator() {
 }
@@ skipping 73 matching lines @@
     const {
   return net::GenerateMimeMultipartBoundary();
 }
 
 UploadJobImpl::UploadJobImpl(
     const GURL& upload_url,
     const std::string& account_id,
     OAuth2TokenService* token_service,
     scoped_refptr<net::URLRequestContextGetter> url_context_getter,
     Delegate* delegate,
-    std::unique_ptr<MimeBoundaryGenerator> boundary_generator)
+    std::unique_ptr<MimeBoundaryGenerator> boundary_generator,
+    const scoped_refptr<base::SequencedTaskRunner> task_runner)

[Andrew T Wilson (Slow), 2016/05/02 09:15:49]

pass as a reference

[Marton Hunyady, 2016/05/04 11:26:41]

Removed const, passing as value.

     : OAuth2TokenService::Consumer("cros_upload_job"),
       upload_url_(upload_url),
       account_id_(account_id),
       token_service_(token_service),
       url_context_getter_(url_context_getter),
       delegate_(delegate),
       boundary_generator_(std::move(boundary_generator)),
       state_(IDLE),
-      retry_(0) {
+      retry_(0),
+      task_runner_(task_runner),

[Andrew T Wilson (Slow), 2016/05/02 09:15:49]

Now that we have a task_runner, we need to make sure we're being called on the
right thread. So I would recommend adding a base::ThreadChecker to this class,
and calling thread_checker_.CalledOnValidThread() in every method that is
invoked via the task_runner_ (as well as any public methods).

[Marton Hunyady, 2016/05/04 11:26:41]

Done.

+      weak_factory_(this) {
   DCHECK(token_service_);
   DCHECK(url_context_getter_);
   DCHECK(delegate_);
   if (!upload_url_.is_valid()) {
     state_ = ERROR;
     NOTREACHED() << upload_url_ << " is not a valid URL.";
   }
 }
 
 UploadJobImpl::~UploadJobImpl() {
@@ skipping 12 matching lines @@
   std::unique_ptr<DataSegment> data_segment(
       new DataSegment(name, filename, std::move(data), header_entries));
   data_segments_.push_back(std::move(data_segment));
 }
 
 void UploadJobImpl::Start() {
   // Cannot start an upload on a busy or failed instance.
   DCHECK_EQ(IDLE, state_);
   if (state_ != IDLE)
     return;
+  retry_ = 0;

[Andrew T Wilson (Slow), 2016/05/02 09:15:49]

Why is this necessary? You can only get to IDLE state via the constructor, which
sets retry_ to 0.

If you are concerned that retry_ might not be 0 here, then add a DCHECK_EQ(0,
retry_);

[Marton Hunyady, 2016/05/04 11:26:41]

Done.

   RequestAccessToken();
 }
 
 void UploadJobImpl::RequestAccessToken() {
   state_ = ACQUIRING_TOKEN;
 
   OAuth2TokenService::ScopeSet scope_set;
   scope_set.insert(GaiaConstants::kDeviceManagementServiceOAuth);
   access_token_request_ =
       token_service_->StartRequest(account_id_, scope_set, this);
@@ skipping 75 matching lines @@
 
   upload_fetcher_ =
       net::URLFetcher::Create(upload_url_, net::URLFetcher::POST, this);
   upload_fetcher_->SetRequestContext(url_context_getter_.get());
   upload_fetcher_->SetUploadData(content_type, *post_data_);
   upload_fetcher_->AddExtraRequestHeader(
       base::StringPrintf(kAuthorizationHeaderFormat, access_token.c_str()));
   upload_fetcher_->Start();
 }
 
-void UploadJobImpl::StartUpload(const std::string& access_token) {
+void UploadJobImpl::StartUpload() {
   if (!SetUpMultipart()) {
     LOG(ERROR) << "Multipart message assembly failed.";
     state_ = ERROR;
     return;
   }
-  CreateAndStartURLFetcher(access_token);
+  CreateAndStartURLFetcher(access_token_);
   state_ = UPLOADING;
 }
 
 void UploadJobImpl::OnGetTokenSuccess(
     const OAuth2TokenService::Request* request,
     const std::string& access_token,
     const base::Time& expiration_time) {
   DCHECK_EQ(ACQUIRING_TOKEN, state_);
   DCHECK_EQ(access_token_request_.get(), request);
   access_token_request_.reset();
 
   // Also cache the token locally, so that we can revoke it later if necessary.
   access_token_ = access_token;
-  StartUpload(access_token);
+  StartUpload();
 }
 
 void UploadJobImpl::OnGetTokenFailure(
     const OAuth2TokenService::Request* request,
     const GoogleServiceAuthError& error) {
   DCHECK_EQ(ACQUIRING_TOKEN, state_);
   DCHECK_EQ(access_token_request_.get(), request);
-  access_token_request_.reset();

[Andrew T Wilson (Slow), 2016/05/02 09:15:49]

Why did you remove this? I think you still want to clear out the
access_token_request_, even though it will be overwritten by RequestAccessToken.
In fact, you may want to add a DCHECK_EQ(nullptr, access_token_request_) in
RequestAccessToken() just to make sure we don't call it with an existing request
already active.

[Marton Hunyady, 2016/05/04 11:26:41]

Done.

   LOG(ERROR) << "Token request failed: " << error.ToString();
-  state_ = ERROR;
-  delegate_->OnFailure(AUTHENTICATION_ERROR);
+  HandleError(AUTHENTICATION_ERROR);
+}
+
+void UploadJobImpl::HandleError(ErrorCode errorCode) {
+  retry_++;

[Andrew T Wilson (Slow), 2016/05/02 09:15:49]

Should log the errorCode here.

[Marton Hunyady, 2016/05/04 11:26:41]

Done.

+  upload_fetcher_.reset();
+  if (retry_ >= kMaxAttempts) {
+    // Maximum number of attempts reached, failure.
+    LOG(ERROR) << "Upload failed, maximum number of attempts reached.";
+    access_token_.clear();
+    post_data_.reset();
+    state_ = ERROR;
+    delegate_->OnFailure(errorCode);
+  } else {
+    if (errorCode == AUTHENTICATION_ERROR) {
+      LOG(ERROR) << "Upload failed, retrying upload with a new token.";
+      // Request new token and retry.
+      OAuth2TokenService::ScopeSet scope_set;
+      scope_set.insert(GaiaConstants::kDeviceManagementServiceOAuth);
+      token_service_->InvalidateAccessToken(account_id_, scope_set,
+                                            access_token_);
+      access_token_.clear();
+      task_runner_->PostDelayedTask(
+          FROM_HERE, base::Bind(&UploadJobImpl::RequestAccessToken,
+                                weak_factory_.GetWeakPtr()),
+          base::TimeDelta::FromMilliseconds(kRetryDelayMs * retry_));
+    } else {
+      // Retry without a new token.
+      state_ = ACQUIRING_TOKEN;
+      LOG(WARNING) << "Upload failed, retrying upload with the same token.";
+      task_runner_->PostDelayedTask(
+          FROM_HERE,
+          base::Bind(&UploadJobImpl::StartUpload, weak_factory_.GetWeakPtr()),
+          base::TimeDelta::FromMilliseconds(kRetryDelayMs * retry_));
+    }
+  }
 }
 
 void UploadJobImpl::OnURLFetchComplete(const net::URLFetcher* source) {
   DCHECK_EQ(upload_fetcher_.get(), source);
+  DCHECK_EQ(UPLOADING, state_);
   const net::URLRequestStatus& status = source->GetStatus();
   if (!status.is_success()) {
     LOG(ERROR) << "URLRequestStatus error " << status.error();
-    upload_fetcher_.reset();
-    state_ = ERROR;
-    post_data_.reset();
-    delegate_->OnFailure(NETWORK_ERROR);
-    return;
-  }
-
-  const int response_code = source->GetResponseCode();
-  const bool success = response_code == net::HTTP_OK;
-  if (!success)
-    LOG(ERROR) << "POST request failed with HTTP status code " << response_code;
-
-  if (response_code == net::HTTP_UNAUTHORIZED) {
-    if (retry_ >= kMaxRetries) {
+    HandleError(NETWORK_ERROR);
+  } else {
+    const int response_code = source->GetResponseCode();
+    if (response_code == net::HTTP_OK) {
+      // Successful uploading

[Andrew T Wilson (Slow), 2016/05/02 09:15:49]

nit: uploading -> upload.

[Marton Hunyady, 2016/05/04 11:26:41]

Done.

       upload_fetcher_.reset();
+      access_token_.clear();
+      post_data_.reset();
+      state_ = SUCCESS;
+      delegate_->OnSuccess();
+    } else if (response_code == net::HTTP_UNAUTHORIZED) {
       LOG(ERROR) << "Unauthorized request.";
-      state_ = ERROR;
-      post_data_.reset();
-      delegate_->OnFailure(AUTHENTICATION_ERROR);
-      return;
+      HandleError(AUTHENTICATION_ERROR);
+    } else {
+      LOG(ERROR) << "POST request failed with HTTP status code "
+                 << response_code << ".";
+      HandleError(SERVER_ERROR);
     }
-    retry_++;
-    upload_fetcher_.reset();
-    OAuth2TokenService::ScopeSet scope_set;
-    scope_set.insert(GaiaConstants::kDeviceManagementServiceOAuth);
-    token_service_->InvalidateAccessToken(account_id_, scope_set,
-                                          access_token_);
-    access_token_.clear();
-    RequestAccessToken();
-    return;
-  }
-
-  upload_fetcher_.reset();
-  access_token_.clear();
-  upload_fetcher_.reset();
-  post_data_.reset();
-  if (success) {
-    state_ = SUCCESS;
-    delegate_->OnSuccess();
-  } else {
-    state_ = ERROR;
-    delegate_->OnFailure(SERVER_ERROR);
   }
 }
 
 }  // namespace policy