Retry uploading in UploadJobImpl when error occurs

chrome/browser/chromeos/policy/upload_job_impl.cc

Index: chrome/browser/chromeos/policy/upload_job_impl.cc
diff --git a/chrome/browser/chromeos/policy/upload_job_impl.cc b/chrome/browser/chromeos/policy/upload_job_impl.cc
index d557c287b505fc11be6cbe0f215af769a4bd996e..d04477cd71d549b4317462dda603825c2fef8ba8 100644
--- a/chrome/browser/chromeos/policy/upload_job_impl.cc
+++ b/chrome/browser/chromeos/policy/upload_job_impl.cc
@@ -8,6 +8,7 @@
 #include <set>
 #include <utility>
 
+#include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/strings/stringprintf.h"
@@ -28,12 +29,15 @@ 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 each unsuccessful upload attempt.
+const long kRetryDelayMs = 25000;
+
 }  // namespace
 
 UploadJobImpl::Delegate::~Delegate() {
@@ -127,7 +131,8 @@ UploadJobImpl::UploadJobImpl(
     OAuth2TokenService* token_service,
     scoped_refptr<net::URLRequestContextGetter> url_context_getter,
     Delegate* delegate,
-    std::unique_ptr<MimeBoundaryGenerator> boundary_generator)
+    std::unique_ptr<MimeBoundaryGenerator> boundary_generator,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
     : OAuth2TokenService::Consumer("cros_upload_job"),
       upload_url_(upload_url),
       account_id_(account_id),
@@ -136,7 +141,9 @@ UploadJobImpl::UploadJobImpl(
       delegate_(delegate),
       boundary_generator_(std::move(boundary_generator)),
       state_(IDLE),
-      retry_(0) {
+      retry_(0),
+      task_runner_(task_runner),
+      weak_factory_(this) {
   DCHECK(token_service_);
   DCHECK(url_context_getter_);
   DCHECK(delegate_);
@@ -158,6 +165,7 @@ void UploadJobImpl::AddDataSegment(
   DCHECK_EQ(IDLE, state_);
   if (state_ != IDLE)
     return;
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   std::unique_ptr<DataSegment> data_segment(
       new DataSegment(name, filename, std::move(data), header_entries));
@@ -169,10 +177,16 @@ void UploadJobImpl::Start() {
   DCHECK_EQ(IDLE, state_);
   if (state_ != IDLE)
     return;
+  DCHECK_EQ(0, retry_);
+  DCHECK(thread_checker_.CalledOnValidThread());

[Andrew T Wilson (Slow), 2016/05/10 22:00:42]

Move call to CalledOnValidThread() to the top of the function, in every place
you use it in this file. It should be the first thing done in every function you
call it from.

[Marton Hunyady, 2016/05/11 12:55:30]

Done.

+
   RequestAccessToken();
 }
 
 void UploadJobImpl::RequestAccessToken() {
+  DCHECK(!access_token_request_);
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   state_ = ACQUIRING_TOKEN;
 
   OAuth2TokenService::ScopeSet scope_set;
@@ -264,13 +278,15 @@ void UploadJobImpl::CreateAndStartURLFetcher(const std::string& access_token) {
   upload_fetcher_->Start();
 }
 
-void UploadJobImpl::StartUpload(const std::string& access_token) {
+void UploadJobImpl::StartUpload() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   if (!SetUpMultipart()) {
     LOG(ERROR) << "Multipart message assembly failed.";
     state_ = ERROR;
     return;
   }
-  CreateAndStartURLFetcher(access_token);
+  CreateAndStartURLFetcher(access_token_);
   state_ = UPLOADING;
 }
 
@@ -284,7 +300,7 @@ void UploadJobImpl::OnGetTokenSuccess(
 
   // 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(
@@ -294,57 +310,83 @@ void UploadJobImpl::OnGetTokenFailure(
   DCHECK_EQ(access_token_request_.get(), request);
   access_token_request_.reset();
   LOG(ERROR) << "Token request failed: " << error.ToString();
-  state_ = ERROR;
-  delegate_->OnFailure(AUTHENTICATION_ERROR);
+  HandleError(AUTHENTICATION_ERROR);
+}
+
+void UploadJobImpl::HandleError(ErrorCode errorCode) {

[Andrew T Wilson (Slow), 2016/05/10 22:00:42]

errorCode -> error_code

[Marton Hunyady, 2016/05/11 12:55:30]

Done.

+  retry_++;
+  upload_fetcher_.reset();
+
+  std::string errorString;

[Andrew T Wilson (Slow), 2016/05/10 22:00:42]

errorString->error_string

[Marton Hunyady, 2016/05/11 12:55:30]

Done.

+  switch (errorCode) {
+    case NETWORK_ERROR:
+      errorString = "NETWORK_ERROR";
+      break;
+    case AUTHENTICATION_ERROR:
+      errorString = "AUTHENTICATION_ERROR";
+      break;
+    case SERVER_ERROR:
+      errorString = "SERVER_ERROR";
+      break;
+  }
+  LOG(ERROR) << "Upload failed: " << errorString;

[Andrew T Wilson (Slow), 2016/05/10 22:00:42]

This is OK, but I think it's superior to just log the errorCode itself.

[Marton Hunyady, 2016/05/11 12:55:30]

Done.

+
+  if (retry_ >= kMaxAttempts) {
+    // Maximum number of attempts reached, failure.
+    LOG(ERROR) << "Maximum number of attempts reached.";
+    access_token_.clear();
+    post_data_.reset();
+    state_ = ERROR;
+    delegate_->OnFailure(errorCode);
+  } else {
+    if (errorCode == AUTHENTICATION_ERROR) {
+      LOG(ERROR) << "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));
+    } else {
+      // Retry without a new token.
+      state_ = ACQUIRING_TOKEN;
+      LOG(WARNING) << "Retrying upload with the same token.";
+      task_runner_->PostDelayedTask(
+          FROM_HERE,
+          base::Bind(&UploadJobImpl::StartUpload, weak_factory_.GetWeakPtr()),
+          base::TimeDelta::FromMilliseconds(kRetryDelayMs));
+    }
+  }
 }
 
 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 upload
       upload_fetcher_.reset();
-      LOG(ERROR) << "Unauthorized request.";
-      state_ = ERROR;
+      access_token_.clear();
       post_data_.reset();
-      delegate_->OnFailure(AUTHENTICATION_ERROR);
-      return;
+      state_ = SUCCESS;
+      delegate_->OnSuccess();
+    } else if (response_code == net::HTTP_UNAUTHORIZED) {
+      LOG(ERROR) << "Unauthorized request.";
+      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);
   }
 }