Added retry policy

chrome/browser/chromeos/login/enrollment/enrollment_screen.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 "chrome/browser/chromeos/login/enrollment/enrollment_screen.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
@@ skipping 48 matching lines @@
 // static
 EnrollmentScreen* EnrollmentScreen::Get(ScreenManager* manager) {
   return static_cast<EnrollmentScreen*>(
       manager->GetScreen(WizardController::kEnrollmentScreenName));
 }
 
 EnrollmentScreen::EnrollmentScreen(BaseScreenDelegate* base_screen_delegate,
                                    EnrollmentScreenActor* actor)
     : BaseScreen(base_screen_delegate),
       actor_(actor),
-      weak_ptr_factory_(this) {}
+      retry_backoff_(&retry_policy_),

[xiyuan, 2016/11/28 22:16:34]

Can we use a unique_ptr and create |retry_backoff_| after policy is filled up,
similar to [1] ?

[1]
https://cs.chromium.org/chromium/src/chromeos/network/portal_detector/network...

[kumarniranjan, 2016/12/05 22:04:53]

Done.

+      num_retries_(0),

[xiyuan, 2016/11/28 22:16:34]

nit: move the initializer to header

https://chromium-cpp.appspot.com/, check out "Non-Static Class Member
Initializers" row.

[kumarniranjan, 2016/12/05 22:04:52]

Done.

+      weak_ptr_factory_(this) {
+  // It is okay to initialize the fields of retry_policy_ after constructing
+  // retry_backoff_ with a pointer to this struct.
+  retry_policy_.num_errors_to_ignore = 0;
+  retry_policy_.initial_delay_ms = 4000;
+  retry_policy_.multiply_factor = 1.5;
+  retry_policy_.jitter_factor = 0.1;
+  retry_policy_.maximum_backoff_ms = 480000;

[The one and only Dr. Crash, 2016/11/30 23:30:08]

You set a maximum backoff but you never call ShouldRejectRequest() so what
effect does that backoff time have? Is that the maximum interval between two
attempts at the very worst? Also maybe interesting to see how different your
policy is from, say the one for chromoting:
https://cs.chromium.org/chromium/src/remoting/host/chromoting_host.cc?rcl=148...
 

How did you come up w/ the different numbers?

[kumarniranjan, 2016/12/05 22:04:52]

Jonathan wanted the maximum interval between retry attempts to be 8 min (480000
ms). He also wanted the multiplication factor to be 1.5. I came up with the 4 s
initial delay. It works well when I try it on my device. If you think I should
change these numbers, please let me know.

[joth__google, 2016/12/05 22:59:26]

I gave a bit of reasoning in https://codereview.chromium.org/2526973002/#msg4
but basically:
- x2 multiple is reasonable for network requests but for local operatoin I like
the retry not to back quiet so quick as we expect it to normally be done within
the initial delay, and if it wasn't then waiting a further 2x initial delay is a
high penalty.

- 8mins max retry interval is kind of arbitrary, but if the failure is due to
network reception (likely case) then it's actually cheap to keep the retry bound
relatively low (effectively each retry is a fail fast no-op) and connecting to a
network (when it comes in range) may take ~5mins so a retry  rate in this
vicinity feels about right.
If we let it back out to taking hours (or days) between retries, they become
worthless.

+  retry_policy_.entry_lifetime_ms = -1;
+  retry_policy_.always_use_initial_delay = true;
+}
 
 EnrollmentScreen::~EnrollmentScreen() {
   DCHECK(!enrollment_helper_ || g_browser_process->IsShuttingDown());
 }
 
 void EnrollmentScreen::SetParameters(
     const policy::EnrollmentConfig& enrollment_config,
     pairing_chromeos::ControllerPairingController* shark_controller) {
   enrollment_config_ = enrollment_config;
   switch (enrollment_config_.auth_mechanism) {
@@ skipping 111 matching lines @@
   // TODO(rsorokin): Move ShowAdJoin after STEP_REGISTRATION
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           chromeos::switches::kEnableAd)) {
     actor_->ShowAdJoin();
   } else {
     OnAdJoined("");
   }
 }
 
 void EnrollmentScreen::OnRetry() {
+  num_retries_++;

[xiyuan, 2016/11/28 22:16:34]

nit: ++num_retries_;

[The one and only Dr. Crash, 2016/11/30 23:09:56]

Yes! (Sorry, I am a big fan of pre-increment/decrement).

[kumarniranjan, 2016/12/05 22:04:52]

Done.

+  LOG(WARNING) << "Enrollment retry " << num_retries_
+               << " initiated by user button press.";
+  ProcessRetry();

[xiyuan, 2016/11/28 22:16:34]

retry_timer_.Stop() in case the timer is running?

[kumarniranjan, 2016/12/05 22:04:52]

Good catch! We don't want an automatic retry to happen while we are processing
the user's retry request.

+}
+
+void EnrollmentScreen::AutomaticRetry() {

[xiyuan, 2016/11/28 22:16:34]

nit: AutomaticRetry -> StartRetryTimer

+  retry_backoff_.InformOfRequest(false);
+  retry_timer_.Start(FROM_HERE, retry_backoff_.GetTimeUntilRelease(),
+                     base::Bind(&EnrollmentScreen::ProcessRetry,
+                                weak_ptr_factory_.GetWeakPtr()));
+  num_retries_++;

[xiyuan, 2016/11/28 22:16:34]

nit: ditto

[kumarniranjan, 2016/12/05 22:04:52]

Done.

+  LOG(WARNING) << "Enrollment retry " << num_retries_ << " initiated by timer.";

[xiyuan, 2016/11/28 22:16:34]

What is the purpose of |num_retries_| other than output in the log? Should we
have an upper limit of the retry?

[kumarniranjan, 2016/12/05 22:04:52]

It gives us the capability to set an upper limit on the number of retries in the
future. This number will probably be quite large. Since there is no UI and the
user's only recourse in the event of the problem is to call tech support, it
makes sense to retry every 8 minutes for multiple days. The num_retries_
variable is also very useful for testing.

+}
+
+void EnrollmentScreen::ProcessRetry() {
   Show();
 }
 
 void EnrollmentScreen::OnCancel() {
   if (AdvanceToNextAuth()) {
     Show();
     return;
   }
 
   UMA(policy::kMetricEnrollmentCancelled);
@@ skipping 33 matching lines @@
 void EnrollmentScreen::OnEnrollmentError(policy::EnrollmentStatus status) {
   // TODO(pbond): remove this LOG once http://crbug.com/586961 is fixed.
   LOG(WARNING) << "Enrollment error occured: status=" << status.status()
                << " http status=" << status.http_status()
                << " DM status=" << status.client_status();
   RecordEnrollmentErrorMetrics();
   // If the DM server does not have a device pre-provisioned for attestation-
   // based enrollment and we have a fallback authentication, show it.
   if (status.status() == policy::EnrollmentStatus::STATUS_REGISTRATION_FAILED &&
       status.client_status() == policy::DM_STATUS_SERVICE_DEVICE_NOT_FOUND &&
-      current_auth_ == AUTH_ATTESTATION && AdvanceToNextAuth())
+      current_auth_ == AUTH_ATTESTATION && AdvanceToNextAuth()) {
     Show();
-  else
+  } else {
     actor_->ShowEnrollmentStatus(status);
+    if (policy::DeviceCloudPolicyManagerChromeOS::
+            GetZeroTouchEnrollmentMode() ==
+        policy::ZeroTouchEnrollmentMode::HANDS_OFF)

[xiyuan, 2016/11/28 22:16:34]

nit: wrap the condition into a helper function (e.g. ShouldAutoRetryOnError)
since it is used in two places.

+      AutomaticRetry();

[xiyuan, 2016/11/28 22:16:34]

nit: wrap with {} since condition takes more than 1 line

[The one and only Dr. Crash, 2016/11/30 23:09:56]

Probably unnecessary once a helper method is available.

+  }
 }
 
 void EnrollmentScreen::OnOtherError(
     EnterpriseEnrollmentHelper::OtherError error) {
   RecordEnrollmentErrorMetrics();
   actor_->ShowOtherError(error);
+  if (policy::DeviceCloudPolicyManagerChromeOS::GetZeroTouchEnrollmentMode() ==
+      policy::ZeroTouchEnrollmentMode::HANDS_OFF)
+    AutomaticRetry();

[xiyuan, 2016/11/28 22:16:33]

nit: wrap with {}

[The one and only Dr. Crash, 2016/11/30 23:09:56]

Probably unnecessary once a helper method is available.

 }
 
 void EnrollmentScreen::OnDeviceEnrolled(const std::string& additional_token) {
   // TODO(pbond): remove this LOG once http://crbug.com/586961 is fixed.
   LOG(WARNING) << "Device is successfully enrolled.";
   if (!additional_token.empty())
     SendEnrollmentAuthToken(additional_token);
 
   enrollment_helper_->GetDeviceAttributeUpdatePermission();
 }
@@ skipping 50 matching lines @@
   std::string location = policy ? policy->annotated_location() : std::string();
   actor_->ShowAttributePromptScreen(asset_id, location);
 }
 
 void EnrollmentScreen::SendEnrollmentAuthToken(const std::string& token) {
   DCHECK(shark_controller_);
   shark_controller_->OnAuthenticationDone(enrolling_user_domain_, token);
 }
 
 void EnrollmentScreen::ShowEnrollmentStatusOnSuccess() {
+  retry_backoff_.InformOfRequest(true);
   if (elapsed_timer_)
     UMA_ENROLLMENT_TIME(kMetricEnrollmentTimeSuccess, elapsed_timer_);
   actor_->ShowEnrollmentStatus(policy::EnrollmentStatus::ForStatus(
       policy::EnrollmentStatus::STATUS_SUCCESS));
 }
 
 void EnrollmentScreen::UMA(policy::MetricEnrollment sample) {
   EnrollmentUMA(sample, config_.mode);
 }
 
 void EnrollmentScreen::ShowSigninScreen() {
   actor_->Show();
   actor_->ShowSigninScreen();
 }
 
 void EnrollmentScreen::RecordEnrollmentErrorMetrics() {
   enrollment_failed_once_ = true;
   //  TODO(drcrash): Maybe create multiple metrics (http://crbug.com/640313)?
   if (elapsed_timer_)
     UMA_ENROLLMENT_TIME(kMetricEnrollmentTimeFailure, elapsed_timer_);
 }
 
 }  // namespace chromeos