diff options
Diffstat (limited to 'sync/engine')
| -rw-r--r-- | sync/engine/sync_scheduler.cc | 255 | ||||
| -rw-r--r-- | sync/engine/sync_scheduler.h | 49 | ||||
| -rw-r--r-- | sync/engine/sync_scheduler_unittest.cc | 192 | ||||
| -rw-r--r-- | sync/engine/sync_scheduler_whitebox_unittest.cc | 2 |
4 files changed, 317 insertions, 181 deletions
diff --git a/sync/engine/sync_scheduler.cc b/sync/engine/sync_scheduler.cc index f798d67..e745d19 100644 --- a/sync/engine/sync_scheduler.cc +++ b/sync/engine/sync_scheduler.cc @@ -68,6 +68,24 @@ bool IsActionableError( } } // namespace +ConfigurationParams::ConfigurationParams() + : source(GetUpdatesCallerInfo::UNKNOWN), + keystore_key_status(KEYSTORE_KEY_UNNECESSARY) {} +ConfigurationParams::ConfigurationParams( + const sync_pb::GetUpdatesCallerInfo::GetUpdatesSource& source, + const syncable::ModelTypeSet& types_to_download, + const browser_sync::ModelSafeRoutingInfo& routing_info, + KeystoreKeyStatus keystore_key_status, + const base::Closure& ready_task) + : source(source), + types_to_download(types_to_download), + routing_info(routing_info), + keystore_key_status(keystore_key_status), + ready_task(ready_task) { + DCHECK(!ready_task.is_null()); +} +ConfigurationParams::~ConfigurationParams() {} + SyncScheduler::DelayProvider::DelayProvider() {} SyncScheduler::DelayProvider::~DelayProvider() {} @@ -99,12 +117,16 @@ SyncScheduler::SyncSessionJob::~SyncSessionJob() {} SyncScheduler::SyncSessionJob::SyncSessionJob(SyncSessionJobPurpose purpose, base::TimeTicks start, - linked_ptr<sessions::SyncSession> session, bool is_canary_job, - const tracked_objects::Location& from_here) : purpose(purpose), - scheduled_start(start), - session(session), - is_canary_job(is_canary_job), - from_here(from_here) { + linked_ptr<sessions::SyncSession> session, + bool is_canary_job, + ConfigurationParams config_params, + const tracked_objects::Location& from_here) + : purpose(purpose), + scheduled_start(start), + session(session), + is_canary_job(is_canary_job), + config_params(config_params), + from_here(from_here) { } const char* SyncScheduler::SyncSessionJob::GetPurposeString( @@ -247,7 +269,7 @@ void SyncScheduler::UpdateServerConnectionManagerStatus( connection_code_ = code; } -void SyncScheduler::Start(Mode mode, const base::Closure& callback) { +void SyncScheduler::Start(Mode mode) { DCHECK_EQ(MessageLoop::current(), sync_loop_); std::string thread_name = MessageLoop::current()->thread_name(); if (thread_name.empty()) @@ -264,8 +286,6 @@ void SyncScheduler::Start(Mode mode, const base::Closure& callback) { Mode old_mode = mode_; mode_ = mode; AdjustPolling(NULL); // Will kick start poll timer if needed. - if (!callback.is_null()) - callback.Run(); if (old_mode != mode_) { // We just changed our mode. See if there are any pending jobs that we could @@ -284,6 +304,112 @@ void SyncScheduler::SendInitialSnapshot() { session_context_->NotifyListeners(event); } +namespace { + +// Helper to extract the routing info and workers corresponding to types in +// |types| from |current_routes| and |current_workers|. +void BuildModelSafeParams( + const ModelTypeSet& types_to_download, + const ModelSafeRoutingInfo& current_routes, + const std::vector<ModelSafeWorker*>& current_workers, + ModelSafeRoutingInfo* result_routes, + std::vector<ModelSafeWorker*>* result_workers) { + std::set<ModelSafeGroup> active_groups; + active_groups.insert(GROUP_PASSIVE); + for (ModelTypeSet::Iterator iter = types_to_download.First(); iter.Good(); + iter.Inc()) { + syncable::ModelType type = iter.Get(); + ModelSafeRoutingInfo::const_iterator route = current_routes.find(type); + DCHECK(route != current_routes.end()); + ModelSafeGroup group = route->second; + (*result_routes)[type] = group; + active_groups.insert(group); + } + + for(std::vector<ModelSafeWorker*>::const_iterator iter = + current_workers.begin(); iter != current_workers.end(); ++iter) { + if (active_groups.count((*iter)->GetModelSafeGroup()) > 0) + result_workers->push_back(*iter); + } +} + +} // namespace. + +bool SyncScheduler::ScheduleConfiguration(const ConfigurationParams& params) { + DCHECK_EQ(MessageLoop::current(), sync_loop_); + DCHECK(IsConfigRelatedUpdateSourceValue(params.source)); + DCHECK_EQ(CONFIGURATION_MODE, mode_); + DCHECK(!params.ready_task.is_null()); + SDVLOG(2) << "Reconfiguring syncer."; + + // Only one configuration is allowed at a time. Verify we're not waiting + // for a pending configure job. + DCHECK(!wait_interval_.get() || !wait_interval_->pending_configure_job.get()); + + // TODO(sync): now that ModelChanging commands only use those workers within + // the routing info, we don't really need |restricted_workers|. Remove it. + // crbug.com/133030 + browser_sync::ModelSafeRoutingInfo restricted_routes; + std::vector<ModelSafeWorker*> restricted_workers; + BuildModelSafeParams(params.types_to_download, + params.routing_info, + session_context_->workers(), + &restricted_routes, + &restricted_workers); + session_context_->set_routing_info(params.routing_info); + + // We rely on this not failing, so don't need to worry about checking for + // success. In addition, this will be removed as part of crbug.com/131433. + SyncSessionJob cleanup_job( + SyncSessionJob::CLEANUP_DISABLED_TYPES, + TimeTicks::Now(), + make_linked_ptr(CreateSyncSession(SyncSourceInfo())), + false, + ConfigurationParams(), + FROM_HERE); + DoSyncSessionJob(cleanup_job); + + if (params.keystore_key_status == ConfigurationParams::KEYSTORE_KEY_NEEDED) { + // TODO(zea): implement in such a way that we can handle failures and the + // subsequent retrys the scheduler might perform. See crbug.com/129665. + NOTIMPLEMENTED(); + } + + // Only reconfigure if we have types to download. + if (!params.types_to_download.Empty()) { + DCHECK(!restricted_routes.empty()); + linked_ptr<SyncSession> session(new SyncSession( + session_context_, + this, + SyncSourceInfo(params.source, + syncable::ModelTypePayloadMapFromRoutingInfo( + restricted_routes, + std::string())), + restricted_routes, + restricted_workers)); + SyncSessionJob job(SyncSessionJob::CONFIGURATION, + TimeTicks::Now(), + session, + false, + params, + FROM_HERE); + DoSyncSessionJob(job); + + // If we failed, the job would have been saved as the pending configure + // job and a wait interval would have been set. + if (!session->Succeeded()) { + DCHECK(wait_interval_.get() && + wait_interval_->pending_configure_job.get()); + return false; + } + } else { + SDVLOG(2) << "No change in routing info, calling ready task directly."; + params.ready_task.Run(); + } + + return true; +} + SyncScheduler::JobProcessDecision SyncScheduler::DecideWhileInWaitInterval( const SyncSessionJob& job) { DCHECK_EQ(MessageLoop::current(), sync_loop_); @@ -382,7 +508,8 @@ void SyncScheduler::InitOrCoalescePendingJob(const SyncSessionJob& job) { s->delegate(), s->source(), s->routing_info(), s->workers())); SyncSessionJob new_job(SyncSessionJob::NUDGE, job.scheduled_start, - make_linked_ptr(session.release()), false, job.from_here); + make_linked_ptr(session.release()), false, + ConfigurationParams(), job.from_here); pending_nudge_.reset(new SyncSessionJob(new_job)); return; @@ -428,11 +555,17 @@ void SyncScheduler::SaveJob(const SyncSessionJob& job) { DCHECK(wait_interval_.get()); DCHECK(mode_ == CONFIGURATION_MODE); + // Config params should always get set. + DCHECK(!job.config_params.ready_task.is_null()); SyncSession* old = job.session.get(); SyncSession* s(new SyncSession(session_context_, this, old->source(), old->routing_info(), old->workers())); - SyncSessionJob new_job(job.purpose, TimeTicks::Now(), - make_linked_ptr(s), false, job.from_here); + SyncSessionJob new_job(job.purpose, + TimeTicks::Now(), + make_linked_ptr(s), + false, + job.config_params, + job.from_here); wait_interval_->pending_configure_job.reset(new SyncSessionJob(new_job)); } // drop the rest. // TODO(sync): Is it okay to drop the rest? It's weird that @@ -451,23 +584,16 @@ struct ModelSafeWorkerGroupIs { void SyncScheduler::ClearUserData() { DCHECK_EQ(MessageLoop::current(), sync_loop_); - SyncSessionJob job(SyncSessionJob::CLEAR_USER_DATA, TimeTicks::Now(), + SyncSessionJob job(SyncSessionJob::CLEAR_USER_DATA, + TimeTicks::Now(), make_linked_ptr(CreateSyncSession(SyncSourceInfo())), false, + ConfigurationParams(), FROM_HERE); DoSyncSessionJob(job); } -void SyncScheduler::CleanupDisabledTypes() { - DCHECK_EQ(MessageLoop::current(), sync_loop_); - SyncSessionJob job(SyncSessionJob::CLEANUP_DISABLED_TYPES, TimeTicks::Now(), - make_linked_ptr(CreateSyncSession(SyncSourceInfo())), - false, - FROM_HERE); - DoSyncSessionJob(job); -} - void SyncScheduler::ScheduleNudgeAsync( const TimeDelta& delay, NudgeSource source, ModelTypeSet types, @@ -525,7 +651,7 @@ void SyncScheduler::ScheduleNudgeImpl( SyncSession* session(CreateSyncSession(info)); SyncSessionJob job(SyncSessionJob::NUDGE, TimeTicks::Now() + delay, make_linked_ptr(session), is_canary_job, - nudge_location); + ConfigurationParams(), nudge_location); session = NULL; if (!ShouldRunJob(job)) @@ -556,77 +682,6 @@ void SyncScheduler::ScheduleNudgeImpl( ScheduleSyncSessionJob(job); } -// Helper to extract the routing info and workers corresponding to types in -// |types| from |current_routes| and |current_workers|. -void GetModelSafeParamsForTypes(ModelTypeSet types, - const ModelSafeRoutingInfo& current_routes, - const std::vector<ModelSafeWorker*>& current_workers, - ModelSafeRoutingInfo* result_routes, - std::vector<ModelSafeWorker*>* result_workers) { - bool passive_group_added = false; - - typedef std::vector<ModelSafeWorker*>::const_iterator iter; - for (ModelTypeSet::Iterator it = types.First(); - it.Good(); it.Inc()) { - const syncable::ModelType t = it.Get(); - ModelSafeRoutingInfo::const_iterator route = current_routes.find(t); - DCHECK(route != current_routes.end()); - ModelSafeGroup group = route->second; - - (*result_routes)[t] = group; - iter w_tmp_it = std::find_if(current_workers.begin(), current_workers.end(), - ModelSafeWorkerGroupIs(group)); - if (w_tmp_it != current_workers.end()) { - iter result_workers_it = std::find_if( - result_workers->begin(), result_workers->end(), - ModelSafeWorkerGroupIs(group)); - if (result_workers_it == result_workers->end()) - result_workers->push_back(*w_tmp_it); - - if (group == GROUP_PASSIVE) - passive_group_added = true; - } else { - NOTREACHED(); - } - } - - // Always add group passive. - if (passive_group_added == false) { - iter it = std::find_if(current_workers.begin(), current_workers.end(), - ModelSafeWorkerGroupIs(GROUP_PASSIVE)); - if (it != current_workers.end()) - result_workers->push_back(*it); - else - NOTREACHED(); - } -} - -void SyncScheduler::ScheduleConfiguration( - ModelTypeSet types, - GetUpdatesCallerInfo::GetUpdatesSource source) { - DCHECK_EQ(MessageLoop::current(), sync_loop_); - DCHECK(IsConfigRelatedUpdateSourceValue(source)); - SDVLOG(2) << "Scheduling a config"; - - ModelSafeRoutingInfo routes; - std::vector<ModelSafeWorker*> workers; - GetModelSafeParamsForTypes(types, - session_context_->routing_info(), - session_context_->workers(), - &routes, &workers); - - SyncSession* session = new SyncSession(session_context_, this, - SyncSourceInfo(source, - syncable::ModelTypePayloadMapFromRoutingInfo( - routes, std::string())), - routes, workers); - SyncSessionJob job(SyncSessionJob::CONFIGURATION, TimeTicks::Now(), - make_linked_ptr(session), - false, - FROM_HERE); - DoSyncSessionJob(job); -} - const char* SyncScheduler::GetModeString(SyncScheduler::Mode mode) { switch (mode) { ENUM_CASE(CONFIGURATION_MODE); @@ -829,6 +884,11 @@ void SyncScheduler::FinishSyncSessionJob(const SyncSessionJob& job) { // here; see DCHECKs in SaveJob(). (See http://crbug.com/90868.) SaveJob(job); return; // Nothing to do. + } else if (job.session->Succeeded() && + !job.config_params.ready_task.is_null()) { + // If this was a configuration job with a ready task, invoke it now that + // we finished successfully. + job.config_params.ready_task.Run(); } SDVLOG(2) << "Updating the next polling time after SyncMain"; @@ -931,11 +991,15 @@ void SyncScheduler::HandleContinuationError( wait_interval_.reset(new WaitInterval(WaitInterval::EXPONENTIAL_BACKOFF, length)); if (old_job.purpose == SyncSessionJob::CONFIGURATION) { + SDVLOG(2) << "Configuration did not succeed, scheduling retry."; + // Config params should always get set. + DCHECK(!old_job.config_params.ready_task.is_null()); SyncSession* old = old_job.session.get(); SyncSession* s(new SyncSession(session_context_, this, old->source(), old->routing_info(), old->workers())); SyncSessionJob job(old_job.purpose, TimeTicks::Now() + length, - make_linked_ptr(s), false, FROM_HERE); + make_linked_ptr(s), false, old_job.config_params, + FROM_HERE); wait_interval_->pending_configure_job.reset(new SyncSessionJob(job)); } else { // We are not in configuration mode. So wait_interval's pending job @@ -1057,6 +1121,7 @@ void SyncScheduler::PollTimerCallback() { SyncSessionJob job(SyncSessionJob::POLL, TimeTicks::Now(), make_linked_ptr(s), false, + ConfigurationParams(), FROM_HERE); ScheduleSyncSessionJob(job); diff --git a/sync/engine/sync_scheduler.h b/sync/engine/sync_scheduler.h index d32ee25..8ebd2aa 100644 --- a/sync/engine/sync_scheduler.h +++ b/sync/engine/sync_scheduler.h @@ -37,6 +37,32 @@ namespace browser_sync { struct ServerConnectionEvent; +struct ConfigurationParams { + enum KeystoreKeyStatus { + KEYSTORE_KEY_UNNECESSARY, + KEYSTORE_KEY_NEEDED + }; + ConfigurationParams(); + ConfigurationParams( + const sync_pb::GetUpdatesCallerInfo::GetUpdatesSource& source, + const syncable::ModelTypeSet& types_to_download, + const browser_sync::ModelSafeRoutingInfo& routing_info, + KeystoreKeyStatus keystore_key_status, + const base::Closure& ready_task); + ~ConfigurationParams(); + + // Source for the configuration. + sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source; + // The types that should be downloaded. + syncable::ModelTypeSet types_to_download; + // The new routing info (superset of types to be downloaded). + ModelSafeRoutingInfo routing_info; + // Whether we need to perform a GetKey command. + KeystoreKeyStatus keystore_key_status; + // Callback to invoke on configuration completion. + base::Closure ready_task; +}; + class SyncScheduler : public sessions::SyncSession::Delegate { public: enum Mode { @@ -63,10 +89,15 @@ class SyncScheduler : public sessions::SyncSession::Delegate { // Start the scheduler with the given mode. If the scheduler is // already started, switch to the given mode, although some - // scheduled tasks from the old mode may still run. If non-NULL, - // |callback| will be invoked when the mode has been changed to - // |mode|. Takes ownership of |callback|. - void Start(Mode mode, const base::Closure& callback); + // scheduled tasks from the old mode may still run. + virtual void Start(Mode mode); + + // 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. + // Note: must already be in CONFIGURATION mode. + virtual bool ScheduleConfiguration(const ConfigurationParams& params); // Request that any running syncer task stop as soon as possible and // cancel all scheduled tasks. This function can be called from any thread, @@ -86,14 +117,6 @@ class SyncScheduler : public sessions::SyncSession::Delegate { const syncable::ModelTypePayloadMap& types_with_payloads, const tracked_objects::Location& nudge_location); - // Schedule a configuration cycle. May execute immediately or at a later time - // (depending on backoff/throttle state). - // Note: The source argument of this function must come from the subset of - // GetUpdatesCallerInfo values related to configurations. - void ScheduleConfiguration( - syncable::ModelTypeSet types, - sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source); - // TODO(tim): remove this. crbug.com/131336 void ClearUserData(); @@ -164,6 +187,7 @@ class SyncScheduler : public sessions::SyncSession::Delegate { SyncSessionJob(); SyncSessionJob(SyncSessionJobPurpose purpose, base::TimeTicks start, linked_ptr<sessions::SyncSession> session, bool is_canary_job, + ConfigurationParams config_params, const tracked_objects::Location& nudge_location); ~SyncSessionJob(); static const char* GetPurposeString(SyncSessionJobPurpose purpose); @@ -172,6 +196,7 @@ class SyncScheduler : public sessions::SyncSession::Delegate { base::TimeTicks scheduled_start; linked_ptr<sessions::SyncSession> session; bool is_canary_job; + ConfigurationParams config_params; // This is the location the job came from. Used for debugging. // In case of multiple nudges getting coalesced this stores the diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc index a6d8059..7aa30ab 100644 --- a/sync/engine/sync_scheduler_unittest.cc +++ b/sync/engine/sync_scheduler_unittest.cc @@ -12,6 +12,7 @@ #include "sync/engine/syncer.h" #include "sync/engine/throttled_data_type_tracker.h" #include "sync/sessions/test_util.h" +#include "sync/test/callback_counter.h" #include "sync/test/engine/fake_model_worker.h" #include "sync/test/engine/mock_connection_manager.h" #include "sync/test/engine/test_directory_setter_upper.h" @@ -27,6 +28,7 @@ using testing::DoAll; using testing::Eq; using testing::Invoke; using testing::Mock; +using testing::Not; using testing::Return; using testing::WithArg; @@ -69,6 +71,14 @@ void PumpLoop() { RunLoop(); } +ModelSafeRoutingInfo TypesToRoutingInfo(const ModelTypeSet& types) { + ModelSafeRoutingInfo routes; + for (ModelTypeSet::Iterator iter = types.First(); iter.Good(); iter.Inc()) { + routes[iter.Get()] = GROUP_PASSIVE; + } + return routes; +} + // Convenient to use in tests wishing to analyze SyncShare calls over time. static const size_t kMinNumSamples = 5; class SyncSchedulerTest : public testing::Test { @@ -152,7 +162,7 @@ class SyncSchedulerTest : public testing::Test { } void StartSyncScheduler(SyncScheduler::Mode mode) { - scheduler()->Start(mode, base::Closure()); + scheduler()->Start(mode); } // This stops the scheduler synchronously. @@ -291,14 +301,23 @@ TEST_F(SyncSchedulerTest, Config) { SyncShareRecords records; const ModelTypeSet model_types(syncable::BOOKMARKS); - EXPECT_CALL(*syncer(), SyncShare(_,_,_)) + EXPECT_CALL(*syncer(), + SyncShare(_,_,_)) + .WillOnce(Invoke(sessions::test_util::SimulateSuccess)) .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess), WithArg<0>(RecordSyncShare(&records)))); StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); - scheduler()->ScheduleConfiguration( - model_types, GetUpdatesCallerInfo::RECONFIGURATION); + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + model_types, + TypesToRoutingInfo(model_types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_TRUE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(1, counter.times_called()); ASSERT_EQ(1U, records.snapshots.size()); EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(model_types, @@ -315,7 +334,9 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) { SyncShareRecords records; const ModelTypeSet model_types(syncable::BOOKMARKS); - EXPECT_CALL(*syncer(), SyncShare(_,_,_)) + EXPECT_CALL(*syncer(), + SyncShare(_,_,_)) + .WillOnce(Invoke(sessions::test_util::SimulateSuccess)) .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed), WithArg<0>(RecordSyncShare(&records)))) .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess), @@ -324,61 +345,27 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) { StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); ASSERT_EQ(0U, records.snapshots.size()); - scheduler()->ScheduleConfiguration( - model_types, GetUpdatesCallerInfo::RECONFIGURATION); + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + model_types, + TypesToRoutingInfo(model_types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_FALSE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(0, counter.times_called()); ASSERT_EQ(1U, records.snapshots.size()); RunLoop(); ASSERT_EQ(2U, records.snapshots.size()); + ASSERT_EQ(1, counter.times_called()); EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(model_types, records.snapshots[1].source().types)); EXPECT_EQ(GetUpdatesCallerInfo::RECONFIGURATION, records.snapshots[1].source().updates_source); } -// Issue 2 config commands. Second one right after the first has failed -// and make sure LATEST is executed. -TEST_F(SyncSchedulerTest, MultipleConfigWithBackingOff) { - const ModelTypeSet - model_types1(syncable::BOOKMARKS), - model_types2(syncable::AUTOFILL); - UseMockDelayProvider(); - EXPECT_CALL(*delay(), GetDelay(_)) - .WillRepeatedly(Return(TimeDelta::FromMilliseconds(30))); - SyncShareRecords records; - - EXPECT_CALL(*syncer(), SyncShare(_,_,_)) - .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed), - WithArg<0>(RecordSyncShare(&records)))) - .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed), - WithArg<0>(RecordSyncShare(&records)))) - .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess), - WithArg<0>(RecordSyncShare(&records)))); - - StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); - - ASSERT_EQ(0U, records.snapshots.size()); - scheduler()->ScheduleConfiguration( - model_types1, GetUpdatesCallerInfo::RECONFIGURATION); - - ASSERT_EQ(1U, records.snapshots.size()); - scheduler()->ScheduleConfiguration( - model_types2, GetUpdatesCallerInfo::RECONFIGURATION); - - // A canary job gets posted when we go into exponential backoff. - RunLoop(); - - ASSERT_EQ(2U, records.snapshots.size()); - RunLoop(); - - ASSERT_EQ(3U, records.snapshots.size()); - EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(model_types2, - records.snapshots[2].source().types)); - EXPECT_EQ(GetUpdatesCallerInfo::RECONFIGURATION, - records.snapshots[2].source().updates_source); -} - // Issue a nudge when the config has failed. Make sure both the config and // nudge are executed. TEST_F(SyncSchedulerTest, NudgeWithConfigWithBackingOff) { @@ -401,19 +388,28 @@ TEST_F(SyncSchedulerTest, NudgeWithConfigWithBackingOff) { StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); ASSERT_EQ(0U, records.snapshots.size()); - scheduler()->ScheduleConfiguration( - model_types, GetUpdatesCallerInfo::RECONFIGURATION); - + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + model_types, + TypesToRoutingInfo(model_types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_FALSE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(0, counter.times_called()); ASSERT_EQ(1U, records.snapshots.size()); + scheduler()->ScheduleNudgeAsync( zero(), NUDGE_SOURCE_LOCAL, model_types, FROM_HERE); RunLoop(); - ASSERT_EQ(2U, records.snapshots.size()); + ASSERT_EQ(0, counter.times_called()); + RunLoop(); + ASSERT_EQ(3U, records.snapshots.size()); + ASSERT_EQ(1, counter.times_called()); // Now change the mode so nudge can execute. - ASSERT_EQ(3U, records.snapshots.size()); StartSyncScheduler(SyncScheduler::NORMAL_MODE); ASSERT_EQ(4U, records.snapshots.size()); @@ -718,13 +714,19 @@ TEST_F(SyncSchedulerTest, HasMoreToSyncThenFails) { EXPECT_TRUE(RunAndGetBackoff()); } -// Test that no syncing occurs when throttled. +// Test that no syncing occurs when throttled (although CleanupDisabledTypes +// is allowed). TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) { const ModelTypeSet types(syncable::BOOKMARKS); TimeDelta poll(TimeDelta::FromMilliseconds(5)); TimeDelta throttle(TimeDelta::FromMinutes(10)); scheduler()->OnReceivedLongPollIntervalUpdate(poll); - EXPECT_CALL(*syncer(), SyncShare(_,_,_)) + + EXPECT_CALL(*syncer(), + SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES)) + .WillOnce(Invoke(sessions::test_util::SimulateSuccess)); + EXPECT_CALL(*syncer(), SyncShare(_,Not(CLEANUP_DISABLED_TYPES), + Not(CLEANUP_DISABLED_TYPES))) .WillOnce(WithArg<0>(sessions::test_util::SimulateThrottled(throttle))) .WillRepeatedly(AddFailureAndQuitLoopNow()); @@ -736,8 +738,15 @@ TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) { StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); - scheduler()->ScheduleConfiguration( - types, GetUpdatesCallerInfo::RECONFIGURATION); + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + types, + TypesToRoutingInfo(types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_FALSE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(0, counter.times_called()); } TEST_F(SyncSchedulerTest, ThrottlingExpires) { @@ -770,6 +779,7 @@ TEST_F(SyncSchedulerTest, ConfigurationMode) { SyncShareRecords records; scheduler()->OnReceivedLongPollIntervalUpdate(poll); EXPECT_CALL(*syncer(), SyncShare(_,_,_)) + .WillOnce(Invoke(sessions::test_util::SimulateSuccess)) .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess), WithArg<0>(RecordSyncShare(&records)))); @@ -783,8 +793,15 @@ TEST_F(SyncSchedulerTest, ConfigurationMode) { const ModelTypeSet config_types(syncable::BOOKMARKS); - scheduler()->ScheduleConfiguration( - config_types, GetUpdatesCallerInfo::RECONFIGURATION); + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + config_types, + TypesToRoutingInfo(config_types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_TRUE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(1, counter.times_called()); ASSERT_EQ(1U, records.snapshots.size()); EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(config_types, @@ -853,7 +870,7 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) { scheduler()->OnReceivedLongPollIntervalUpdate(poll); UseMockDelayProvider(); - EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(1) + EXPECT_CALL(*syncer(), SyncShare(_,_,_)) .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulateCommitFailed), RecordSyncShareMultiple(&r, 1U))); EXPECT_CALL(*delay(), GetDelay(_)). @@ -895,9 +912,15 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) { StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); - scheduler()->CleanupDisabledTypes(); - scheduler()->ScheduleConfiguration( - types, GetUpdatesCallerInfo::RECONFIGURATION); + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + types, + TypesToRoutingInfo(types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_FALSE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(0, counter.times_called()); StartSyncScheduler(SyncScheduler::NORMAL_MODE); @@ -1066,28 +1089,49 @@ TEST_F(SyncSchedulerTest, SyncerSteps) { StopSyncScheduler(); Mock::VerifyAndClearExpectations(syncer()); - // Configuration. + // Configuration (always includes a cleanup disabled types). + EXPECT_CALL(*syncer(), + SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES)) + .WillOnce(Invoke(sessions::test_util::SimulateSuccess)); EXPECT_CALL(*syncer(), SyncShare(_, DOWNLOAD_UPDATES, APPLY_UPDATES)) .WillOnce(Invoke(sessions::test_util::SimulateSuccess)); StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); - scheduler()->ScheduleConfiguration( - ModelTypeSet(), GetUpdatesCallerInfo::RECONFIGURATION); - + syncable::ModelTypeSet model_types(syncable::BOOKMARKS); + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + model_types, + TypesToRoutingInfo(model_types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_TRUE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(1, counter.times_called()); + // Runs directly so no need to pump the loop. StopSyncScheduler(); Mock::VerifyAndClearExpectations(syncer()); - // Cleanup disabled types. + // Cleanup disabled types. Because no types are being configured, we just + // perform the cleanup. EXPECT_CALL(*syncer(), - SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES)) - .WillOnce(Invoke(sessions::test_util::SimulateSuccess)); - StartSyncScheduler(SyncScheduler::NORMAL_MODE); - - scheduler()->CleanupDisabledTypes(); + SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES)). + WillOnce(Invoke(sessions::test_util::SimulateSuccess)); + StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); + CallbackCounter counter2; + ConfigurationParams params2( + GetUpdatesCallerInfo::RECONFIGURATION, + ModelTypeSet(), + ModelSafeRoutingInfo(), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter2))); + ASSERT_TRUE(scheduler()->ScheduleConfiguration(params2)); + ASSERT_EQ(1, counter2.times_called()); StopSyncScheduler(); Mock::VerifyAndClearExpectations(syncer()); + StartSyncScheduler(SyncScheduler::NORMAL_MODE); + // Poll. EXPECT_CALL(*syncer(), SyncShare(_, SYNCER_BEGIN, SYNCER_END)) .Times(AtLeast(1)) diff --git a/sync/engine/sync_scheduler_whitebox_unittest.cc b/sync/engine/sync_scheduler_whitebox_unittest.cc index 34ff7d9..390a6cd 100644 --- a/sync/engine/sync_scheduler_whitebox_unittest.cc +++ b/sync/engine/sync_scheduler_whitebox_unittest.cc @@ -107,6 +107,7 @@ class SyncSchedulerWhiteboxTest : public testing::Test { SyncScheduler::SyncSessionJob job(purpose, TimeTicks::Now(), make_linked_ptr(s), false, + ConfigurationParams(), FROM_HERE); return DecideOnJob(job); } @@ -160,6 +161,7 @@ TEST_F(SyncSchedulerWhiteboxTest, SaveNudgeWhileTypeThrottled) { TimeTicks::Now(), make_linked_ptr(s), false, + ConfigurationParams(), FROM_HERE); SyncScheduler::JobProcessDecision decision = DecideOnJob(job); |