Pass retry_task to SyncScheduler::ScheduleConfiguration

Pass retry_task through ConfigurationParams to
SyncScheduler::ScheduleConfiguration. We might in the future do
configuration asynchronously and passing retry_task through asynchronous
calls will be handy.

SyncManagerTestWithMockScheduler.ConfigurationRetry is no longer needed,
SyncSchedulerTest.ConfigWithBackingOff tests this functionality.

BUG=259913

Review URL: https://codereview.chromium.org/62283011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@235541 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 98 insertions, 58 deletions

diff --git a/sync/engine/sync_scheduler.h b/sync/engine/sync_scheduler.h
index b31af82..c613ec9 100644
--- a/sync/engine/sync_scheduler.h
+++ b/sync/engine/sync_scheduler.h
@@ -30,7 +30,8 @@ struct SYNC_EXPORT_PRIVATE ConfigurationParams {
       const sync_pb::GetUpdatesCallerInfo::GetUpdatesSource& source,
       ModelTypeSet types_to_download,
       const ModelSafeRoutingInfo& routing_info,
-      const base::Closure& ready_task);
+      const base::Closure& ready_task,
+      const base::Closure& retry_task);
   ~ConfigurationParams();
 
   // Source for the configuration.
@@ -41,6 +42,8 @@ struct SYNC_EXPORT_PRIVATE ConfigurationParams {
   ModelSafeRoutingInfo routing_info;
   // Callback to invoke on configuration completion.
   base::Closure ready_task;
+  // Callback to invoke on configuration failure.
+  base::Closure retry_task;
 };
 
 class SYNC_EXPORT_PRIVATE SyncScheduler
@@ -71,9 +74,11 @@ class SYNC_EXPORT_PRIVATE SyncScheduler
   // Schedules the configuration task specified by |params|. Returns true if
   // the configuration task executed immediately, false if it had to be
   // scheduled for a later attempt. |params.ready_task| is invoked whenever the
-  // configuration task executes.
+  // configuration task executes. |params.retry_task| is invoked once if the
+  // configuration task could not execute. |params.ready_task| will still be
+  // called when configuration finishes.
   // Note: must already be in CONFIGURATION mode.
-  virtual bool ScheduleConfiguration(const ConfigurationParams& params) = 0;
+  virtual void ScheduleConfiguration(const ConfigurationParams& params) = 0;
 
   // Request that the syncer avoid starting any new tasks and prepare for
   // shutdown.
diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index 3cb5619..5906995 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -73,12 +73,15 @@ ConfigurationParams::ConfigurationParams(
     const sync_pb::GetUpdatesCallerInfo::GetUpdatesSource& source,
     ModelTypeSet types_to_download,
     const ModelSafeRoutingInfo& routing_info,
-    const base::Closure& ready_task)
+    const base::Closure& ready_task,
+    const base::Closure& retry_task)
     : source(source),
       types_to_download(types_to_download),
       routing_info(routing_info),
-      ready_task(ready_task) {
+      ready_task(ready_task),
+      retry_task(retry_task) {
   DCHECK(!ready_task.is_null());
+  DCHECK(!retry_task.is_null());
 }
 ConfigurationParams::~ConfigurationParams() {}
 
@@ -274,7 +277,7 @@ void BuildModelSafeParams(
 
 }  // namespace.
 
-bool SyncSchedulerImpl::ScheduleConfiguration(
+void SyncSchedulerImpl::ScheduleConfiguration(
     const ConfigurationParams& params) {
   DCHECK(CalledOnValidThread());
   DCHECK(IsConfigRelatedUpdateSourceValue(params.source));
@@ -296,22 +299,11 @@ bool SyncSchedulerImpl::ScheduleConfiguration(
   // Only reconfigure if we have types to download.
   if (!params.types_to_download.Empty()) {
     pending_configure_params_.reset(new ConfigurationParams(params));
-    bool succeeded = DoConfigurationSyncSessionJob(NORMAL_PRIORITY);
-
-    // If we failed, the job would have been saved as the pending configure
-    // job and a wait interval would have been set.
-    if (!succeeded) {
-      DCHECK(pending_configure_params_);
-    } else {
-      DCHECK(!pending_configure_params_);
-    }
-    return succeeded;
+    DoConfigurationSyncSessionJob(NORMAL_PRIORITY);
   } else {
     SDVLOG(2) << "No change in routing info, calling ready task directly.";
     params.ready_task.Run();
   }
-
-  return true;
 }
 
 bool SyncSchedulerImpl::CanRunJobNow(JobPriority priority) {
@@ -497,13 +489,18 @@ void SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) {
   }
 }
 
-bool SyncSchedulerImpl::DoConfigurationSyncSessionJob(JobPriority priority) {
+void SyncSchedulerImpl::DoConfigurationSyncSessionJob(JobPriority priority) {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(mode_, CONFIGURATION_MODE);
+  DCHECK(pending_configure_params_ != NULL);
 
   if (!CanRunJobNow(priority)) {
     SDVLOG(2) << "Unable to run configure job right now.";
-    return false;
+    if (!pending_configure_params_->retry_task.is_null()) {
+      pending_configure_params_->retry_task.Run();
+      pending_configure_params_->retry_task.Reset();
+    }
+    return;
   }
 
   SDVLOG(2) << "Will run configure SyncShare with routes "
@@ -529,10 +526,12 @@ bool SyncSchedulerImpl::DoConfigurationSyncSessionJob(JobPriority priority) {
     // If we're here, then we successfully reached the server.  End all backoff.
     wait_interval_.reset();
     NotifyRetryTime(base::Time());
-    return true;
   } else {
     HandleFailure(session->status_controller().model_neutral_state());
-    return false;
+    if (!pending_configure_params_->retry_task.is_null()) {
+      pending_configure_params_->retry_task.Run();
+      pending_configure_params_->retry_task.Reset();
+    }
   }
 }
 
diff --git a/sync/engine/sync_scheduler_impl.h b/sync/engine/sync_scheduler_impl.h
index 8cb5af7..13ef4db 100644
--- a/sync/engine/sync_scheduler_impl.h
+++ b/sync/engine/sync_scheduler_impl.h
@@ -52,7 +52,7 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
   virtual ~SyncSchedulerImpl();
 
   virtual void Start(Mode mode) OVERRIDE;
-  virtual bool ScheduleConfiguration(
+  virtual void ScheduleConfiguration(
       const ConfigurationParams& params) OVERRIDE;
   virtual void Stop() OVERRIDE;
   virtual void ScheduleLocalNudge(
@@ -159,7 +159,7 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
   void DoNudgeSyncSessionJob(JobPriority priority);
 
   // Invoke the syncer to perform a configuration job.
-  bool DoConfigurationSyncSessionJob(JobPriority priority);
+  void DoConfigurationSyncSessionJob(JobPriority priority);
 
   // Helper function for Do{Nudge,Configuration}SyncSessionJob.
   void HandleFailure(
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index 08e919a..56ac356 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -307,14 +307,17 @@ TEST_F(SyncSchedulerTest, Config) {
 
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  CallbackCounter counter;
+  CallbackCounter ready_counter;
+  CallbackCounter retry_counter;
   ConfigurationParams params(
       GetUpdatesCallerInfo::RECONFIGURATION,
       model_types,
       TypesToRoutingInfo(model_types),
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  ASSERT_TRUE(scheduler()->ScheduleConfiguration(params));
-  ASSERT_EQ(1, counter.times_called());
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
+  scheduler()->ScheduleConfiguration(params);
+  ASSERT_EQ(1, ready_counter.times_called());
+  ASSERT_EQ(0, retry_counter.times_called());
 }
 
 // Simulate a failure and make sure the config request is retried.
@@ -329,16 +332,27 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) {
 
   EXPECT_CALL(*syncer(), ConfigureSyncShare(_,_,_))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateConfigureFailed),
+                      RecordSyncShare(&times)))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateConfigureFailed),
                       RecordSyncShare(&times)));
 
-  CallbackCounter counter;
+  CallbackCounter ready_counter;
+  CallbackCounter retry_counter;
   ConfigurationParams params(
       GetUpdatesCallerInfo::RECONFIGURATION,
       model_types,
       TypesToRoutingInfo(model_types),
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  ASSERT_FALSE(scheduler()->ScheduleConfiguration(params));
-  ASSERT_EQ(0, counter.times_called());
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
+  scheduler()->ScheduleConfiguration(params);
+  ASSERT_EQ(0, ready_counter.times_called());
+  ASSERT_EQ(1, retry_counter.times_called());
+
+  // RunLoop() will trigger TryCanaryJob which will retry configuration.
+  // Since retry_task was already called it shouldn't be called again.
+  RunLoop();
+  ASSERT_EQ(0, ready_counter.times_called());
+  ASSERT_EQ(1, retry_counter.times_called());
 
   Mock::VerifyAndClearExpectations(syncer());
 
@@ -347,7 +361,7 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) {
                       RecordSyncShare(&times)));
   RunLoop();
 
-  ASSERT_EQ(1, counter.times_called());
+  ASSERT_EQ(1, ready_counter.times_called());
 }
 
 // Issue a nudge when the config has failed. Make sure both the config and
@@ -365,14 +379,17 @@ TEST_F(SyncSchedulerTest, NudgeWithConfigWithBackingOff) {
   EXPECT_CALL(*syncer(), ConfigureSyncShare(_,_,_))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateConfigureFailed),
                       RecordSyncShare(&times)));
-  CallbackCounter counter;
+  CallbackCounter ready_counter;
+  CallbackCounter retry_counter;
   ConfigurationParams params(
       GetUpdatesCallerInfo::RECONFIGURATION,
       model_types,
       TypesToRoutingInfo(model_types),
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  ASSERT_FALSE(scheduler()->ScheduleConfiguration(params));
-  ASSERT_EQ(0, counter.times_called());
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
+  scheduler()->ScheduleConfiguration(params);
+  ASSERT_EQ(0, ready_counter.times_called());
+  ASSERT_EQ(1, retry_counter.times_called());
   Mock::VerifyAndClearExpectations(syncer());
 
   // Ask for a nudge while dealing with repeated configure failure.
@@ -385,7 +402,7 @@ TEST_F(SyncSchedulerTest, NudgeWithConfigWithBackingOff) {
   // for the first retry attempt from the config job (after
   // waiting ~+/- 50ms).
   Mock::VerifyAndClearExpectations(syncer());
-  ASSERT_EQ(0, counter.times_called());
+  ASSERT_EQ(0, ready_counter.times_called());
 
   // Let the next configure retry succeed.
   EXPECT_CALL(*syncer(), ConfigureSyncShare(_,_,_))
@@ -599,14 +616,18 @@ TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) {
 
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  CallbackCounter counter;
+  CallbackCounter ready_counter;
+  CallbackCounter retry_counter;
   ConfigurationParams params(
       GetUpdatesCallerInfo::RECONFIGURATION,
       types,
       TypesToRoutingInfo(types),
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  ASSERT_FALSE(scheduler()->ScheduleConfiguration(params));
-  ASSERT_EQ(0, counter.times_called());
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
+  scheduler()->ScheduleConfiguration(params);
+  ASSERT_EQ(0, ready_counter.times_called());
+  ASSERT_EQ(1, retry_counter.times_called());
+
 }
 
 TEST_F(SyncSchedulerTest, ThrottlingExpiresFromPoll) {
@@ -682,14 +703,17 @@ TEST_F(SyncSchedulerTest, ThrottlingExpiresFromConfigure) {
   const ModelTypeSet types(BOOKMARKS);
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  CallbackCounter counter;
+  CallbackCounter ready_counter;
+  CallbackCounter retry_counter;
   ConfigurationParams params(
       GetUpdatesCallerInfo::RECONFIGURATION,
       types,
       TypesToRoutingInfo(types),
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  EXPECT_FALSE(scheduler()->ScheduleConfiguration(params));
-  EXPECT_EQ(0, counter.times_called());
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
+  scheduler()->ScheduleConfiguration(params);
+  EXPECT_EQ(0, ready_counter.times_called());
+  EXPECT_EQ(1, retry_counter.times_called());
   EXPECT_TRUE(scheduler()->IsCurrentlyThrottled());
 
   RunLoop();
@@ -797,14 +821,18 @@ TEST_F(SyncSchedulerTest, ConfigurationMode) {
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateConfigureSuccess),
                       RecordSyncShare(&times)))
       .RetiresOnSaturation();
-  CallbackCounter counter;
+  CallbackCounter ready_counter;
+  CallbackCounter retry_counter;
   ConfigurationParams params(
       GetUpdatesCallerInfo::RECONFIGURATION,
       config_types,
       TypesToRoutingInfo(config_types),
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  ASSERT_TRUE(scheduler()->ScheduleConfiguration(params));
-  ASSERT_EQ(1, counter.times_called());
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
+  scheduler()->ScheduleConfiguration(params);
+  ASSERT_EQ(1, ready_counter.times_called());
+  ASSERT_EQ(0, retry_counter.times_called());
+
   Mock::VerifyAndClearExpectations(syncer());
 
   // Switch to NORMAL_MODE to ensure NUDGES were properly saved and run.
@@ -895,12 +923,14 @@ TEST_F(BackoffTriggersSyncSchedulerTest, FailGetEncryptionKey) {
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
   ModelTypeSet types(BOOKMARKS);
-  CallbackCounter counter;
+  CallbackCounter ready_counter;
+  CallbackCounter retry_counter;
   ConfigurationParams params(
       GetUpdatesCallerInfo::RECONFIGURATION,
       types,
       TypesToRoutingInfo(types),
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
   scheduler()->ScheduleConfiguration(params);
   RunLoop();
 
@@ -947,14 +977,18 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) {
 
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  CallbackCounter counter;
+  CallbackCounter ready_counter;
+  CallbackCounter retry_counter;
   ConfigurationParams params(
       GetUpdatesCallerInfo::RECONFIGURATION,
       types,
       TypesToRoutingInfo(types),
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  ASSERT_FALSE(scheduler()->ScheduleConfiguration(params));
-  ASSERT_EQ(0, counter.times_called());
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
+  scheduler()->ScheduleConfiguration(params);
+  ASSERT_EQ(0, ready_counter.times_called());
+  ASSERT_EQ(1, retry_counter.times_called());
+
 }
 
 // Test that backoff is shaping traffic properly with consecutive errors.
@@ -1168,12 +1202,14 @@ TEST_F(SyncSchedulerTest, DoubleCanaryInConfigure) {
   connection()->UpdateConnectionStatus();
 
   ModelTypeSet model_types(BOOKMARKS);
-  CallbackCounter counter;
+  CallbackCounter ready_counter;
+  CallbackCounter retry_counter;
   ConfigurationParams params(
       GetUpdatesCallerInfo::RECONFIGURATION,
       model_types,
       TypesToRoutingInfo(model_types),
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
   scheduler()->ScheduleConfiguration(params);
 
   scheduler()->OnConnectionStatusChange();