Add delayed retry to DeviceManagementService requests.

components/policy/core/common/cloud/device_management_service.cc

 // Copyright (c) 2012 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 "components/policy/core/common/cloud/device_management_service.h"
 
+#include <cmath>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "net/base/escape.h"
@@ skipping 10 matching lines @@
 namespace policy {
 
 namespace {
 
 const char kPostContentType[] = "application/protobuf";
 
 const char kServiceTokenAuthHeader[] = "Authorization: GoogleLogin auth=";
 const char kDMTokenAuthHeader[] = "Authorization: GoogleDMToken token=";
 
 // Number of times to retry on ERR_NETWORK_CHANGED errors.
-const int kMaxNetworkChangedRetries = 3;
+const int kMaxRetries = 3;
 
 // HTTP Error Codes of the DM Server with their concrete meanings in the context
 // of the DM Server communication.
 const int kSuccess = 200;
 const int kInvalidArgument = 400;
 const int kInvalidAuthCookieOrDMToken = 401;
 const int kMissingLicenses = 402;
 const int kDeviceManagementNotAllowed = 403;
 const int kInvalidURL = 404;  // This error is not coming from the GFE.
 const int kInvalidSerialNumber = 405;
 const int kDomainMismatch = 406;
 const int kDeviceIdConflict = 409;
 const int kDeviceNotFound = 410;
 const int kPendingApproval = 412;
 const int kInternalServerError = 500;
 const int kServiceUnavailable = 503;
 const int kPolicyNotFound = 902;
 const int kDeprovisioned = 903;
 
+// Delay after first unsuccessful upload attempt. After each additional failure,
+// the delay increases exponentially. Can be changed for testing to prevent
+// timeouts.
+long g_retry_delay_ms = 10000;
+
 bool IsProxyError(const net::URLRequestStatus status) {
   switch (status.error()) {
     case net::ERR_PROXY_CONNECTION_FAILED:
     case net::ERR_TUNNEL_CONNECTION_FAILED:
     case net::ERR_PROXY_AUTH_UNSUPPORTED:
     case net::ERR_HTTPS_PROXY_TUNNEL_RESPONSE:
     case net::ERR_MANDATORY_PROXY_CONFIGURATION_FAILED:
     case net::ERR_PROXY_CERTIFICATE_INVALID:
     case net::ERR_SOCKS_CONNECTION_FAILED:
     case net::ERR_SOCKS_CONNECTION_HOST_UNREACHABLE:
       return true;
   }
   return false;
 }
 
+bool IsConnectionError(const net::URLRequestStatus status) {
+  switch (status.error()) {
+    case net::ERR_NETWORK_CHANGED:
+    case net::ERR_NAME_NOT_RESOLVED:
+    case net::ERR_INTERNET_DISCONNECTED:
+    case net::ERR_ADDRESS_UNREACHABLE:
+    case net::ERR_CONNECTION_TIMED_OUT:
+    case net::ERR_NAME_RESOLUTION_FAILED:
+      return true;
+  }
+  return false;
+}
+
 bool IsProtobufMimeType(const net::URLFetcher* fetcher) {
   return fetcher->GetResponseHeaders()->HasHeaderValue(
       "content-type", "application/x-protobuffer");
 }
 
 bool FailedWithProxy(const net::URLFetcher* fetcher) {
   if ((fetcher->GetLoadFlags() & net::LOAD_BYPASS_PROXY) != 0) {
     // The request didn't use a proxy.
     return false;
   }
@@ skipping 78 matching lines @@
   void ConfigureRequest(net::URLFetcher* fetcher);
 
   // Returns true if this job should be retried. |fetcher| has just completed,
   // and can be inspected to determine if the request failed and should be
   // retried.
   bool ShouldRetry(const net::URLFetcher* fetcher);
 
   // Invoked right before retrying this job.
   void PrepareRetry();
 
+  // Number of times that this job has been retried due to connection errors.
+  int retries_count() { return retries_count_; }
+
  protected:
   // DeviceManagementRequestJob:
   void Run() override;
 
  private:
   // Invokes the callback with the given error code.
   void ReportError(DeviceManagementStatus code);
 
   // Pointer to the service this job is associated with.
   DeviceManagementService* service_;
 
   // Whether the BYPASS_PROXY flag should be set by ConfigureRequest().
   bool bypass_proxy_;
 
-  // Number of times that this job has been retried due to ERR_NETWORK_CHANGED.
+  // Number of times that this job has been retried due to connection errors.
   int retries_count_;
 
   // The request context to use for this job.
   scoped_refptr<net::URLRequestContextGetter> request_context_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceManagementRequestJobImpl);
 };
 
 DeviceManagementRequestJobImpl::DeviceManagementRequestJobImpl(
     JobType type,
@@ skipping 130 matching lines @@
 bool DeviceManagementRequestJobImpl::ShouldRetry(
     const net::URLFetcher* fetcher) {
   if (FailedWithProxy(fetcher) && !bypass_proxy_) {
     // Retry the job if it failed due to a broken proxy, by bypassing the
     // proxy on the next try.
     bypass_proxy_ = true;
     return true;
   }
 
   // Early device policy fetches on ChromeOS and Auto-Enrollment checks are
-  // often interrupted during ChromeOS startup when network change notifications
-  // are sent. Allowing the fetcher to retry once after that is enough to
-  // recover; allow it to retry up to 3 times just in case.
-  if (fetcher->GetStatus().error() == net::ERR_NETWORK_CHANGED &&
-      retries_count_ < kMaxNetworkChangedRetries) {
+  // often interrupted during ChromeOS startup when network is not yet ready.
+  // Allowing the fetcher to retry once after that is enough to recover; allow
+  // it to retry up to 3 times just in case.
+  if (IsConnectionError(fetcher->GetStatus()) && retries_count_ < kMaxRetries) {
     ++retries_count_;
     return true;
   }
 
   // The request didn't fail, or the limit of retry attempts has been reached;
   // forward the result to the job owner.
   return false;
 }
 
 void DeviceManagementRequestJobImpl::PrepareRetry() {
@@ skipping 59 matching lines @@
 
 DeviceManagementService::~DeviceManagementService() {
   // All running jobs should have been cancelled by now.
   DCHECK(pending_jobs_.empty());
   DCHECK(queued_jobs_.empty());
 }
 
 DeviceManagementRequestJob* DeviceManagementService::CreateJob(
     DeviceManagementRequestJob::JobType type,
     const scoped_refptr<net::URLRequestContextGetter>& request_context) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   return new DeviceManagementRequestJobImpl(
       type,
       configuration_->GetAgentParameter(),
       configuration_->GetPlatformParameter(),
       this,
       request_context);
 }
 
 void DeviceManagementService::ScheduleInitialization(
     int64_t delay_milliseconds) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   if (initialized_)
     return;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+  task_runner_->PostDelayedTask(
       FROM_HERE, base::Bind(&DeviceManagementService::Initialize,
                             weak_ptr_factory_.GetWeakPtr()),
       base::TimeDelta::FromMilliseconds(delay_milliseconds));
 }
 
 void DeviceManagementService::Initialize() {
   if (initialized_)

[Andrew T Wilson (Slow), 2016/05/10 17:58:32]

Add a thread_checker_.CalledOnValidThread() to this routine as well, to help
ensure that the task_runner is bound to the correct thread.

[Marton Hunyady, 2016/05/11 09:40:08]

Done.

     return;
   initialized_ = true;
 
   while (!queued_jobs_.empty()) {
     StartJob(queued_jobs_.front());
     queued_jobs_.pop_front();
   }
 }
 
 void DeviceManagementService::Shutdown() {
+  DCHECK(thread_checker_.CalledOnValidThread());

[Andrew T Wilson (Slow), 2016/05/10 17:58:33]

This is fine as-is, but I do wonder if perhaps we should clear our
weak_ptr_factory_ to kill any queued tasks here as well.

[Marton Hunyady, 2016/05/11 09:40:08]

Done, I invalidated the pointers. (However, this method is only called from a
test currently.)

   for (JobFetcherMap::iterator job(pending_jobs_.begin());
        job != pending_jobs_.end();
        ++job) {
     delete job->first;
     queued_jobs_.push_back(job->second);
   }
   pending_jobs_.clear();
 }
 
 DeviceManagementService::DeviceManagementService(
     std::unique_ptr<Configuration> configuration)
     : configuration_(std::move(configuration)),
       initialized_(false),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
       weak_ptr_factory_(this) {
   DCHECK(configuration_);
 }
 
 void DeviceManagementService::StartJob(DeviceManagementRequestJobImpl* job) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   std::string server_url = GetServerUrl();
   net::URLFetcher* fetcher =
       net::URLFetcher::Create(kURLFetcherID, job->GetURL(server_url),
                               net::URLFetcher::POST, this).release();
   data_use_measurement::DataUseUserData::AttachToFetcher(
       fetcher, data_use_measurement::DataUseUserData::POLICY);
   job->ConfigureRequest(fetcher);
   pending_jobs_[fetcher] = job;
   fetcher->Start();
 }
 
 std::string DeviceManagementService::GetServerUrl() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   return configuration_->GetServerUrl();
 }
 
+// static
+void DeviceManagementService::SetRetryDelayForTesting(long retryDelayMs) {

[Andrew T Wilson (Slow), 2016/05/10 17:58:33]

nit: retry_delay_ms

[Marton Hunyady, 2016/05/11 09:40:08]

Done.

+  CHECK_GE(retryDelayMs, 0);
+  g_retry_delay_ms = retryDelayMs;
+}
+
 void DeviceManagementService::OnURLFetchComplete(
     const net::URLFetcher* source) {
   JobFetcherMap::iterator entry(pending_jobs_.find(source));
   if (entry == pending_jobs_.end()) {
     NOTREACHED() << "Callback from foreign URL fetcher";
     return;
   }
 
   DeviceManagementRequestJobImpl* job = entry->second;
   pending_jobs_.erase(entry);
 
   if (job->ShouldRetry(source)) {
-    VLOG(1) << "Retrying dmserver request.";
     job->PrepareRetry();
-    StartJob(job);
+    int delay = g_retry_delay_ms * (int)exp2(job->retries_count() - 1);

[Andrew T Wilson (Slow), 2016/05/10 17:58:33]

direct cast is bad, use static_cast.

Also, just consider g_retry_delay_ms << (job->retries_count() -1); I'm fine with
exp2() but I tend to see bit shifting more often than exp2() in the codebase.

[Marton Hunyady, 2016/05/11 09:40:08]

Done.

+    LOG(WARNING) << "Dmserver request failed, retrying in " << delay / 1000
+                 << "s.";
+    task_runner_->PostDelayedTask(
+        FROM_HERE, base::Bind(&DeviceManagementService::StartJob,
+                              weak_ptr_factory_.GetWeakPtr(), job),
+        base::TimeDelta::FromMilliseconds(delay));
   } else {
     std::string data;
     source->GetResponseAsString(&data);
     job->HandleResponse(source->GetStatus(), source->GetResponseCode(),
                         source->GetCookies(), data);
   }
   delete source;
 }
 
 void DeviceManagementService::AddJob(DeviceManagementRequestJobImpl* job) {
@@ skipping 14 matching lines @@
     }
   }
 
   const JobQueue::iterator elem =
       std::find(queued_jobs_.begin(), queued_jobs_.end(), job);
   if (elem != queued_jobs_.end())
     queued_jobs_.erase(elem);
 }
 
 }  // namespace policy