From e3e44e71643ae5d04dc9ea3303c2ae949c2038ab Mon Sep 17 00:00:00 2001 From: "haitaol@chromium.org" Date: Sat, 11 Jan 2014 23:00:43 +0000 Subject: Support GU retry command in sync engine. The command specifies a delay after which syncer should issue a GU to pick up updates missed by last GU. BUG= Review URL: https://codereview.chromium.org/124083002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@244381 0039d316-1c4b-4281-b951-d872f2087c98 --- sync/engine/download.cc | 40 ++++++++ sync/engine/download.h | 15 +++ sync/engine/download_unittest.cc | 37 +++++++ sync/engine/sync_scheduler_impl.cc | 53 +++++++--- sync/engine/sync_scheduler_impl.h | 16 ++- sync/engine/sync_scheduler_unittest.cc | 122 ++++++++++++++++++++--- sync/engine/syncer.cc | 16 ++- sync/engine/syncer.h | 2 + sync/engine/syncer_proto_util.cc | 5 + sync/engine/syncer_unittest.cc | 5 +- sync/protocol/client_commands.proto | 3 + sync/protocol/get_updates_caller_info.proto | 1 + sync/protocol/proto_enum_conversions.cc | 6 +- sync/protocol/proto_enum_conversions_unittest.cc | 2 +- sync/protocol/sync.proto | 6 +- sync/protocol/sync_enums.proto | 8 +- sync/sessions/nudge_tracker.cc | 16 ++- sync/sessions/nudge_tracker.h | 18 +++- sync/sessions/nudge_tracker_unittest.cc | 93 ++++++++++------- sync/sessions/sync_session.h | 3 + sync/sessions/test_util.cc | 17 +++- sync/sessions/test_util.h | 17 +++- sync/test/engine/fake_sync_scheduler.cc | 4 + sync/test/engine/fake_sync_scheduler.h | 2 + 24 files changed, 413 insertions(+), 94 deletions(-) (limited to 'sync') diff --git a/sync/engine/download.cc b/sync/engine/download.cc index 2bc7f7a..448481f 100644 --- a/sync/engine/download.cc +++ b/sync/engine/download.cc @@ -234,6 +234,8 @@ void BuildNormalDownloadUpdatesImpl( // Set the new and improved version of source, too. get_updates->set_get_updates_origin(sync_pb::SyncEnums::GU_TRIGGER); + get_updates->set_is_retry( + nudge_tracker.IsRetryRequired(base::TimeTicks::Now())); // Fill in the notification hints. for (int i = 0; i < get_updates->from_progress_marker_size(); ++i) { @@ -331,6 +333,44 @@ void BuildDownloadUpdatesForPollImpl( get_updates->set_get_updates_origin(sync_pb::SyncEnums::PERIODIC); } +void BuildDownloadUpdatesForRetry( + SyncSession* session, + bool create_mobile_bookmarks_folder, + ModelTypeSet request_types, + sync_pb::ClientToServerMessage* client_to_server_message) { + DVLOG(1) << "Retrying for types " + << ModelTypeSetToString(request_types); + + InitDownloadUpdatesContext( + session, + create_mobile_bookmarks_folder, + client_to_server_message); + BuildDownloadUpdatesForRetryImpl( + Intersection(request_types, ProtocolTypes()), + session->context()->update_handler_map(), + client_to_server_message->mutable_get_updates()); +} + +void BuildDownloadUpdatesForRetryImpl( + ModelTypeSet proto_request_types, + UpdateHandlerMap* update_handler_map, + sync_pb::GetUpdatesMessage* get_updates) { + DCHECK(!proto_request_types.Empty()); + + InitDownloadUpdatesProgress( + proto_request_types, + update_handler_map, + get_updates); + + // Set legacy GetUpdatesMessage.GetUpdatesCallerInfo information. + get_updates->mutable_caller_info()->set_source( + sync_pb::GetUpdatesCallerInfo::RETRY); + + // Set the new and improved version of source, too. + get_updates->set_get_updates_origin(sync_pb::SyncEnums::RETRY); + get_updates->set_is_retry(true); +} + SyncerError ExecuteDownloadUpdates( ModelTypeSet request_types, SyncSession* session, diff --git a/sync/engine/download.h b/sync/engine/download.h index 5bc08d4..e3230d8 100644 --- a/sync/engine/download.h +++ b/sync/engine/download.h @@ -75,6 +75,21 @@ SYNC_EXPORT_PRIVATE void BuildDownloadUpdatesForPollImpl( UpdateHandlerMap* update_handler_map, sync_pb::GetUpdatesMessage* get_updates); +// Same as BuildDownloadUpdatesForPoll() except the update origin/source is +// RETRY. +SYNC_EXPORT_PRIVATE void BuildDownloadUpdatesForRetry( + sessions::SyncSession* session, + bool create_mobile_bookmarks_folder, + ModelTypeSet request_types, + sync_pb::ClientToServerMessage* client_to_server_message); + +// Same as BuildDownloadUpdatesForPollImpl() except the update origin/source is +// RETRY. +SYNC_EXPORT_PRIVATE void BuildDownloadUpdatesForRetryImpl( + ModelTypeSet proto_request_types, + UpdateHandlerMap* update_handler_map, + sync_pb::GetUpdatesMessage* get_updates); + // Sends the specified message to the server and stores the response in a member // of the |session|'s StatusController. SYNC_EXPORT_PRIVATE SyncerError diff --git a/sync/engine/download_unittest.cc b/sync/engine/download_unittest.cc index eae6277..134f441 100644 --- a/sync/engine/download_unittest.cc +++ b/sync/engine/download_unittest.cc @@ -219,6 +219,43 @@ TEST_F(DownloadUpdatesTest, PollTest) { EXPECT_TRUE(proto_request_types().Equals(progress_types)); } +TEST_F(DownloadUpdatesTest, RetryTest) { + sync_pb::ClientToServerMessage msg; + download::BuildDownloadUpdatesForRetryImpl( + proto_request_types(), + update_handler_map(), + msg.mutable_get_updates()); + + const sync_pb::GetUpdatesMessage& gu_msg = msg.get_updates(); + + EXPECT_EQ(sync_pb::SyncEnums::RETRY, gu_msg.get_updates_origin()); + EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RETRY, + gu_msg.caller_info().source()); + EXPECT_TRUE(gu_msg.is_retry()); + + ModelTypeSet progress_types; + for (int i = 0; i < gu_msg.from_progress_marker_size(); ++i) { + syncer::ModelType type = GetModelTypeFromSpecificsFieldNumber( + gu_msg.from_progress_marker(i).data_type_id()); + progress_types.Put(type); + } + EXPECT_TRUE(proto_request_types().Equals(progress_types)); +} + +TEST_F(DownloadUpdatesTest, NudgeWithRetryTest) { + sessions::NudgeTracker nudge_tracker; + nudge_tracker.RecordLocalChange(ModelTypeSet(BOOKMARKS)); + nudge_tracker.set_next_retry_time( + base::TimeTicks::Now() - base::TimeDelta::FromSeconds(1)); + + sync_pb::ClientToServerMessage msg; + download::BuildNormalDownloadUpdatesImpl(proto_request_types(), + update_handler_map(), + nudge_tracker, + msg.mutable_get_updates()); + EXPECT_TRUE(msg.get_updates().is_retry()); +} + // Verify that a bogus response message is detected. TEST_F(DownloadUpdatesTest, InvalidResponse) { sync_pb::GetUpdatesResponse gu_response; diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc index 6a4f1ef..6c784bc 100644 --- a/sync/engine/sync_scheduler_impl.cc +++ b/sync/engine/sync_scheduler_impl.cc @@ -234,7 +234,8 @@ void SyncSchedulerImpl::Start(Mode mode) { if (old_mode != mode_ && mode_ == NORMAL_MODE && - nudge_tracker_.IsSyncRequired() && + (nudge_tracker_.IsSyncRequired() || + nudge_tracker_.IsRetryRequired(base::TimeTicks::Now())) && CanRunNudgeJobNow(NORMAL_PRIORITY)) { // We just got back to normal mode. Let's try to run the work that was // queued up while we were configuring. @@ -469,7 +470,7 @@ void SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) { if (success) { // That cycle took care of any outstanding work we had. SDVLOG(2) << "Nudge succeeded."; - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); scheduled_nudge_time_ = base::TimeTicks(); // If we're here, then we successfully reached the server. End all backoff. @@ -549,18 +550,8 @@ void SyncSchedulerImpl::HandleFailure( void SyncSchedulerImpl::DoPollSyncSessionJob() { base::AutoReset protector(&no_scheduling_allowed_, true); - if (!CanRunJobNow(NORMAL_PRIORITY)) { - SDVLOG(2) << "Unable to run a poll job right now."; - return; - } - - if (mode_ != NORMAL_MODE) { - SDVLOG(2) << "Not running poll job in configure mode."; - return; - } - SDVLOG(2) << "Polling with types " - << ModelTypeSetToString(session_context_->enabled_types()); + << ModelTypeSetToString(GetEnabledAndUnthrottledTypes()); scoped_ptr session(SyncSession::Build(session_context_, this)); syncer_->PollSyncShare( GetEnabledAndUnthrottledTypes(), @@ -576,6 +567,25 @@ void SyncSchedulerImpl::DoPollSyncSessionJob() { } } +void SyncSchedulerImpl::DoRetrySyncSessionJob() { + DCHECK(CalledOnValidThread()); + DCHECK_EQ(mode_, NORMAL_MODE); + + base::AutoReset protector(&no_scheduling_allowed_, true); + + SDVLOG(2) << "Retrying with types " + << ModelTypeSetToString(GetEnabledAndUnthrottledTypes()); + scoped_ptr session(SyncSession::Build(session_context_, this)); + if (syncer_->RetrySyncShare(GetEnabledAndUnthrottledTypes(), + session.get()) && + !sessions::HasSyncerError( + session->status_controller().model_neutral_state())) { + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); + } else { + HandleFailure(session->status_controller().model_neutral_state()); + } +} + void SyncSchedulerImpl::UpdateNudgeTimeRecords(ModelTypeSet types) { DCHECK(CalledOnValidThread()); base::TimeTicks now = TimeTicks::Now(); @@ -683,11 +693,12 @@ void SyncSchedulerImpl::TrySyncSessionJobImpl() { SDVLOG(2) << "Found pending configure job"; DoConfigurationSyncSessionJob(priority); } - } else { - DCHECK(mode_ == NORMAL_MODE); - if (nudge_tracker_.IsSyncRequired() && CanRunNudgeJobNow(priority)) { + } else if (CanRunNudgeJobNow(priority)) { + if (nudge_tracker_.IsSyncRequired()) { SDVLOG(2) << "Found pending nudge job"; DoNudgeSyncSessionJob(priority); + } else if (nudge_tracker_.IsRetryRequired(base::TimeTicks::Now())) { + DoRetrySyncSessionJob(); } else if (do_poll_after_credentials_updated_ || ((base::TimeTicks::Now() - last_poll_reset_) >= GetPollInterval())) { DoPollSyncSessionJob(); @@ -737,6 +748,10 @@ void SyncSchedulerImpl::PollTimerCallback() { TrySyncSessionJob(); } +void SyncSchedulerImpl::RetryTimerCallback() { + TrySyncSessionJob(); +} + void SyncSchedulerImpl::Unthrottle() { DCHECK(CalledOnValidThread()); DCHECK_EQ(WaitInterval::THROTTLED, wait_interval_->mode); @@ -890,6 +905,12 @@ void SyncSchedulerImpl::OnSyncProtocolError( OnActionableError(snapshot); } +void SyncSchedulerImpl::OnReceivedGuRetryDelay(const base::TimeDelta& delay) { + nudge_tracker_.set_next_retry_time(base::TimeTicks::Now() + delay); + retry_timer_.Start(FROM_HERE, delay, this, + &SyncSchedulerImpl::RetryTimerCallback); +} + void SyncSchedulerImpl::SetNotificationsEnabled(bool notifications_enabled) { DCHECK(CalledOnValidThread()); session_context_->set_notifications_enabled(notifications_enabled); diff --git a/sync/engine/sync_scheduler_impl.h b/sync/engine/sync_scheduler_impl.h index 81a4fcf..68d2a80 100644 --- a/sync/engine/sync_scheduler_impl.h +++ b/sync/engine/sync_scheduler_impl.h @@ -89,6 +89,10 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl virtual void OnReceivedClientInvalidationHintBufferSize(int size) OVERRIDE; virtual void OnSyncProtocolError( const sessions::SyncSessionSnapshot& snapshot) OVERRIDE; + virtual void OnReceivedGuRetryDelay(const base::TimeDelta& delay) OVERRIDE; + + // Returns true if the client is currently in exponential backoff. + bool IsBackingOff() const; private: enum JobPriority { @@ -167,6 +171,9 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl // Invoke the Syncer to perform a poll job. void DoPollSyncSessionJob(); + // Invoke the Syncer to perform a retry job. + void DoRetrySyncSessionJob(); + // Helper function to calculate poll interval. base::TimeDelta GetPollInterval(); @@ -191,9 +198,6 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl const base::TimeDelta& delay, const tracked_objects::Location& nudge_location); - // Returns true if the client is currently in exponential backoff. - bool IsBackingOff() const; - // Helper to signal all listeners registered with |session_context_|. void Notify(SyncEngineEvent::EventCause cause); @@ -230,6 +234,9 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl // Creates a session for a poll and performs the sync. void PollTimerCallback(); + // Creates a session for a retry and performs the sync. + void RetryTimerCallback(); + // Returns the set of types that are enabled and not currently throttled. ModelTypeSet GetEnabledAndUnthrottledTypes(); @@ -336,6 +343,9 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl // to be const and alleviate threading concerns. base::WeakPtrFactory weak_ptr_factory_for_weak_handle_; + // One-shot timer for scheduling GU retry according to delay set by server. + base::OneShotTimer retry_timer_; + DISALLOW_COPY_AND_ASSIGN(SyncSchedulerImpl); }; diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc index 97d9ab8..5ea1f7b 100644 --- a/sync/engine/sync_scheduler_unittest.cc +++ b/sync/engine/sync_scheduler_unittest.cc @@ -34,6 +34,7 @@ using testing::Mock; using testing::Return; using testing::WithArg; using testing::WithArgs; +using testing::WithoutArgs; namespace syncer { using sessions::SyncSession; @@ -51,6 +52,7 @@ class MockSyncer : public Syncer { sync_pb::GetUpdatesCallerInfo::GetUpdatesSource, SyncSession*)); MOCK_METHOD2(PollSyncShare, bool(ModelTypeSet, sessions::SyncSession*)); + MOCK_METHOD2(RetrySyncShare, bool(ModelTypeSet, sessions::SyncSession*)); }; MockSyncer::MockSyncer() @@ -210,6 +212,11 @@ class SyncSchedulerTest : public testing::Test { return scheduler_->nudge_tracker_.GetThrottledTypes(); } + base::TimeDelta GetRetryTimerDelay() { + EXPECT_TRUE(scheduler_->retry_timer_.IsRunning()); + return scheduler_->retry_timer_.GetCurrentDelay(); + } + private: syncable::Directory* directory() { return dir_maker_.directory(); @@ -539,8 +546,9 @@ TEST_F(SyncSchedulerTest, Polling) { SyncShareTimes times; TimeDelta poll_interval(TimeDelta::FromMilliseconds(30)); EXPECT_CALL(*syncer(), PollSyncShare(_,_)).Times(AtLeast(kMinNumSamples)) - .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulatePollSuccess), - RecordSyncShareMultiple(×, kMinNumSamples))); + .WillRepeatedly( + DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess), + RecordSyncShareMultiple(×, kMinNumSamples))); scheduler()->OnReceivedLongPollIntervalUpdate(poll_interval); @@ -559,8 +567,9 @@ TEST_F(SyncSchedulerTest, PollNotificationsDisabled) { SyncShareTimes times; TimeDelta poll_interval(TimeDelta::FromMilliseconds(30)); EXPECT_CALL(*syncer(), PollSyncShare(_,_)).Times(AtLeast(kMinNumSamples)) - .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulatePollSuccess), - RecordSyncShareMultiple(×, kMinNumSamples))); + .WillRepeatedly( + DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess), + RecordSyncShareMultiple(×, kMinNumSamples))); scheduler()->OnReceivedShortPollIntervalUpdate(poll_interval); scheduler()->SetNotificationsEnabled(false); @@ -587,7 +596,7 @@ TEST_F(SyncSchedulerTest, PollIntervalUpdate) { sessions::test_util::SimulatePollIntervalUpdate(poll2)), Return(true))) .WillRepeatedly( - DoAll(Invoke(sessions::test_util::SimulatePollSuccess), + DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess), WithArg<1>( RecordSyncShareMultiple(×, kMinNumSamples)))); @@ -678,8 +687,9 @@ TEST_F(SyncSchedulerTest, ThrottlingExpiresFromPoll) { Return(true))) .RetiresOnSaturation(); EXPECT_CALL(*syncer(), PollSyncShare(_,_)) - .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulatePollSuccess), - RecordSyncShareMultiple(×, kMinNumSamples))); + .WillRepeatedly( + DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess), + RecordSyncShareMultiple(×, kMinNumSamples))); TimeTicks optimal_start = TimeTicks::Now() + poll + throttle1; StartSyncScheduler(SyncScheduler::NORMAL_MODE); @@ -1111,7 +1121,7 @@ TEST_F(SyncSchedulerTest, BackoffRelief) { // Now let the Poll timer do its thing. EXPECT_CALL(*syncer(), PollSyncShare(_,_)) .WillRepeatedly(DoAll( - Invoke(sessions::test_util::SimulatePollSuccess), + Invoke(sessions::test_util::SimulatePollRetrySuccess), RecordSyncShareMultiple(×, kMinNumSamples))); RunLoop(); Mock::VerifyAndClearExpectations(syncer()); @@ -1134,9 +1144,9 @@ TEST_F(SyncSchedulerTest, TransientPollFailure) { UseMockDelayProvider(); // Will cause test failure if backoff is initiated. EXPECT_CALL(*syncer(), PollSyncShare(_,_)) - .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollFailed), + .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollRetryFailed), RecordSyncShare(×))) - .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollSuccess), + .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess), RecordSyncShare(×))); StartSyncScheduler(SyncScheduler::NORMAL_MODE); @@ -1269,8 +1279,9 @@ TEST_F(SyncSchedulerTest, PollFromCanaryAfterAuthError) { ::testing::InSequence seq; EXPECT_CALL(*syncer(), PollSyncShare(_,_)) - .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulatePollSuccess), - RecordSyncShareMultiple(×, kMinNumSamples))); + .WillRepeatedly( + DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess), + RecordSyncShareMultiple(×, kMinNumSamples))); connection()->SetServerStatus(HttpResponse::SYNC_AUTH_ERROR); StartSyncScheduler(SyncScheduler::NORMAL_MODE); @@ -1282,7 +1293,7 @@ TEST_F(SyncSchedulerTest, PollFromCanaryAfterAuthError) { // but after poll finished with auth error from poll timer it should retry // poll once more EXPECT_CALL(*syncer(), PollSyncShare(_,_)) - .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollSuccess), + .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess), RecordSyncShare(×))); scheduler()->OnCredentialsUpdated(); connection()->SetServerStatus(HttpResponse::SERVER_CONNECTION_OK); @@ -1290,4 +1301,89 @@ TEST_F(SyncSchedulerTest, PollFromCanaryAfterAuthError) { StopSyncScheduler(); } +TEST_F(SyncSchedulerTest, SuccessfulRetry) { + StartSyncScheduler(SyncScheduler::NORMAL_MODE); + + SyncShareTimes times; + base::TimeDelta delay = base::TimeDelta::FromMilliseconds(1); + scheduler()->OnReceivedGuRetryDelay(delay); + EXPECT_EQ(delay, GetRetryTimerDelay()); + + EXPECT_CALL(*syncer(), RetrySyncShare(_,_)) + .WillOnce( + DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess), + RecordSyncShare(×))); + + // Run to wait for retrying. + RunLoop(); + + StopSyncScheduler(); +} + +TEST_F(SyncSchedulerTest, FailedRetry) { + UseMockDelayProvider(); + EXPECT_CALL(*delay(), GetDelay(_)) + .WillRepeatedly(Return(TimeDelta::FromMilliseconds(1))); + + StartSyncScheduler(SyncScheduler::NORMAL_MODE); + + base::TimeDelta delay = base::TimeDelta::FromMilliseconds(1); + scheduler()->OnReceivedGuRetryDelay(delay); + + EXPECT_CALL(*syncer(), RetrySyncShare(_,_)) + .WillOnce( + DoAll(Invoke(sessions::test_util::SimulatePollRetryFailed), + QuitLoopNowAction())); + + // Run to wait for retrying. + RunLoop(); + + EXPECT_TRUE(scheduler()->IsBackingOff()); + EXPECT_CALL(*syncer(), RetrySyncShare(_,_)) + .WillOnce( + DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess), + QuitLoopNowAction())); + + // Run to wait for second retrying. + RunLoop(); + + StopSyncScheduler(); +} + +ACTION_P2(VerifyRetryTimerDelay, scheduler_test, expected_delay) { + EXPECT_EQ(expected_delay, scheduler_test->GetRetryTimerDelay()); +} + +TEST_F(SyncSchedulerTest, ReceiveNewRetryDelay) { + StartSyncScheduler(SyncScheduler::NORMAL_MODE); + + SyncShareTimes times; + base::TimeDelta delay1 = base::TimeDelta::FromMilliseconds(10); + base::TimeDelta delay2 = base::TimeDelta::FromMilliseconds(20); + + scheduler()->OnReceivedGuRetryDelay(delay1); + scheduler()->ScheduleLocalRefreshRequest(zero(), ModelTypeSet(BOOKMARKS), + FROM_HERE); + + EXPECT_CALL(*syncer(), NormalSyncShare(_,_,_)) + .WillOnce(DoAll( + WithoutArgs(VerifyRetryTimerDelay(this, delay1)), + WithArg<2>(sessions::test_util::SimulateGuRetryDelayCommand(delay2)), + WithoutArgs(VerifyRetryTimerDelay(this, delay2)), + RecordSyncShare(×))); + + // Run nudge GU. + RunLoop(); + + EXPECT_CALL(*syncer(), RetrySyncShare(_,_)) + .WillOnce( + DoAll(Invoke(sessions::test_util::SimulatePollRetrySuccess), + RecordSyncShare(×))); + + // Run to wait for retrying. + RunLoop(); + + StopSyncScheduler(); +} + } // namespace syncer diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc index 13e1f79..e75c1c6 100644 --- a/sync/engine/syncer.cc +++ b/sync/engine/syncer.cc @@ -57,7 +57,7 @@ bool Syncer::NormalSyncShare(ModelTypeSet request_types, SyncSession* session) { HandleCycleBegin(session); VLOG(1) << "Downloading types " << ModelTypeSetToString(request_types); - if (nudge_tracker.IsGetUpdatesRequired() || + if (nudge_tracker.IsGetUpdatesRequired(base::TimeTicks::Now()) || session->context()->ShouldFetchUpdatesBeforeCommit()) { if (!DownloadAndApplyUpdates( request_types, @@ -109,6 +109,20 @@ bool Syncer::PollSyncShare(ModelTypeSet request_types, return HandleCycleEnd(session, sync_pb::GetUpdatesCallerInfo::PERIODIC); } +bool Syncer::RetrySyncShare(ModelTypeSet request_types, + SyncSession* session) { + HandleCycleBegin(session); + VLOG(1) << "Retrying types " << ModelTypeSetToString(request_types); + DownloadAndApplyUpdates( + request_types, + session, + base::Bind(&download::BuildDownloadUpdatesForRetry, + session, + kCreateMobileBookmarksFolder, + request_types)); + return HandleCycleEnd(session, sync_pb::GetUpdatesCallerInfo::RETRY); +} + void Syncer::ApplyUpdates(SyncSession* session) { TRACE_EVENT0("sync", "ApplyUpdates"); diff --git a/sync/engine/syncer.h b/sync/engine/syncer.h index 6154f91..225b4e3 100644 --- a/sync/engine/syncer.h +++ b/sync/engine/syncer.h @@ -65,6 +65,8 @@ class SYNC_EXPORT_PRIVATE Syncer { // in sync despite bugs or transient failures. virtual bool PollSyncShare(ModelTypeSet request_types, sessions::SyncSession* session); + virtual bool RetrySyncShare(ModelTypeSet request_types, + sessions::SyncSession* session); private: void ApplyUpdates(sessions::SyncSession* session); diff --git a/sync/engine/syncer_proto_util.cc b/sync/engine/syncer_proto_util.cc index bfd8151..bcc0bd4 100644 --- a/sync/engine/syncer_proto_util.cc +++ b/sync/engine/syncer_proto_util.cc @@ -433,6 +433,11 @@ SyncerError SyncerProtoUtil::PostClientToServerMessage( session->delegate()->OnReceivedClientInvalidationHintBufferSize( command.client_invalidation_hint_buffer_size()); } + + if (command.has_gu_retry_delay_seconds()) { + session->delegate()->OnReceivedGuRetryDelay( + base::TimeDelta::FromSeconds(command.gu_retry_delay_seconds())); + } } // Now do any special handling for the error type and decide on the return diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc index 951d443c..119deb1 100644 --- a/sync/engine/syncer_unittest.cc +++ b/sync/engine/syncer_unittest.cc @@ -150,6 +150,7 @@ class SyncerTest : public testing::Test, int size) OVERRIDE { last_client_invalidation_hint_buffer_size_ = size; } + virtual void OnReceivedGuRetryDelay(const base::TimeDelta& delay) OVERRIDE {} virtual void OnSyncProtocolError( const sessions::SyncSessionSnapshot& snapshot) OVERRIDE { } @@ -465,10 +466,10 @@ class SyncerTest : public testing::Test, void ConfigureNoGetUpdatesRequired() { context_->set_server_enabled_pre_commit_update_avoidance(true); nudge_tracker_.OnInvalidationsEnabled(); - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); ASSERT_FALSE(context_->ShouldFetchUpdatesBeforeCommit()); - ASSERT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); + ASSERT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); } base::MessageLoop message_loop_; diff --git a/sync/protocol/client_commands.proto b/sync/protocol/client_commands.proto index c3c18ef..e36914a 100644 --- a/sync/protocol/client_commands.proto +++ b/sync/protocol/client_commands.proto @@ -31,4 +31,7 @@ message ClientCommand { // Maximum number of local nudges to buffer per-type. optional int32 client_invalidation_hint_buffer_size = 6; + + // Time to wait before issuing a retry GU. + optional int32 gu_retry_delay_seconds = 7; }; diff --git a/sync/protocol/get_updates_caller_info.proto b/sync/protocol/get_updates_caller_info.proto index a542324..ce37e61 100644 --- a/sync/protocol/get_updates_caller_info.proto +++ b/sync/protocol/get_updates_caller_info.proto @@ -42,6 +42,7 @@ message GetUpdatesCallerInfo { DATATYPE_REFRESH = 11; // A datatype has requested a refresh. This is // typically used when datatype's have custom // sync UI, e.g. sessions. + RETRY = 13; // A follow-up GU to pick up updates missed by previous GU. } required GetUpdatesSource source = 1; diff --git a/sync/protocol/proto_enum_conversions.cc b/sync/protocol/proto_enum_conversions.cc index c624efe..130aad9 100644 --- a/sync/protocol/proto_enum_conversions.cc +++ b/sync/protocol/proto_enum_conversions.cc @@ -83,7 +83,7 @@ const char* GetPageTransitionRedirectTypeString( const char* GetUpdatesSourceString( sync_pb::GetUpdatesCallerInfo::GetUpdatesSource updates_source) { ASSERT_ENUM_BOUNDS(sync_pb::GetUpdatesCallerInfo, GetUpdatesSource, - UNKNOWN, DATATYPE_REFRESH); + UNKNOWN, RETRY); switch (updates_source) { ENUM_CASE(sync_pb::GetUpdatesCallerInfo, UNKNOWN); ENUM_CASE(sync_pb::GetUpdatesCallerInfo, FIRST_UPDATE); @@ -96,6 +96,7 @@ const char* GetUpdatesSourceString( ENUM_CASE(sync_pb::GetUpdatesCallerInfo, NEW_CLIENT); ENUM_CASE(sync_pb::GetUpdatesCallerInfo, RECONFIGURATION); ENUM_CASE(sync_pb::GetUpdatesCallerInfo, DATATYPE_REFRESH); + ENUM_CASE(sync_pb::GetUpdatesCallerInfo, RETRY); } NOTREACHED(); return ""; @@ -104,7 +105,7 @@ const char* GetUpdatesSourceString( const char* GetUpdatesOriginString( sync_pb::SyncEnums::GetUpdatesOrigin origin) { ASSERT_ENUM_BOUNDS(sync_pb::SyncEnums, GetUpdatesOrigin, - UNKNOWN_ORIGIN, GU_TRIGGER); + UNKNOWN_ORIGIN, RETRY); switch (origin) { ENUM_CASE(sync_pb::SyncEnums, UNKNOWN_ORIGIN); ENUM_CASE(sync_pb::SyncEnums, PERIODIC); @@ -113,6 +114,7 @@ const char* GetUpdatesOriginString( ENUM_CASE(sync_pb::SyncEnums, NEW_CLIENT); ENUM_CASE(sync_pb::SyncEnums, RECONFIGURATION); ENUM_CASE(sync_pb::SyncEnums, GU_TRIGGER); + ENUM_CASE(sync_pb::SyncEnums, RETRY); } NOTREACHED(); return ""; diff --git a/sync/protocol/proto_enum_conversions_unittest.cc b/sync/protocol/proto_enum_conversions_unittest.cc index 7b323a5..d566436 100644 --- a/sync/protocol/proto_enum_conversions_unittest.cc +++ b/sync/protocol/proto_enum_conversions_unittest.cc @@ -60,7 +60,7 @@ TEST_F(ProtoEnumConversionsTest, GetUpdatesSourceString) { sync_pb::GetUpdatesCallerInfo::PERIODIC); TestEnumStringFunction( GetUpdatesSourceString, - sync_pb::GetUpdatesCallerInfo::NEWLY_SUPPORTED_DATATYPE, + sync_pb::GetUpdatesCallerInfo::RETRY, sync_pb::GetUpdatesCallerInfo::GetUpdatesSource_MAX); } diff --git a/sync/protocol/sync.proto b/sync/protocol/sync.proto index ee051cd..d5fc00c 100644 --- a/sync/protocol/sync.proto +++ b/sync/protocol/sync.proto @@ -584,10 +584,14 @@ message GetUpdatesMessage { // already created. Should be set to true only by mobile clients. optional bool create_mobile_bookmarks_folder = 1000 [default = false]; - // This value is an udpated version of the GetUpdatesCallerInfo's + // This value is an updated version of the GetUpdatesCallerInfo's // GetUpdatesSource. It describes the reason for the GetUpdate request. // Introduced in M29. optional SyncEnums.GetUpdatesOrigin get_updates_origin = 9; + + // Whether this GU also serves as a retry GU. Any GU that happens after + // retry timer timeout is a retry GU effectively. + optional bool is_retry = 10 [default = false]; }; message AuthenticateMessage { diff --git a/sync/protocol/sync_enums.proto b/sync/protocol/sync_enums.proto index 348d6d1..90c652b 100644 --- a/sync/protocol/sync_enums.proto +++ b/sync/protocol/sync_enums.proto @@ -141,8 +141,10 @@ message SyncEnums { // confused with FIRST_UPDATE. RECONFIGURATION = 10; // The client is in configuration mode because the // user opted to sync a different set of datatypes. - GU_TRIGGER = 12; // The client is in 'normal' mode. It may have several - // reasons for requesting an update. See the per-type - // GetUpdateTriggers message for more details. + GU_TRIGGER = 12; // The client is in 'normal' mode. It may have several + // reasons for requesting an update. See the per-type + // GetUpdateTriggers message for more details. + RETRY = 13; // A retry GU to pick up updates missed by last GU due to + // replication delay, missing hints, etc. } } diff --git a/sync/sessions/nudge_tracker.cc b/sync/sessions/nudge_tracker.cc index d109294..f96d346 100644 --- a/sync/sessions/nudge_tracker.cc +++ b/sync/sessions/nudge_tracker.cc @@ -44,9 +44,13 @@ bool NudgeTracker::IsSyncRequired() const { return false; } -bool NudgeTracker::IsGetUpdatesRequired() const { +bool NudgeTracker::IsGetUpdatesRequired(base::TimeTicks now) const { if (invalidations_out_of_sync_) return true; + + if (IsRetryRequired(now)) + return true; + for (TypeTrackerMap::const_iterator it = type_trackers_.begin(); it != type_trackers_.end(); ++it) { if (it->second.IsGetUpdatesRequired()) { @@ -56,8 +60,16 @@ bool NudgeTracker::IsGetUpdatesRequired() const { return false; } -void NudgeTracker::RecordSuccessfulSyncCycle() { +bool NudgeTracker::IsRetryRequired(base::TimeTicks now) const { + return !next_retry_time_.is_null() && next_retry_time_ < now; +} + +void NudgeTracker::RecordSuccessfulSyncCycle(base::TimeTicks now) { updates_source_ = sync_pb::GetUpdatesCallerInfo::UNKNOWN; + last_successful_sync_time_ = now; + + if (next_retry_time_ < now) + next_retry_time_ = base::TimeTicks(); // A successful cycle while invalidations are enabled puts us back into sync. invalidations_out_of_sync_ = !invalidations_enabled_; diff --git a/sync/sessions/nudge_tracker.h b/sync/sessions/nudge_tracker.h index fcd0150..1663f0c 100644 --- a/sync/sessions/nudge_tracker.h +++ b/sync/sessions/nudge_tracker.h @@ -36,11 +36,14 @@ class SYNC_EXPORT_PRIVATE NudgeTracker { // Returns true if there is a good reason for performing a get updates // request as part of the next sync cycle. - bool IsGetUpdatesRequired() const; + bool IsGetUpdatesRequired(base::TimeTicks now) const; - // Tells this class that all required update fetching and committing has + // Return true if should perform a sync cycle for GU retry. + bool IsRetryRequired(base::TimeTicks now) const; + + // Tells this class that all required update fetching or committing has // completed successfully. - void RecordSuccessfulSyncCycle(); + void RecordSuccessfulSyncCycle(base::TimeTicks now); // Takes note of a local change. void RecordLocalChange(ModelTypeSet types); @@ -105,6 +108,10 @@ class SYNC_EXPORT_PRIVATE NudgeTracker { // Adjusts the number of hints that can be stored locally. void SetHintBufferSize(size_t size); + void set_next_retry_time(base::TimeTicks next_retry_time) { + next_retry_time_ = next_retry_time; + } + private: typedef std::map TypeTrackerMap; @@ -130,6 +137,11 @@ class SYNC_EXPORT_PRIVATE NudgeTracker { size_t num_payloads_per_type_; + base::TimeTicks last_successful_sync_time_; + + // A retry GU should be issued after this time. + base::TimeTicks next_retry_time_; + DISALLOW_COPY_AND_ASSIGN(NudgeTracker); }; diff --git a/sync/sessions/nudge_tracker_unittest.cc b/sync/sessions/nudge_tracker_unittest.cc index 8fdf2f5..15dcb60 100644 --- a/sync/sessions/nudge_tracker_unittest.cc +++ b/sync/sessions/nudge_tracker_unittest.cc @@ -69,7 +69,7 @@ class NudgeTrackerTest : public ::testing::Test { void SetInvalidationsInSync() { nudge_tracker_.OnInvalidationsEnabled(); - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); } protected: @@ -81,7 +81,7 @@ class NudgeTrackerTest : public ::testing::Test { TEST_F(NudgeTrackerTest, EmptyNudgeTracker) { // Now we're at the normal, "idle" state. EXPECT_FALSE(nudge_tracker_.IsSyncRequired()); - EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::UNKNOWN, nudge_tracker_.updates_source()); @@ -271,7 +271,7 @@ TEST_F(NudgeTrackerTest, DropHintsAtServer_Alone) { } // Clear status then verify. - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); { sync_pb::GetUpdateTriggers gu_trigger; nudge_tracker_.FillProtoMessage(BOOKMARKS, &gu_trigger); @@ -300,7 +300,7 @@ TEST_F(NudgeTrackerTest, DropHintsAtServer_WithOtherInvalidations) { } // Clear status then verify. - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); { sync_pb::GetUpdateTriggers gu_trigger; nudge_tracker_.FillProtoMessage(BOOKMARKS, &gu_trigger); @@ -315,34 +315,34 @@ TEST_F(NudgeTrackerTest, EnableDisableInvalidations) { // Start with invalidations offline. nudge_tracker_.OnInvalidationsDisabled(); EXPECT_TRUE(InvalidationsOutOfSync()); - EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); // Simply enabling invalidations does not bring us back into sync. nudge_tracker_.OnInvalidationsEnabled(); EXPECT_TRUE(InvalidationsOutOfSync()); - EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); // We must successfully complete a sync cycle while invalidations are enabled // to be sure that we're in sync. - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); EXPECT_FALSE(InvalidationsOutOfSync()); - EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); // If the invalidator malfunctions, we go become unsynced again. nudge_tracker_.OnInvalidationsDisabled(); EXPECT_TRUE(InvalidationsOutOfSync()); - EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); // A sync cycle while invalidations are disabled won't reset the flag. - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); EXPECT_TRUE(InvalidationsOutOfSync()); - EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); // Nor will the re-enabling of invalidations be sufficient, even now that // we've had a successful sync cycle. - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); EXPECT_TRUE(InvalidationsOutOfSync()); - EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); } // Tests that locally modified types are correctly written out to the @@ -356,7 +356,7 @@ TEST_F(NudgeTrackerTest, WriteLocallyModifiedTypesToProto) { EXPECT_EQ(1, ProtoLocallyModifiedCount(PREFERENCES)); // Record a successful sync cycle. Verify the count is cleared. - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); EXPECT_EQ(0, ProtoLocallyModifiedCount(PREFERENCES)); } @@ -371,7 +371,7 @@ TEST_F(NudgeTrackerTest, WriteRefreshRequestedTypesToProto) { EXPECT_EQ(1, ProtoRefreshRequestedCount(SESSIONS)); // Record a successful sync cycle. Verify the count is cleared. - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); EXPECT_EQ(0, ProtoRefreshRequestedCount(SESSIONS)); } @@ -382,13 +382,13 @@ TEST_F(NudgeTrackerTest, IsSyncRequired) { // Local changes. nudge_tracker_.RecordLocalChange(ModelTypeSet(SESSIONS)); EXPECT_TRUE(nudge_tracker_.IsSyncRequired()); - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); EXPECT_FALSE(nudge_tracker_.IsSyncRequired()); // Refresh requests. nudge_tracker_.RecordLocalRefreshRequest(ModelTypeSet(SESSIONS)); EXPECT_TRUE(nudge_tracker_.IsSyncRequired()); - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); EXPECT_FALSE(nudge_tracker_.IsSyncRequired()); // Invalidations. @@ -396,33 +396,33 @@ TEST_F(NudgeTrackerTest, IsSyncRequired) { BuildInvalidationMap(PREFERENCES, 1, "hint"); nudge_tracker_.RecordRemoteInvalidation(invalidation_map); EXPECT_TRUE(nudge_tracker_.IsSyncRequired()); - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); EXPECT_FALSE(nudge_tracker_.IsSyncRequired()); } // Basic tests for the IsGetUpdatesRequired() flag. TEST_F(NudgeTrackerTest, IsGetUpdatesRequired) { - EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); // Local changes. nudge_tracker_.RecordLocalChange(ModelTypeSet(SESSIONS)); - EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); - nudge_tracker_.RecordSuccessfulSyncCycle(); - EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); + EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); // Refresh requests. nudge_tracker_.RecordLocalRefreshRequest(ModelTypeSet(SESSIONS)); - EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired()); - nudge_tracker_.RecordSuccessfulSyncCycle(); - EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); + EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); // Invalidations. ObjectIdInvalidationMap invalidation_map = BuildInvalidationMap(PREFERENCES, 1, "hint"); nudge_tracker_.RecordRemoteInvalidation(invalidation_map); - EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired()); - nudge_tracker_.RecordSuccessfulSyncCycle(); - EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); + EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); } // Test IsSyncRequired() responds correctly to data type throttling. @@ -448,7 +448,7 @@ TEST_F(NudgeTrackerTest, IsSyncRequired_Throttling) { EXPECT_TRUE(nudge_tracker_.IsSyncRequired()); // A successful sync cycle means we took care of bookmarks. - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); EXPECT_FALSE(nudge_tracker_.IsSyncRequired()); // But we still haven't dealt with sessions. We'll need to remember @@ -465,32 +465,32 @@ TEST_F(NudgeTrackerTest, IsGetUpdatesRequired_Throttling) { const base::TimeDelta throttle_length = base::TimeDelta::FromMinutes(10); const base::TimeTicks t1 = t0 + throttle_length; - EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); // A refresh request to sessions enables the flag. nudge_tracker_.RecordLocalRefreshRequest(ModelTypeSet(SESSIONS)); - EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); // But the throttling of sessions unsets it. nudge_tracker_.SetTypesThrottledUntil(ModelTypeSet(SESSIONS), throttle_length, t0); - EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); // A refresh request for bookmarks means we have reason to sync again. nudge_tracker_.RecordLocalRefreshRequest(ModelTypeSet(BOOKMARKS)); - EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); // A successful sync cycle means we took care of bookmarks. - nudge_tracker_.RecordSuccessfulSyncCycle(); - EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); + EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); // But we still haven't dealt with sessions. We'll need to remember // that sessions are out of sync and re-enable the flag when their // throttling interval expires. nudge_tracker_.UpdateTypeThrottlingState(t1); EXPECT_FALSE(nudge_tracker_.IsTypeThrottled(SESSIONS)); - EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired()); + EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(base::TimeTicks::Now())); } // Tests throttling-related getter functions when no types are throttled. @@ -566,6 +566,25 @@ TEST_F(NudgeTrackerTest, OverlappingThrottleIntervals) { EXPECT_TRUE(nudge_tracker_.GetThrottledTypes().Empty()); } +TEST_F(NudgeTrackerTest, Retry) { + const base::TimeTicks retry_time = base::TimeTicks::FromInternalValue(12345); + const base::TimeTicks pre_retry_time = + retry_time - base::TimeDelta::FromSeconds(1); + const base::TimeTicks post_retry_time = + retry_time + base::TimeDelta::FromSeconds(1); + nudge_tracker_.set_next_retry_time(retry_time); + + EXPECT_FALSE(nudge_tracker_.IsRetryRequired(pre_retry_time)); + EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired(pre_retry_time)); + nudge_tracker_.RecordSuccessfulSyncCycle(pre_retry_time); + + EXPECT_TRUE(nudge_tracker_.IsRetryRequired(post_retry_time)); + EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired(post_retry_time)); + nudge_tracker_.RecordSuccessfulSyncCycle(post_retry_time); + EXPECT_FALSE(nudge_tracker_.IsRetryRequired( + post_retry_time + base::TimeDelta::FromSeconds(1))); +} + class NudgeTrackerAckTrackingTest : public NudgeTrackerTest { public: NudgeTrackerAckTrackingTest() {} @@ -627,7 +646,7 @@ class NudgeTrackerAckTrackingTest : public NudgeTrackerTest { } void RecordSuccessfulSyncCycle() { - nudge_tracker_.RecordSuccessfulSyncCycle(); + nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now()); } private: diff --git a/sync/sessions/sync_session.h b/sync/sessions/sync_session.h index f576720..e25c718 100644 --- a/sync/sessions/sync_session.h +++ b/sync/sessions/sync_session.h @@ -81,6 +81,9 @@ class SYNC_EXPORT_PRIVATE SyncSession { // will buffer locally. virtual void OnReceivedClientInvalidationHintBufferSize(int size) = 0; + // Called when server wants to schedule a retry GU. + virtual void OnReceivedGuRetryDelay(const base::TimeDelta& delay) = 0; + protected: virtual ~Delegate() {} }; diff --git a/sync/sessions/test_util.cc b/sync/sessions/test_util.cc index 538ed0f..951db8d 100644 --- a/sync/sessions/test_util.cc +++ b/sync/sessions/test_util.cc @@ -81,15 +81,15 @@ void SimulateConnectionFailure( NETWORK_CONNECTION_UNAVAILABLE); } -void SimulatePollSuccess(ModelTypeSet requested_types, - sessions::SyncSession* session) { +void SimulatePollRetrySuccess(ModelTypeSet requested_types, + sessions::SyncSession* session) { ASSERT_EQ(0U, session->status_controller().num_server_changes_remaining()); session->mutable_status_controller()->set_last_download_updates_result( SYNCER_OK); } -void SimulatePollFailed(ModelTypeSet requested_types, - sessions::SyncSession* session) { +void SimulatePollRetryFailed(ModelTypeSet requested_types, + sessions::SyncSession* session) { ASSERT_EQ(0U, session->status_controller().num_server_changes_remaining()); session->mutable_status_controller()->set_last_download_updates_result( SERVER_RETURN_TRANSIENT_ERROR); @@ -116,7 +116,7 @@ void SimulatePollIntervalUpdateImpl( ModelTypeSet requested_types, sessions::SyncSession* session, const base::TimeDelta& new_poll) { - SimulatePollSuccess(requested_types, session); + SimulatePollRetrySuccess(requested_types, session); session->delegate()->OnReceivedLongPollIntervalUpdate(new_poll); } @@ -129,6 +129,13 @@ void SimulateSessionsCommitDelayUpdateImpl( session->delegate()->OnReceivedSessionsCommitDelay(new_delay); } +void SimulateGuRetryDelayCommandImpl(sessions::SyncSession* session, + base::TimeDelta delay) { + session->mutable_status_controller()->set_last_download_updates_result( + SYNCER_OK); + session->delegate()->OnReceivedGuRetryDelay(delay); +} + } // namespace test_util } // namespace sessions } // namespace syncer diff --git a/sync/sessions/test_util.h b/sync/sessions/test_util.h index c670128..bbdf32a 100644 --- a/sync/sessions/test_util.h +++ b/sync/sessions/test_util.h @@ -47,11 +47,14 @@ void SimulateConnectionFailure(ModelTypeSet requested_types, const sessions::NudgeTracker& nudge_tracker, sessions::SyncSession* session); -// Poll successes and failures. -void SimulatePollSuccess(ModelTypeSet requested_types, - sessions::SyncSession* session); -void SimulatePollFailed(ModelTypeSet requested_types, - sessions::SyncSession* session); +// Poll/Retry successes and failures. +void SimulatePollRetrySuccess(ModelTypeSet requested_types, + sessions::SyncSession* session); +void SimulatePollRetryFailed(ModelTypeSet requested_types, + sessions::SyncSession* session); + +void SimulateGuRetryDelayCommandImpl(sessions::SyncSession* session, + base::TimeDelta delay); void SimulateThrottledImpl(sessions::SyncSession* session, const base::TimeDelta& delta); @@ -90,6 +93,10 @@ ACTION_P(SimulateSessionsCommitDelayUpdate, poll) { SimulateSessionsCommitDelayUpdateImpl(arg0, arg1, arg2, poll); } +ACTION_P(SimulateGuRetryDelayCommand, delay) { + SimulateGuRetryDelayCommandImpl(arg0, delay); +} + } // namespace test_util } // namespace sessions } // namespace syncer diff --git a/sync/test/engine/fake_sync_scheduler.cc b/sync/test/engine/fake_sync_scheduler.cc index 6f704c6..7a6d20c 100644 --- a/sync/test/engine/fake_sync_scheduler.cc +++ b/sync/test/engine/fake_sync_scheduler.cc @@ -86,4 +86,8 @@ void FakeSyncScheduler::OnSyncProtocolError( const sessions::SyncSessionSnapshot& snapshot) { } +void FakeSyncScheduler::OnReceivedGuRetryDelay( + const base::TimeDelta& delay) { +} + } // namespace syncer diff --git a/sync/test/engine/fake_sync_scheduler.h b/sync/test/engine/fake_sync_scheduler.h index 9cab49d..0020ccd 100644 --- a/sync/test/engine/fake_sync_scheduler.h +++ b/sync/test/engine/fake_sync_scheduler.h @@ -57,6 +57,8 @@ class FakeSyncScheduler : public SyncScheduler { virtual void OnReceivedClientInvalidationHintBufferSize(int size) OVERRIDE; virtual void OnSyncProtocolError( const sessions::SyncSessionSnapshot& snapshot) OVERRIDE; + virtual void OnReceivedGuRetryDelay( + const base::TimeDelta& delay) OVERRIDE; }; } // namespace syncer -- cgit v1.1