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

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

Index: components/policy/core/common/cloud/device_management_service.cc
diff --git a/components/policy/core/common/cloud/device_management_service.cc b/components/policy/core/common/cloud/device_management_service.cc
index 7061e6f3661bff2270b26f61593b287d5ddb3c05..b303b8133310c81bef6f8c35769fd01f70346af2 100644
--- a/components/policy/core/common/cloud/device_management_service.cc
+++ b/components/policy/core/common/cloud/device_management_service.cc
@@ -176,10 +176,12 @@ class DeviceManagementRequestJobImpl : public DeviceManagementRequestJob {
   // 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();
@@ -349,27 +351,28 @@ void DeviceManagementRequestJobImpl::ConfigureRequest(
   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() {
@@ -407,7 +410,8 @@ em::DeviceManagementRequest* DeviceManagementRequestJob::GetRequest() {
 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);
@@ -542,9 +546,15 @@ void DeviceManagementService::OnURLFetchComplete(
   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(