Don't retry fetching policies if it's a blocking operation,

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 <utility>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
@@ skipping 158 matching lines @@
   void HandleResponse(const net::URLRequestStatus& status,
                       int response_code,
                       const std::string& data);
 
   // Gets the URL to contact.
   GURL GetURL(const std::string& server_url);
 
   // Configures the fetcher, setting up payload and headers.
   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);
+  enum RetryMethod { NO_RETRY, RETRY_IMMEDIATELY, RETRY_WITH_DELAY };

[Andrew T Wilson (Slow), 2016/08/23 14:53:08]

In general, enums are not crammed on one line like this (makes it hard to
comment the various types). let's break this up like so:

enum RetryMethod {
  // No retry required for this request
  NO_RETRY,

  // Should retry immediately (no delay)
  RETRY_IMMEDIATELY,
...

[Marton Hunyady, 2016/08/23 16:00:38]

Done.

+
+  // Returns if and how this job should be retried. |fetcher| has just
+  // completed, and can be inspected to determine if the request failed and
+  // should be retried.
+  RetryMethod 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_; }
 
   // Get weak pointer
   base::WeakPtr<DeviceManagementRequestJobImpl> GetWeakPtr() {
     return weak_ptr_factory_.GetWeakPtr();
@@ skipping 149 matching lines @@
   CHECK(request_.SerializeToString(&payload));
   fetcher->SetUploadData(kPostContentType, payload);
   std::string extra_headers;
   if (!gaia_token_.empty())
     extra_headers += kServiceTokenAuthHeader + gaia_token_ + "\n";
   if (!dm_token_.empty())
     extra_headers += kDMTokenAuthHeader + dm_token_ + "\n";
   fetcher->SetExtraRequestHeaders(extra_headers);
 }
 
-bool DeviceManagementRequestJobImpl::ShouldRetry(
-    const net::URLFetcher* fetcher) {
+DeviceManagementRequestJobImpl::RetryMethod
+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.
+    // Retry the job immediately if it failed due to a broken proxy, by
+    // bypassing the proxy on the next try.
     bypass_proxy_ = true;
-    return true;
+    return RETRY_IMMEDIATELY;
   }
 
   // Early device policy fetches on ChromeOS and Auto-Enrollment checks are
   // 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) {
+  if (IsConnectionError(fetcher->GetStatus()) && retries_count_ < kMaxRetries &&
+      type_ != DeviceManagementRequestJob::TYPE_POLICY_FETCH) {

[Andrew T Wilson (Slow), 2016/08/23 14:53:08]

Per our conversation, let's retry TYPE_POLICY_FETCH immediately, and update the
comment above about why we do it.

[Marton Hunyady, 2016/08/23 16:00:38]

Done.

     ++retries_count_;
-    return true;
+    return RETRY_WITH_DELAY;
   }
 
   // The request didn't fail, or the limit of retry attempts has been reached;
   // forward the result to the job owner.
-  return false;
+  return NO_RETRY;
 }
 
 void DeviceManagementRequestJobImpl::PrepareRetry() {
   if (!retry_callback_.is_null())
     retry_callback_.Run(this);
 }
 
 void DeviceManagementRequestJobImpl::ReportError(DeviceManagementStatus code) {
   em::DeviceManagementResponse dummy_response;
   callback_.Run(code, net::OK, dummy_response);
@@ skipping 17 matching lines @@
   AddParameter(dm_protocol::kParamDeviceID, client_id);
 }
 
 em::DeviceManagementRequest* DeviceManagementRequestJob::GetRequest() {
   return &request_;
 }
 
 DeviceManagementRequestJob::DeviceManagementRequestJob(
     JobType type,
     const std::string& agent_parameter,
-    const std::string& platform_parameter) {
+    const std::string& platform_parameter)
+    : type_(type) {
   AddParameter(dm_protocol::kParamRequest, JobTypeToRequestType(type));
   AddParameter(dm_protocol::kParamDeviceType, dm_protocol::kValueDeviceType);
   AddParameter(dm_protocol::kParamAppType, dm_protocol::kValueAppType);
   AddParameter(dm_protocol::kParamAgent, agent_parameter);
   AddParameter(dm_protocol::kParamPlatform, platform_parameter);
 }
 
 void DeviceManagementRequestJob::SetRetryCallback(
     const RetryCallback& retry_callback) {
   retry_callback_ = retry_callback;
@@ skipping 114 matching lines @@
     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)) {
+  DeviceManagementRequestJobImpl::RetryMethod retry_method =
+      job->ShouldRetry(source);
+  if (retry_method != DeviceManagementRequestJobImpl::RetryMethod::NO_RETRY) {
     job->PrepareRetry();
-    int delay = g_retry_delay_ms << (job->retries_count() - 1);
+    int delay =
+        retry_method ==

[Andrew T Wilson (Slow), 2016/08/23 14:53:08]

This is hard to read. Maybe break this out into a helper function:

int DeviceManagementRequestJobImpl::GetRetryDelay(RetryMethod method) {
  switch (method) {
    case RETRY_WITH_DELAY:
      return g_retry_delay_ms << (job->retries_count() - 1);
    case RETRY_IMMEDIATELY:
      return 0;
    default:
      NOTREACHED();
}

Then you can just use delay = job->GetRetryDelay(retry_method); This has the
nice side effect of keeping the retry timing/logic down in the JobImple.

[Marton Hunyady, 2016/08/23 16:00:38]

Done.

+                DeviceManagementRequestJobImpl::RetryMethod::RETRY_WITH_DELAY
+            ? g_retry_delay_ms << (job->retries_count() - 1)
+            : 0;
     LOG(WARNING) << "Dmserver request failed, retrying in " << delay / 1000
                  << "s.";
     task_runner_->PostDelayedTask(
         FROM_HERE,
         base::Bind(&DeviceManagementService::StartJobAfterDelay,
                    weak_ptr_factory_.GetWeakPtr(), job->GetWeakPtr()),
         base::TimeDelta::FromMilliseconds(delay));
   } else {
     std::string data;
     source->GetResponseAsString(&data);
@@ skipping 20 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