diff options
38 files changed, 531 insertions, 211 deletions
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc index 2f91728..3816312 100644 --- a/chrome/browser/sync/glue/sync_backend_host.cc +++ b/chrome/browser/sync/glue/sync_backend_host.cc @@ -198,11 +198,10 @@ class SyncBackendHost::Core void DoFinishInitialProcessControlTypes(); // The shutdown order is a bit complicated: - // 1) Call DoStopSyncManagerForShutdown() from |frontend_loop_| to request - // sync manager to stop as soon as possible. + // 1) Call the SyncManagerStopHandle's RequestStop() from |frontend_loop_| to + // request sync manager to stop as soon as possible. // 2) Post DoShutdown() to sync loop to clean up backend state, save // directory and destroy sync manager. - void DoStopSyncManagerForShutdown(); void DoShutdown(bool sync_disabled); void DoDestroySyncManager(); @@ -401,7 +400,8 @@ void SyncBackendHost::Initialize( new InternalComponentsFactoryImpl(factory_switches)).Pass(), unrecoverable_error_handler.Pass(), report_unrecoverable_error_function, - !cl->HasSwitch(switches::kSyncDisableOAuth2Token))); + !cl->HasSwitch(switches::kSyncDisableOAuth2Token), + &cancelation_signal_)); InitCore(init_opts.Pass()); } @@ -492,24 +492,9 @@ bool SyncBackendHost::SetDecryptionPassphrase(const std::string& passphrase) { return true; } -void SyncBackendHost::StopSyncManagerForShutdown() { - DCHECK_GT(initialization_state_, NOT_ATTEMPTED); - if (initialization_state_ == CREATING_SYNC_MANAGER) { - // We post here to implicitly wait for the SyncManager to be created, - // if needed. We have to wait, since we need to shutdown immediately, - // and we need to tell the SyncManager so it can abort any activity - // (net I/O, data application). - DCHECK(registrar_->sync_thread()->IsRunning()); - registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, - base::Bind(&SyncBackendHost::Core::DoStopSyncManagerForShutdown, - core_.get())); - } else { - core_->DoStopSyncManagerForShutdown(); - } -} - void SyncBackendHost::StopSyncingForShutdown() { DCHECK_EQ(base::MessageLoop::current(), frontend_loop_); + DCHECK_GT(initialization_state_, NOT_ATTEMPTED); // Immediately stop sending messages to the frontend. frontend_ = NULL; @@ -521,7 +506,7 @@ void SyncBackendHost::StopSyncingForShutdown() { registrar_->RequestWorkerStopOnUIThread(); - StopSyncManagerForShutdown(); + cancelation_signal_.RequestStop(); } scoped_ptr<base::Thread> SyncBackendHost::Shutdown(ShutdownOption option) { @@ -883,7 +868,8 @@ SyncBackendHost::DoInitializeOptions::DoInitializeOptions( scoped_ptr<syncer::UnrecoverableErrorHandler> unrecoverable_error_handler, syncer::ReportUnrecoverableErrorFunction report_unrecoverable_error_function, - bool use_oauth2_token) + bool use_oauth2_token, + syncer::CancelationSignal* const cancelation_signal) : sync_loop(sync_loop), registrar(registrar), routing_info(routing_info), @@ -903,7 +889,8 @@ SyncBackendHost::DoInitializeOptions::DoInitializeOptions( unrecoverable_error_handler(unrecoverable_error_handler.Pass()), report_unrecoverable_error_function( report_unrecoverable_error_function), - use_oauth2_token(use_oauth2_token) { + use_oauth2_token(use_oauth2_token), + cancelation_signal(cancelation_signal) { } SyncBackendHost::DoInitializeOptions::~DoInitializeOptions() {} @@ -1168,7 +1155,8 @@ void SyncBackendHost::Core::DoInitialize( &encryptor_, options->unrecoverable_error_handler.Pass(), options->report_unrecoverable_error_function, - options->use_oauth2_token); + options->use_oauth2_token, + options->cancelation_signal); // |sync_manager_| may end up being NULL here in tests (in // synchronous initialization mode). @@ -1278,11 +1266,6 @@ void SyncBackendHost::Core::DoEnableEncryptEverything() { sync_manager_->GetEncryptionHandler()->EnableEncryptEverything(); } -void SyncBackendHost::Core::DoStopSyncManagerForShutdown() { - if (sync_manager_) - sync_manager_->StopSyncingForShutdown(); -} - void SyncBackendHost::Core::DoShutdown(bool sync_disabled) { DCHECK_EQ(base::MessageLoop::current(), sync_loop_); // It's safe to do this even if the type was never activated. diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h index 113ad42..05ed3fc 100644 --- a/chrome/browser/sync/glue/sync_backend_host.h +++ b/chrome/browser/sync/glue/sync_backend_host.h @@ -20,6 +20,7 @@ #include "content/public/browser/notification_observer.h" #include "content/public/browser/notification_registrar.h" #include "google_apis/gaia/google_service_auth_error.h" +#include "sync/internal_api/public/base/cancelation_signal.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/configure_reason.h" #include "sync/internal_api/public/engine/model_safe_worker.h" @@ -218,10 +219,9 @@ class SyncBackendHost bool SetDecryptionPassphrase(const std::string& passphrase) WARN_UNUSED_RESULT; - // Called on |frontend_loop_| to kick off shutdown procedure. After this, no - // further sync activity will occur with the sync server and no further - // change applications will occur from changes already downloaded. - // Furthermore, no notifications will be sent to any invalidation handler. + // Called on |frontend_loop_| to kick off shutdown procedure. Attempts to cut + // short any long-lived or blocking sync thread tasks so that the shutdown on + // sync thread task that we're about to post won't have to wait very long. virtual void StopSyncingForShutdown(); // Called on |frontend_loop_| to kick off shutdown. @@ -339,7 +339,8 @@ class SyncBackendHost unrecoverable_error_handler, syncer::ReportUnrecoverableErrorFunction report_unrecoverable_error_function, - bool use_oauth2_token); + bool use_oauth2_token, + syncer::CancelationSignal* cancelation_signal); ~DoInitializeOptions(); base::MessageLoop* sync_loop; @@ -363,6 +364,7 @@ class SyncBackendHost syncer::ReportUnrecoverableErrorFunction report_unrecoverable_error_function; bool use_oauth2_token; + syncer::CancelationSignal* const cancelation_signal; }; // Allows tests to perform alternate core initialization work. @@ -523,10 +525,6 @@ class SyncBackendHost virtual void OnIncomingInvalidation( const syncer::ObjectIdInvalidationMap& invalidation_map) OVERRIDE; - // Handles stopping the core's SyncManager, accounting for whether - // initialization is done yet. - void StopSyncManagerForShutdown(); - base::WeakPtrFactory<SyncBackendHost> weak_ptr_factory_; content::NotificationRegistrar notification_registrar_; @@ -589,6 +587,8 @@ class SyncBackendHost invalidation::InvalidationService* invalidator_; bool invalidation_handler_registered_; + syncer::CancelationSignal cancelation_signal_; + DISALLOW_COPY_AND_ASSIGN(SyncBackendHost); }; diff --git a/sync/engine/net/server_connection_manager.cc b/sync/engine/net/server_connection_manager.cc index 0076543..a54c975 100644 --- a/sync/engine/net/server_connection_manager.cc +++ b/sync/engine/net/server_connection_manager.cc @@ -16,6 +16,7 @@ #include "net/http/http_status_code.h" #include "sync/engine/net/url_translator.h" #include "sync/engine/syncer.h" +#include "sync/internal_api/public/base/cancelation_signal.h" #include "sync/protocol/sync.pb.h" #include "sync/syncable/directory.h" #include "url/gurl.h" @@ -114,13 +115,32 @@ bool ServerConnectionManager::Connection::ReadDownloadResponse( } ServerConnectionManager::ScopedConnectionHelper::ScopedConnectionHelper( - ServerConnectionManager* manager, Connection* connection) - : manager_(manager), connection_(connection) {} + CancelationSignal* signaller, scoped_ptr<Connection> connection) + : cancelation_signal_(signaller), connection_(connection.Pass()) { + // Special early return for tests. + if (!connection_.get()) + return; + + if (!cancelation_signal_->TryRegisterHandler(this)) { + connection_.reset(); + } +} + +// This function may be called from another thread. +void ServerConnectionManager::ScopedConnectionHelper::OnStopRequested() { + DCHECK(connection_); + connection_->Abort(); +} ServerConnectionManager::ScopedConnectionHelper::~ScopedConnectionHelper() { - if (connection_) - manager_->OnConnectionDestroyed(connection_.get()); - connection_.reset(); + // We should be registered iff connection_.get() != NULL. + if (connection_.get()) { + // It is important that this be called before this destructor completes. + // Until the unregistration is complete, it's possible that the virtual + // OnStopRequested() function may be called from a different thread. We + // need to unregister it before destruction modifies our vptr. + cancelation_signal_->UnregisterHandler(this); + } } ServerConnectionManager::Connection* @@ -177,43 +197,20 @@ ServerConnectionManager::ServerConnectionManager( const string& server, int port, bool use_ssl, - bool use_oauth2_token) + bool use_oauth2_token, + CancelationSignal* cancelation_signal) : sync_server_(server), sync_server_port_(port), use_ssl_(use_ssl), use_oauth2_token_(use_oauth2_token), proto_sync_path_(kSyncServerSyncPath), server_status_(HttpResponse::NONE), - terminated_(false), - active_connection_(NULL) { + cancelation_signal_(cancelation_signal) { } ServerConnectionManager::~ServerConnectionManager() { } -ServerConnectionManager::Connection* -ServerConnectionManager::MakeActiveConnection() { - base::AutoLock lock(terminate_connection_lock_); - DCHECK(!active_connection_); - if (terminated_) - return NULL; - - active_connection_ = MakeConnection(); - return active_connection_; -} - -void ServerConnectionManager::OnConnectionDestroyed(Connection* connection) { - DCHECK(connection); - base::AutoLock lock(terminate_connection_lock_); - // |active_connection_| can be NULL already if it was aborted. Also, - // it can legitimately be a different Connection object if a new Connection - // was created after a previous one was Aborted and destroyed. - if (active_connection_ != connection) - return; - - active_connection_ = NULL; -} - bool ServerConnectionManager::SetAuthToken(const std::string& auth_token) { DCHECK(thread_checker_.CalledOnValidThread()); if (previously_invalidated_token != auth_token) { @@ -278,7 +275,7 @@ bool ServerConnectionManager::PostBufferToPath(PostBufferParams* params, // When our connection object falls out of scope, it clears itself from // active_connection_. - ScopedConnectionHelper post(this, MakeActiveConnection()); + ScopedConnectionHelper post(cancelation_signal_, MakeConnection()); if (!post.get()) { params->response.server_status = HttpResponse::CONNECTION_UNAVAILABLE; return false; @@ -343,20 +340,10 @@ void ServerConnectionManager::RemoveListener( listeners_.RemoveObserver(listener); } -ServerConnectionManager::Connection* ServerConnectionManager::MakeConnection() +scoped_ptr<ServerConnectionManager::Connection> +ServerConnectionManager::MakeConnection() { - return NULL; // For testing. -} - -void ServerConnectionManager::TerminateAllIO() { - base::AutoLock lock(terminate_connection_lock_); - terminated_ = true; - if (active_connection_) - active_connection_->Abort(); - - // Sever our ties to this connection object. Note that it still may exist, - // since we don't own it, but it has been neutered. - active_connection_ = NULL; + return scoped_ptr<Connection>(); // For testing. } std::ostream& operator << (std::ostream& s, const struct HttpResponse& hr) { diff --git a/sync/engine/net/server_connection_manager.h b/sync/engine/net/server_connection_manager.h index 3bd533e..17c87db 100644 --- a/sync/engine/net/server_connection_manager.h +++ b/sync/engine/net/server_connection_manager.h @@ -8,14 +8,13 @@ #include <iosfwd> #include <string> -#include "base/atomicops.h" #include "base/memory/scoped_ptr.h" #include "base/observer_list.h" #include "base/strings/string_util.h" -#include "base/synchronization/lock.h" #include "base/threading/non_thread_safe.h" #include "base/threading/thread_checker.h" #include "sync/base/sync_export.h" +#include "sync/internal_api/public/base/cancelation_observer.h" #include "sync/syncable/syncable_id.h" namespace sync_pb { @@ -24,6 +23,8 @@ class ClientToServerMessage; namespace syncer { +class CancelationSignal; + namespace syncable { class Directory; } @@ -183,7 +184,8 @@ class SYNC_EXPORT_PRIVATE ServerConnectionManager { ServerConnectionManager(const std::string& server, int port, bool use_ssl, - bool use_oauth2_token); + bool use_oauth2_token, + CancelationSignal* cancelation_signal); virtual ~ServerConnectionManager(); @@ -213,13 +215,7 @@ class SYNC_EXPORT_PRIVATE ServerConnectionManager { // Factory method to create an Connection object we can use for // communication with the server. - virtual Connection* MakeConnection(); - - // Aborts any active HTTP POST request. - // We expect this to get called on a different thread than the valid - // ThreadChecker thread, as we want to kill any pending http traffic without - // having to wait for the request to complete. - void TerminateAllIO(); + virtual scoped_ptr<Connection> MakeConnection(); void set_client_id(const std::string& client_id) { DCHECK(thread_checker_.CalledOnValidThread()); @@ -272,10 +268,6 @@ class SYNC_EXPORT_PRIVATE ServerConnectionManager { // terminated, this will return NULL. Connection* MakeActiveConnection(); - // Called by Connection objects as they are destroyed to allow the - // ServerConnectionManager to cleanup active connections. - void OnConnectionDestroyed(Connection* connection); - // The sync_server_ is the server that requests will be made to. std::string sync_server_; @@ -308,35 +300,34 @@ class SYNC_EXPORT_PRIVATE ServerConnectionManager { base::ThreadChecker thread_checker_; - // Protects all variables below to allow bailing out of active connections. - base::Lock terminate_connection_lock_; - - // If true, we've been told to terminate IO and expect to be destroyed - // shortly. No future network requests will be made. - bool terminated_; - - // A non-owning pointer to any active http connection, so that we can abort - // it if necessary. - Connection* active_connection_; + CancelationSignal* const cancelation_signal_; private: friend class Connection; friend class ScopedServerStatusWatcher; - // A class to help deal with cleaning up active Connection objects when (for - // ex) multiple early-exits are present in some scope. ScopedConnectionHelper - // informs the ServerConnectionManager before the Connection object it takes - // ownership of is destroyed. - class ScopedConnectionHelper { + // A class to help manage the active connection. It handles the registration + // and unregistration with the CancelationSignal. It also takes ownership of + // the connection and will delete it if the abort signal was sent early or + // when this class goes out of scope. + class ScopedConnectionHelper : public CancelationObserver { public: - // |manager| must outlive this. Takes ownership of |connection|. - ScopedConnectionHelper(ServerConnectionManager* manager, - Connection* connection); - ~ScopedConnectionHelper(); + ScopedConnectionHelper(CancelationSignal* cancelation_signal, + scoped_ptr<Connection> connection); + virtual ~ScopedConnectionHelper(); Connection* get(); + + // Called from a different thread when the CancelationObserver's + // RequestStop() is called and this class has been registered as a handler. + // + // Marked final because there's no way to safely override it. See comment + // in this class' destructor. + virtual void OnStopRequested() OVERRIDE FINAL; + private: - ServerConnectionManager* manager_; + CancelationSignal* const cancelation_signal_; scoped_ptr<Connection> connection_; + DISALLOW_COPY_AND_ASSIGN(ScopedConnectionHelper); }; diff --git a/sync/engine/sync_scheduler.h b/sync/engine/sync_scheduler.h index 923c7be..b31af82 100644 --- a/sync/engine/sync_scheduler.h +++ b/sync/engine/sync_scheduler.h @@ -75,11 +75,9 @@ class SYNC_EXPORT_PRIVATE SyncScheduler // Note: must already be in CONFIGURATION mode. virtual bool ScheduleConfiguration(const ConfigurationParams& params) = 0; - // Request that any running syncer task stop as soon as possible and - // cancel all scheduled tasks. This function can be called from any thread, - // and should in fact be called from a thread that isn't the sync loop to - // allow preempting ongoing sync cycles. - virtual void RequestStop() = 0; + // Request that the syncer avoid starting any new tasks and prepare for + // shutdown. + virtual void Stop() = 0; // The meat and potatoes. All three of the following methods will post a // delayed task to attempt the actual nudge (see ScheduleNudgeImpl). diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc index 93eb0fb..78010d7 100644 --- a/sync/engine/sync_scheduler_impl.cc +++ b/sync/engine/sync_scheduler_impl.cc @@ -174,7 +174,7 @@ SyncSchedulerImpl::SyncSchedulerImpl(const std::string& name, SyncSchedulerImpl::~SyncSchedulerImpl() { DCHECK(CalledOnValidThread()); - StopImpl(); + Stop(); } void SyncSchedulerImpl::OnCredentialsUpdated() { @@ -643,17 +643,9 @@ void SyncSchedulerImpl::RestartWaiting() { } } -void SyncSchedulerImpl::RequestStop() { - syncer_->RequestEarlyExit(); // Safe to call from any thread. - DCHECK(weak_handle_this_.IsInitialized()); - SDVLOG(3) << "Posting StopImpl"; - weak_handle_this_.Call(FROM_HERE, - &SyncSchedulerImpl::StopImpl); -} - -void SyncSchedulerImpl::StopImpl() { +void SyncSchedulerImpl::Stop() { DCHECK(CalledOnValidThread()); - SDVLOG(2) << "StopImpl called"; + SDVLOG(2) << "Stop called"; // Kill any in-flight method calls. weak_ptr_factory_.InvalidateWeakPtrs(); @@ -861,7 +853,7 @@ void SyncSchedulerImpl::OnReceivedClientInvalidationHintBufferSize(int size) { void SyncSchedulerImpl::OnShouldStopSyncingPermanently() { DCHECK(CalledOnValidThread()); SDVLOG(2) << "OnShouldStopSyncingPermanently"; - syncer_->RequestEarlyExit(); // Thread-safe. + Stop(); Notify(SyncEngineEvent::STOP_SYNCING_PERMANENTLY); } @@ -880,7 +872,7 @@ void SyncSchedulerImpl::OnSyncProtocolError( if (ShouldRequestEarlyExit( snapshot.model_neutral_state().sync_protocol_error)) { SDVLOG(2) << "Sync Scheduler requesting early exit."; - syncer_->RequestEarlyExit(); // Thread-safe. + Stop(); } if (IsActionableError(snapshot.model_neutral_state().sync_protocol_error)) OnActionableError(snapshot); diff --git a/sync/engine/sync_scheduler_impl.h b/sync/engine/sync_scheduler_impl.h index b8dcce9..8492463 100644 --- a/sync/engine/sync_scheduler_impl.h +++ b/sync/engine/sync_scheduler_impl.h @@ -54,7 +54,7 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl virtual void Start(Mode mode) OVERRIDE; virtual bool ScheduleConfiguration( const ConfigurationParams& params) OVERRIDE; - virtual void RequestStop() OVERRIDE; + virtual void Stop() OVERRIDE; virtual void ScheduleLocalNudge( const base::TimeDelta& desired_delay, ModelTypeSet types, @@ -181,9 +181,6 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl // Determines if we're allowed to contact the server right now. bool CanRunNudgeJobNow(JobPriority priority); - // 'Impl' here refers to real implementation of public functions. - void StopImpl(); - // If the scheduler's current state supports it, this will create a job based // on the passed in parameters and coalesce it with any other pending jobs, // then post a delayed task to run it. It may also choose to drop the job or diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc index e0ea9a8..01cbdd7 100644 --- a/sync/engine/sync_scheduler_unittest.cc +++ b/sync/engine/sync_scheduler_unittest.cc @@ -41,6 +41,7 @@ using sync_pb::GetUpdatesCallerInfo; class MockSyncer : public Syncer { public: + MockSyncer(); MOCK_METHOD3(NormalSyncShare, bool(ModelTypeSet, const sessions::NudgeTracker&, sessions::SyncSession*)); @@ -51,6 +52,9 @@ class MockSyncer : public Syncer { MOCK_METHOD2(PollSyncShare, bool(ModelTypeSet, sessions::SyncSession*)); }; +MockSyncer::MockSyncer() + : Syncer(NULL) {} + typedef std::vector<TimeTicks> SyncShareTimes; void QuitLoopNow() { diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc index 3d66ca3..3fa9a28 100644 --- a/sync/engine/syncer.cc +++ b/sync/engine/syncer.cc @@ -19,6 +19,7 @@ #include "sync/engine/net/server_connection_manager.h" #include "sync/engine/process_commit_response_command.h" #include "sync/engine/syncer_types.h" +#include "sync/internal_api/public/base/cancelation_signal.h" #include "sync/internal_api/public/base/unique_position.h" #include "sync/internal_api/public/util/syncer_error.h" #include "sync/sessions/nudge_tracker.h" @@ -43,20 +44,14 @@ using sessions::StatusController; using sessions::SyncSession; using sessions::NudgeTracker; -Syncer::Syncer() - : early_exit_requested_(false) { +Syncer::Syncer(syncer::CancelationSignal* cancelation_signal) + : cancelation_signal_(cancelation_signal) { } Syncer::~Syncer() {} bool Syncer::ExitRequested() { - base::AutoLock lock(early_exit_requested_lock_); - return early_exit_requested_; -} - -void Syncer::RequestEarlyExit() { - base::AutoLock lock(early_exit_requested_lock_); - early_exit_requested_ = true; + return cancelation_signal_->IsStopRequested(); } bool Syncer::NormalSyncShare(ModelTypeSet request_types, diff --git a/sync/engine/syncer.h b/sync/engine/syncer.h index e1e5eac..132f6ef 100644 --- a/sync/engine/syncer.h +++ b/sync/engine/syncer.h @@ -21,6 +21,8 @@ namespace syncer { +class CancelationSignal; + // A Syncer provides a control interface for driving the individual steps // of the sync cycle. Each cycle (hopefully) moves the client into closer // synchronization with the server. The individual steps are modeled @@ -35,13 +37,10 @@ class SYNC_EXPORT_PRIVATE Syncer { public: typedef std::vector<int64> UnsyncedMetaHandles; - Syncer(); + Syncer(CancelationSignal* cancelation_signal); virtual ~Syncer(); - // Called by other threads to tell the syncer to stop what it's doing - // and return early from SyncShare, if possible. bool ExitRequested(); - void RequestEarlyExit(); // Fetches and applies updates, resolves conflicts and commits local changes // for |request_types| as necessary until client and server states are in @@ -79,8 +78,7 @@ class SYNC_EXPORT_PRIVATE Syncer { sessions::SyncSession* session, sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source); - bool early_exit_requested_; - base::Lock early_exit_requested_lock_; + syncer::CancelationSignal* const cancelation_signal_; friend class SyncerTest; FRIEND_TEST_ALL_PREFIXES(SyncerTest, NameClashWithResolver); diff --git a/sync/engine/syncer_proto_util_unittest.cc b/sync/engine/syncer_proto_util_unittest.cc index c288132..62c752d 100644 --- a/sync/engine/syncer_proto_util_unittest.cc +++ b/sync/engine/syncer_proto_util_unittest.cc @@ -255,7 +255,7 @@ TEST_F(SyncerProtoUtilTest, AddRequestBirthday) { class DummyConnectionManager : public ServerConnectionManager { public: DummyConnectionManager() - : ServerConnectionManager("unused", 0, false, false), + : ServerConnectionManager("unused", 0, false, false, NULL), send_error_(false), access_denied_(false) {} diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc index 068d863..d2a2cec 100644 --- a/sync/engine/syncer_unittest.cc +++ b/sync/engine/syncer_unittest.cc @@ -30,6 +30,7 @@ #include "sync/engine/syncer.h" #include "sync/engine/syncer_proto_util.h" #include "sync/engine/traffic_recorder.h" +#include "sync/internal_api/public/base/cancelation_signal.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/engine/model_safe_worker.h" #include "sync/protocol/bookmark_specifics.pb.h" @@ -228,7 +229,7 @@ class SyncerTest : public testing::Test, false, // force enable pre-commit GU avoidance experiment "fake_invalidator_client_id")); context_->set_routing_info(routing_info); - syncer_ = new Syncer(); + syncer_ = new Syncer(&cancelation_signal_); syncable::ReadTransaction trans(FROM_HERE, directory()); syncable::Directory::Metahandles children; @@ -551,6 +552,7 @@ class SyncerTest : public testing::Test, FakeEncryptor encryptor_; scoped_refptr<ExtensionsActivity> extensions_activity_; scoped_ptr<MockConnectionManager> mock_server_; + CancelationSignal cancelation_signal_; Syncer* syncer_; diff --git a/sync/internal_api/internal_components_factory_impl.cc b/sync/internal_api/internal_components_factory_impl.cc index d5fc102..6ccb143 100644 --- a/sync/internal_api/internal_components_factory_impl.cc +++ b/sync/internal_api/internal_components_factory_impl.cc @@ -21,15 +21,20 @@ InternalComponentsFactoryImpl::InternalComponentsFactoryImpl( InternalComponentsFactoryImpl::~InternalComponentsFactoryImpl() { } scoped_ptr<SyncScheduler> InternalComponentsFactoryImpl::BuildScheduler( - const std::string& name, sessions::SyncSessionContext* context) { + const std::string& name, + sessions::SyncSessionContext* context, + CancelationSignal* cancelation_signal) { scoped_ptr<BackoffDelayProvider> delay(BackoffDelayProvider::FromDefaults()); if (switches_.backoff_override == BACKOFF_SHORT_INITIAL_RETRY_OVERRIDE) delay.reset(BackoffDelayProvider::WithShortInitialRetryOverride()); - return scoped_ptr<SyncScheduler>( - new SyncSchedulerImpl(name, delay.release(), context, new Syncer())); + return scoped_ptr<SyncScheduler>(new SyncSchedulerImpl( + name, + delay.release(), + context, + new Syncer(cancelation_signal))); } scoped_ptr<sessions::SyncSessionContext> diff --git a/sync/internal_api/public/base/cancelation_observer.cc b/sync/internal_api/public/base/cancelation_observer.cc new file mode 100644 index 0000000..f50b6a3 --- /dev/null +++ b/sync/internal_api/public/base/cancelation_observer.cc @@ -0,0 +1,13 @@ +// Copyright 2013 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 "sync/internal_api/public/base/cancelation_observer.h" + +namespace syncer { + +CancelationObserver::CancelationObserver() {} + +CancelationObserver::~CancelationObserver() {} + +} // namespace syncer diff --git a/sync/internal_api/public/base/cancelation_observer.h b/sync/internal_api/public/base/cancelation_observer.h new file mode 100644 index 0000000..9581004 --- /dev/null +++ b/sync/internal_api/public/base/cancelation_observer.h @@ -0,0 +1,25 @@ +// Copyright 2013 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. + +#ifndef SYNC_INTERNAL_API_PUBLIC_BASE_CANCELATION_OBSERVER_H_ +#define SYNC_INTERNAL_API_PUBLIC_BASE_CANCELATION_OBSERVER_H_ + +#include "sync/base/sync_export.h" + +namespace syncer { + +// Interface for classes that handle signals from the CancelationSignal. +class SYNC_EXPORT_PRIVATE CancelationObserver { + public: + CancelationObserver(); + virtual ~CancelationObserver() = 0; + + // This may be called from a foreign thread while the CancelationSignal's lock + // is held. The callee should avoid performing slow or blocking operations. + virtual void OnStopRequested() = 0; +}; + +} // namespace syncer + +#endif // SYNC_INTERNAL_API_PUBLIC_BASE_CANCELATION_OBSERVER_H_ diff --git a/sync/internal_api/public/base/cancelation_signal.cc b/sync/internal_api/public/base/cancelation_signal.cc new file mode 100644 index 0000000..1ce920e --- /dev/null +++ b/sync/internal_api/public/base/cancelation_signal.cc @@ -0,0 +1,52 @@ +// Copyright 2013 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 "sync/internal_api/public/base/cancelation_signal.h" + +#include "base/logging.h" +#include "sync/internal_api/public/base/cancelation_observer.h" + +namespace syncer { + +CancelationSignal::CancelationSignal() + : stop_requested_(false), + handler_(NULL) { } + +CancelationSignal::~CancelationSignal() { + DCHECK(!handler_); +} + +bool CancelationSignal::TryRegisterHandler(CancelationObserver* handler) { + base::AutoLock lock(stop_requested_lock_); + DCHECK(!handler_); + + if (stop_requested_) + return false; + + handler_ = handler; + return true; +} + +void CancelationSignal::UnregisterHandler(CancelationObserver* handler) { + base::AutoLock lock(stop_requested_lock_); + DCHECK_EQ(handler_, handler); + handler_ = NULL; +} + +bool CancelationSignal::IsStopRequested() { + base::AutoLock lock(stop_requested_lock_); + return stop_requested_; +} + +void CancelationSignal::RequestStop() { + base::AutoLock lock(stop_requested_lock_); + DCHECK(!stop_requested_); + + stop_requested_ = true; + if (handler_) { + handler_->OnStopRequested(); + } +} + +} // namespace syncer diff --git a/sync/internal_api/public/base/cancelation_signal.h b/sync/internal_api/public/base/cancelation_signal.h new file mode 100644 index 0000000..3c16543 --- /dev/null +++ b/sync/internal_api/public/base/cancelation_signal.h @@ -0,0 +1,72 @@ +// Copyright 2013 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. + +#ifndef SYNC_INTERNAL_API_PUBLIC_BASE_CANCELATION_SIGNAL_H_ +#define SYNC_INTERNAL_API_PUBLIC_BASE_CANCELATION_SIGNAL_H_ + +#include "base/synchronization/lock.h" +#include "sync/base/sync_export.h" + +namespace syncer { + +class CancelationObserver; + +// This class is used to allow one thread to request that another abort and +// return early. +// +// The signalling thread owns this class and my call RequestStop() at any time. +// After that call, this class' IsStopRequested() will always return true. The +// intended use case is that the task intending to support early exit will +// periodically check the value of IsStopRequested() to see if it should return +// early. +// +// The receiving task may also choose to register an CancelationObserver whose +// OnStopRequested() method will be executed on the signaller's thread when +// RequestStop() is called. This may be used for sending an early Signal() to a +// WaitableEvent. The registration of the handler is necessarily racy. If +// RequestStop() is executes before TryRegisterHandler(), TryRegisterHandler() +// will not perform any registration and return false. That function's caller +// must handle this case. +// +// This class supports only one handler, though it could easily support multiple +// observers if we found a use case for such a feature. +class SYNC_EXPORT_PRIVATE CancelationSignal { + public: + CancelationSignal(); + ~CancelationSignal(); + + // Tries to register a handler to be invoked when RequestStop() is called. + // + // If RequestStop() has already been called, returns false without registering + // the handler. Returns true when the registration is successful. + // + // If the registration was successful, the handler must be unregistered with + // UnregisterHandler before this CancelationSignal is destroyed. + bool TryRegisterHandler(CancelationObserver* handler); + + // Unregisters the abort handler. + void UnregisterHandler(CancelationObserver* handler); + + // Returns true if RequestStop() has been called. + bool IsStopRequested(); + + // Sets the stop_requested_ flag and calls the OnStopRequested() method of the + // registered handler, if there is one registered at the time. + // OnStopRequested() will be called with the |stop_requested_lock_| held. + void RequestStop(); + + private: + // Protects all members of this class. + base::Lock stop_requested_lock_; + + // True if RequestStop() has been invoked. + bool stop_requested_; + + // The registered abort handler. May be NULL. + CancelationObserver* handler_; +}; + +} // namespace syncer + +#endif // SYNC_INTERNAL_API_PUBLIC_BASE_CANCELATION_SIGNAL_H_ diff --git a/sync/internal_api/public/base/cancelation_signal_unittest.cc b/sync/internal_api/public/base/cancelation_signal_unittest.cc new file mode 100644 index 0000000..6ae558c0 --- /dev/null +++ b/sync/internal_api/public/base/cancelation_signal_unittest.cc @@ -0,0 +1,168 @@ +// Copyright 2013 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 "sync/internal_api/public/base/cancelation_signal.h" + +#include "base/bind.h" +#include "base/message_loop/message_loop.h" +#include "base/synchronization/waitable_event.h" +#include "base/threading/thread.h" +#include "sync/internal_api/public/base/cancelation_observer.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace syncer { + +class BlockingTask : public CancelationObserver { + public: + BlockingTask(CancelationSignal* cancel_signal); + virtual ~BlockingTask(); + + // Starts the |exec_thread_| and uses it to execute DoRun(). + void RunAsync(base::WaitableEvent* task_done_signal); + + // Blocks until canceled. Signals |task_done_signal| when finished. + void Run(base::WaitableEvent* task_done_signal); + + // Implementation of CancelationObserver. + // Wakes up the thread blocked in Run(). + virtual void OnStopRequested() OVERRIDE; + + // Checks if we ever did successfully start waiting for |event_|. Be careful + // with this. The flag itself is thread-unsafe, and the event that flips it + // is racy. + bool WasStarted(); + + private: + base::WaitableEvent event_; + base::Thread exec_thread_; + CancelationSignal* cancel_signal_; + bool was_started_; +}; + +BlockingTask::BlockingTask(CancelationSignal* cancel_signal) + : event_(true, false), + exec_thread_("BlockingTaskBackgroundThread"), + cancel_signal_(cancel_signal), + was_started_(false) { } + +BlockingTask::~BlockingTask() {} + +void BlockingTask::RunAsync(base::WaitableEvent* task_done_signal) { + exec_thread_.Start(); + exec_thread_.message_loop()->PostTask( + FROM_HERE, + base::Bind(&BlockingTask::Run, + base::Unretained(this), + base::Unretained(task_done_signal))); +} + +void BlockingTask::Run(base::WaitableEvent* task_done_signal) { + if (cancel_signal_->TryRegisterHandler(this)) { + DCHECK(!event_.IsSignaled()); + event_.Wait(); + } + task_done_signal->Signal(); +} + +void BlockingTask::OnStopRequested() { + event_.Signal(); +} + +bool BlockingTask::WasStarted() { + return was_started_; +} + +class CancelationSignalTest : public ::testing::Test { + public: + CancelationSignalTest(); + virtual ~CancelationSignalTest(); + + // Starts the blocking task on a background thread. + void StartBlockingTask(); + + // Cancels the blocking task. + void RequestStop(); + + // Verifies that the background task is not running. This could be beacause + // it was canceled early or because it was canceled after it was started. + // + // This method may block for a brief period of time while waiting for the + // background thread to make progress. + bool VerifyTaskDone(); + + // Verifies that the background task was canceled early. + // + // This method may block for a brief period of time while waiting for the + // background thread to make progress. + bool VerifyTaskNotStarted(); + + private: + base::MessageLoop main_loop_; + + CancelationSignal signal_; + base::WaitableEvent task_done_event_; + BlockingTask blocking_task_; +}; + +CancelationSignalTest::CancelationSignalTest() + : task_done_event_(false, false), blocking_task_(&signal_) {} + +CancelationSignalTest::~CancelationSignalTest() {} + +void CancelationSignalTest::StartBlockingTask() { + blocking_task_.RunAsync(&task_done_event_); +} + +void CancelationSignalTest::RequestStop() { + signal_.RequestStop(); +} + +bool CancelationSignalTest::VerifyTaskDone() { + // Wait until BlockingTask::Run() has finished. + task_done_event_.Wait(); + return true; +} + +bool CancelationSignalTest::VerifyTaskNotStarted() { + // Wait until BlockingTask::Run() has finished. + task_done_event_.Wait(); + + // Verify the background thread never started blocking. + return !blocking_task_.WasStarted(); +} + +class FakeCancelationObserver : public CancelationObserver { + virtual void OnStopRequested() OVERRIDE { } +}; + +TEST(CancelationSignalTest_SingleThread, CheckFlags) { + FakeCancelationObserver observer; + CancelationSignal signal; + + EXPECT_FALSE(signal.IsStopRequested()); + signal.RequestStop(); + EXPECT_TRUE(signal.IsStopRequested()); + EXPECT_FALSE(signal.TryRegisterHandler(&observer)); +} + +// Send the cancelation signal before the task is started. This will ensure +// that the task will never be attempted. +TEST_F(CancelationSignalTest, CancelEarly) { + RequestStop(); + StartBlockingTask(); + EXPECT_TRUE(VerifyTaskNotStarted()); +} + +// Send the cancelation signal after the request to start the task has been +// posted. This is racy. The signal to stop may arrive before the signal to +// run the task. If that happens, we end up with another instance of the +// CancelEarly test defined earlier. If the signal requesting a stop arrives +// after the task has been started, it should end up stopping the task. +TEST_F(CancelationSignalTest, Cancel) { + StartBlockingTask(); + RequestStop(); + EXPECT_TRUE(VerifyTaskDone()); +} + +} // namespace syncer diff --git a/sync/internal_api/public/internal_components_factory.h b/sync/internal_api/public/internal_components_factory.h index 86509ec..616457d 100644 --- a/sync/internal_api/public/internal_components_factory.h +++ b/sync/internal_api/public/internal_components_factory.h @@ -21,6 +21,7 @@ namespace syncer { class ExtensionsActivity; class ServerConnectionManager; class SyncEngineEventListener; +class CancelationSignal; class SyncScheduler; class TrafficRecorder; @@ -75,7 +76,8 @@ class SYNC_EXPORT InternalComponentsFactory { virtual scoped_ptr<SyncScheduler> BuildScheduler( const std::string& name, - sessions::SyncSessionContext* context) = 0; + sessions::SyncSessionContext* context, + CancelationSignal* cancelation_signal) = 0; virtual scoped_ptr<sessions::SyncSessionContext> BuildContext( ServerConnectionManager* connection_manager, diff --git a/sync/internal_api/public/internal_components_factory_impl.h b/sync/internal_api/public/internal_components_factory_impl.h index 148dd07..7b5c697 100644 --- a/sync/internal_api/public/internal_components_factory_impl.h +++ b/sync/internal_api/public/internal_components_factory_impl.h @@ -21,7 +21,8 @@ class SYNC_EXPORT InternalComponentsFactoryImpl virtual scoped_ptr<SyncScheduler> BuildScheduler( const std::string& name, - sessions::SyncSessionContext* context) OVERRIDE; + sessions::SyncSessionContext* context, + syncer::CancelationSignal* cancelation_signal) OVERRIDE; virtual scoped_ptr<sessions::SyncSessionContext> BuildContext( ServerConnectionManager* connection_manager, diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h index 3f5316b..3a9c7a6 100644 --- a/sync/internal_api/public/sync_manager.h +++ b/sync/internal_api/public/sync_manager.h @@ -45,6 +45,7 @@ class JsEventHandler; class SyncEncryptionHandler; class SyncScheduler; struct UserShare; +class CancelationSignal; namespace sessions { class SyncSessionSnapshot; @@ -320,7 +321,8 @@ class SYNC_EXPORT SyncManager : public syncer::InvalidationHandler { Encryptor* encryptor, scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler, ReportUnrecoverableErrorFunction report_unrecoverable_error_function, - bool use_oauth2_token) = 0; + bool use_oauth2_token, + CancelationSignal* cancelation_signal) = 0; // Throw an unrecoverable error from a transaction (mostly used for // testing). @@ -392,17 +394,6 @@ class SYNC_EXPORT SyncManager : public syncer::InvalidationHandler { // to the syncapi model. virtual void SaveChanges() = 0; - // Initiates shutdown of various components in the sync engine. Must be - // called from the main thread to allow preempting ongoing tasks on the sync - // loop (that may be blocked on I/O). The semantics of |callback| are the - // same as with StartConfigurationMode. If provided and a scheduler / sync - // loop exists, it will be invoked from the sync loop by the scheduler to - // notify that all work has been flushed + cancelled, and it is idle. - // If no scheduler exists, the callback is run immediately (from the loop - // this was created on, which is the sync loop), as sync is effectively - // stopped. - virtual void StopSyncingForShutdown() = 0; - // Issue a final SaveChanges, and close sqlite handles. virtual void ShutdownOnSyncThread() = 0; diff --git a/sync/internal_api/public/test/fake_sync_manager.h b/sync/internal_api/public/test/fake_sync_manager.h index be06ea5..300921c 100644 --- a/sync/internal_api/public/test/fake_sync_manager.h +++ b/sync/internal_api/public/test/fake_sync_manager.h @@ -92,7 +92,8 @@ class FakeSyncManager : public SyncManager { Encryptor* encryptor, scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler, ReportUnrecoverableErrorFunction report_unrecoverable_error_function, - bool use_oauth2_token) OVERRIDE; + bool use_oauth2_token, + CancelationSignal* cancelation_signal) OVERRIDE; virtual void ThrowUnrecoverableError() OVERRIDE; virtual ModelTypeSet InitialSyncEndedTypes() OVERRIDE; virtual ModelTypeSet GetTypesWithEmptyProgressMarkerToken( @@ -114,7 +115,6 @@ class FakeSyncManager : public SyncManager { virtual void RemoveObserver(Observer* observer) OVERRIDE; virtual SyncStatus GetDetailedStatus() const OVERRIDE; virtual void SaveChanges() OVERRIDE; - virtual void StopSyncingForShutdown() OVERRIDE; virtual void ShutdownOnSyncThread() OVERRIDE; virtual UserShare* GetUserShare() OVERRIDE; virtual const std::string cache_guid() OVERRIDE; diff --git a/sync/internal_api/public/test/test_internal_components_factory.h b/sync/internal_api/public/test/test_internal_components_factory.h index d846094..c899676 100644 --- a/sync/internal_api/public/test/test_internal_components_factory.h +++ b/sync/internal_api/public/test/test_internal_components_factory.h @@ -27,7 +27,8 @@ class TestInternalComponentsFactory : public InternalComponentsFactory { virtual scoped_ptr<SyncScheduler> BuildScheduler( const std::string& name, - sessions::SyncSessionContext* context) OVERRIDE; + sessions::SyncSessionContext* context, + syncer::CancelationSignal* cancelation_signal) OVERRIDE; virtual scoped_ptr<sessions::SyncSessionContext> BuildContext( ServerConnectionManager* connection_manager, diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc index 091ed28..f79213d 100644 --- a/sync/internal_api/sync_manager_impl.cc +++ b/sync/internal_api/sync_manager_impl.cc @@ -19,6 +19,7 @@ #include "sync/engine/sync_scheduler.h" #include "sync/engine/syncer_types.h" #include "sync/internal_api/change_reorder_buffer.h" +#include "sync/internal_api/public/base/cancelation_signal.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/base_node.h" #include "sync/internal_api/public/configure_reason.h" @@ -354,12 +355,14 @@ void SyncManagerImpl::Init( Encryptor* encryptor, scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler, ReportUnrecoverableErrorFunction report_unrecoverable_error_function, - bool use_oauth2_token) { + bool use_oauth2_token, + CancelationSignal* cancelation_signal) { CHECK(!initialized_); DCHECK(thread_checker_.CalledOnValidThread()); DCHECK(post_factory.get()); DCHECK(!credentials.email.empty()); DCHECK(!credentials.sync_token.empty()); + DCHECK(cancelation_signal); DVLOG(1) << "SyncManager starting Init..."; weak_handle_this_ = MakeWeakHandle(weak_ptr_factory_.GetWeakPtr()); @@ -419,7 +422,7 @@ void SyncManagerImpl::Init( connection_manager_.reset(new SyncAPIServerConnectionManager( sync_server_and_path, port, use_ssl, use_oauth2_token, - post_factory.release())); + post_factory.release(), cancelation_signal)); connection_manager_->set_client_id(directory()->cache_guid()); connection_manager_->AddListener(this); @@ -447,7 +450,7 @@ void SyncManagerImpl::Init( invalidator_client_id).Pass(); session_context_->set_account_name(credentials.email); scheduler_ = internal_components_factory->BuildScheduler( - name_, session_context_.get()).Pass(); + name_, session_context_.get(), cancelation_signal).Pass(); scheduler_->Start(SyncScheduler::CONFIGURATION_MODE); @@ -618,13 +621,6 @@ void SyncManagerImpl::RemoveObserver(SyncManager::Observer* observer) { observers_.RemoveObserver(observer); } -void SyncManagerImpl::StopSyncingForShutdown() { - DVLOG(2) << "StopSyncingForShutdown"; - scheduler_->RequestStop(); - if (connection_manager_) - connection_manager_->TerminateAllIO(); -} - void SyncManagerImpl::ShutdownOnSyncThread() { DCHECK(thread_checker_.CalledOnValidThread()); diff --git a/sync/internal_api/sync_manager_impl.h b/sync/internal_api/sync_manager_impl.h index 03d6636..0283065 100644 --- a/sync/internal_api/sync_manager_impl.h +++ b/sync/internal_api/sync_manager_impl.h @@ -81,7 +81,8 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl : scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler, ReportUnrecoverableErrorFunction report_unrecoverable_error_function, - bool use_oauth2_token) OVERRIDE; + bool use_oauth2_token, + CancelationSignal* cancelation_signal) OVERRIDE; virtual void ThrowUnrecoverableError() OVERRIDE; virtual ModelTypeSet InitialSyncEndedTypes() OVERRIDE; virtual ModelTypeSet GetTypesWithEmptyProgressMarkerToken( @@ -106,7 +107,6 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl : virtual void RemoveObserver(SyncManager::Observer* observer) OVERRIDE; virtual SyncStatus GetDetailedStatus() const OVERRIDE; virtual void SaveChanges() OVERRIDE; - virtual void StopSyncingForShutdown() OVERRIDE; virtual void ShutdownOnSyncThread() OVERRIDE; virtual UserShare* GetUserShare() OVERRIDE; virtual const std::string cache_guid() OVERRIDE; diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc index 06e65e2..9eb8dc3 100644 --- a/sync/internal_api/sync_manager_impl_unittest.cc +++ b/sync/internal_api/sync_manager_impl_unittest.cc @@ -24,6 +24,7 @@ #include "base/test/values_test_util.h" #include "base/values.h" #include "sync/engine/sync_scheduler.h" +#include "sync/internal_api/public/base/cancelation_signal.h" #include "sync/internal_api/public/base/model_type_test_util.h" #include "sync/internal_api/public/change_record.h" #include "sync/internal_api/public/engine/model_safe_worker.h" @@ -835,7 +836,8 @@ class SyncManagerTest : public testing::Test, scoped_ptr<UnrecoverableErrorHandler>( new TestUnrecoverableErrorHandler).Pass(), NULL, - false); + false, + &cancelation_signal_); sync_manager_.GetEncryptionHandler()->AddObserver(&encryption_observer_); @@ -1018,6 +1020,7 @@ class SyncManagerTest : public testing::Test, protected: FakeEncryptor encryptor_; SyncManagerImpl sync_manager_; + CancelationSignal cancelation_signal_; WeakHandle<JsBackend> js_backend_; StrictMock<SyncManagerObserverMock> manager_observer_; StrictMock<SyncEncryptionHandlerObserverMock> encryption_observer_; @@ -2796,7 +2799,8 @@ class ComponentsFactory : public TestInternalComponentsFactory { virtual scoped_ptr<SyncScheduler> BuildScheduler( const std::string& name, - sessions::SyncSessionContext* context) OVERRIDE { + sessions::SyncSessionContext* context, + CancelationSignal* stop_handle) OVERRIDE { *session_context_ = context; return scheduler_to_use_.Pass(); } diff --git a/sync/internal_api/syncapi_server_connection_manager.cc b/sync/internal_api/syncapi_server_connection_manager.cc index 224d579..4c29f10 100644 --- a/sync/internal_api/syncapi_server_connection_manager.cc +++ b/sync/internal_api/syncapi_server_connection_manager.cc @@ -92,17 +92,23 @@ SyncAPIServerConnectionManager::SyncAPIServerConnectionManager( int port, bool use_ssl, bool use_oauth2_token, - HttpPostProviderFactory* factory) - : ServerConnectionManager(server, port, use_ssl, use_oauth2_token), + HttpPostProviderFactory* factory, + CancelationSignal* cancelation_signal) + : ServerConnectionManager(server, + port, + use_ssl, + use_oauth2_token, + cancelation_signal), post_provider_factory_(factory) { DCHECK(post_provider_factory_.get()); } SyncAPIServerConnectionManager::~SyncAPIServerConnectionManager() {} -ServerConnectionManager::Connection* +scoped_ptr<ServerConnectionManager::Connection> SyncAPIServerConnectionManager::MakeConnection() { - return new SyncAPIBridgedConnection(this, post_provider_factory_.get()); + return scoped_ptr<Connection>( + new SyncAPIBridgedConnection(this, post_provider_factory_.get())); } } // namespace syncer diff --git a/sync/internal_api/syncapi_server_connection_manager.h b/sync/internal_api/syncapi_server_connection_manager.h index 74312b3..8ce8dbb 100644 --- a/sync/internal_api/syncapi_server_connection_manager.h +++ b/sync/internal_api/syncapi_server_connection_manager.h @@ -12,9 +12,11 @@ #include "base/memory/scoped_ptr.h" #include "sync/base/sync_export.h" #include "sync/engine/net/server_connection_manager.h" +#include "sync/internal_api/public/base/cancelation_signal.h" namespace syncer { +class ConnectionDisconnectHandle; class HttpPostProviderFactory; class HttpPostProviderInterface; @@ -55,13 +57,16 @@ class SYNC_EXPORT_PRIVATE SyncAPIServerConnectionManager int port, bool use_ssl, bool use_oauth2_token, - HttpPostProviderFactory* factory); + HttpPostProviderFactory* factory, + CancelationSignal* cancelation_signal); virtual ~SyncAPIServerConnectionManager(); // ServerConnectionManager overrides. - virtual Connection* MakeConnection() OVERRIDE; + virtual scoped_ptr<Connection> MakeConnection() OVERRIDE; private: + FRIEND_TEST_ALL_PREFIXES( + SyncAPIServerConnectionManagerTest, VeryEarlyAbortPost); FRIEND_TEST_ALL_PREFIXES(SyncAPIServerConnectionManagerTest, EarlyAbortPost); FRIEND_TEST_ALL_PREFIXES(SyncAPIServerConnectionManagerTest, AbortPost); diff --git a/sync/internal_api/syncapi_server_connection_manager_unittest.cc b/sync/internal_api/syncapi_server_connection_manager_unittest.cc index 950cd35..18177de 100644 --- a/sync/internal_api/syncapi_server_connection_manager_unittest.cc +++ b/sync/internal_api/syncapi_server_connection_manager_unittest.cc @@ -12,6 +12,7 @@ #include "base/threading/thread.h" #include "base/time/time.h" #include "net/base/net_errors.h" +#include "sync/internal_api/public/base/cancelation_signal.h" #include "sync/internal_api/public/http_post_provider_factory.h" #include "sync/internal_api/public/http_post_provider_interface.h" #include "testing/gtest/include/gtest/gtest.h" @@ -67,14 +68,34 @@ class BlockingHttpPostFactory : public HttpPostProviderFactory { } // namespace +// Ask the ServerConnectionManager to stop before it is created. +TEST(SyncAPIServerConnectionManagerTest, VeryEarlyAbortPost) { + CancelationSignal signal; + signal.RequestStop(); + SyncAPIServerConnectionManager server( + "server", 0, true, false, new BlockingHttpPostFactory(), &signal); + + ServerConnectionManager::PostBufferParams params; + ScopedServerStatusWatcher watcher(&server, ¶ms.response); + + bool result = server.PostBufferToPath( + ¶ms, "/testpath", "testauth", &watcher); + + EXPECT_FALSE(result); + EXPECT_EQ(HttpResponse::CONNECTION_UNAVAILABLE, + params.response.server_status); +} + +// Ask the ServerConnectionManager to stop before its first request is made. TEST(SyncAPIServerConnectionManagerTest, EarlyAbortPost) { + CancelationSignal signal; SyncAPIServerConnectionManager server( - "server", 0, true, false, new BlockingHttpPostFactory()); + "server", 0, true, false, new BlockingHttpPostFactory(), &signal); ServerConnectionManager::PostBufferParams params; ScopedServerStatusWatcher watcher(&server, ¶ms.response); - server.TerminateAllIO(); + signal.RequestStop(); bool result = server.PostBufferToPath( ¶ms, "/testpath", "testauth", &watcher); @@ -83,9 +104,11 @@ TEST(SyncAPIServerConnectionManagerTest, EarlyAbortPost) { params.response.server_status); } +// Ask the ServerConnectionManager to stop during a request. TEST(SyncAPIServerConnectionManagerTest, AbortPost) { + CancelationSignal signal; SyncAPIServerConnectionManager server( - "server", 0, true, false, new BlockingHttpPostFactory()); + "server", 0, true, false, new BlockingHttpPostFactory(), &signal); ServerConnectionManager::PostBufferParams params; ScopedServerStatusWatcher watcher(&server, ¶ms.response); @@ -94,8 +117,8 @@ TEST(SyncAPIServerConnectionManagerTest, AbortPost) { ASSERT_TRUE(abort_thread.Start()); abort_thread.message_loop()->PostDelayedTask( FROM_HERE, - base::Bind(&ServerConnectionManager::TerminateAllIO, - base::Unretained(&server)), + base::Bind(&CancelationSignal::RequestStop, + base::Unretained(&signal)), TestTimeouts::tiny_timeout()); bool result = server.PostBufferToPath( diff --git a/sync/internal_api/test/fake_sync_manager.cc b/sync/internal_api/test/fake_sync_manager.cc index cac310b..362b74e 100644 --- a/sync/internal_api/test/fake_sync_manager.cc +++ b/sync/internal_api/test/fake_sync_manager.cc @@ -91,7 +91,8 @@ void FakeSyncManager::Init( Encryptor* encryptor, scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler, ReportUnrecoverableErrorFunction report_unrecoverable_error_function, - bool use_oauth2_token) { + bool use_oauth2_token, + CancelationSignal* cancelation_signal) { sync_task_runner_ = base::ThreadTaskRunnerHandle::Get(); PurgePartiallySyncedTypes(); @@ -209,9 +210,6 @@ void FakeSyncManager::SaveChanges() { // Do nothing. } -void FakeSyncManager::StopSyncingForShutdown() { -} - void FakeSyncManager::ShutdownOnSyncThread() { DCHECK(sync_task_runner_->RunsTasksOnCurrentThread()); test_user_share_.TearDown(); diff --git a/sync/internal_api/test/test_internal_components_factory.cc b/sync/internal_api/test/test_internal_components_factory.cc index 03d2d82..1493e36 100644 --- a/sync/internal_api/test/test_internal_components_factory.cc +++ b/sync/internal_api/test/test_internal_components_factory.cc @@ -22,7 +22,9 @@ TestInternalComponentsFactory::TestInternalComponentsFactory( TestInternalComponentsFactory::~TestInternalComponentsFactory() { } scoped_ptr<SyncScheduler> TestInternalComponentsFactory::BuildScheduler( - const std::string& name, sessions::SyncSessionContext* context) { + const std::string& name, + sessions::SyncSessionContext* context, + syncer::CancelationSignal* cancelation_signal) { return scoped_ptr<SyncScheduler>(new FakeSyncScheduler()); } diff --git a/sync/sync_internal_api.gypi b/sync/sync_internal_api.gypi index a22195a..be3ce71 100644 --- a/sync/sync_internal_api.gypi +++ b/sync/sync_internal_api.gypi @@ -32,6 +32,10 @@ 'internal_api/js_sync_encryption_handler_observer.h', 'internal_api/js_sync_manager_observer.cc', 'internal_api/js_sync_manager_observer.h', + 'internal_api/public/base/cancelation_observer.cc', + 'internal_api/public/base/cancelation_observer.h', + 'internal_api/public/base/cancelation_signal.cc', + 'internal_api/public/base/cancelation_signal.h', 'internal_api/public/base/enum_set.h', 'internal_api/public/base/invalidation.cc', 'internal_api/public/base/invalidation.h', @@ -73,9 +77,9 @@ 'internal_api/public/sessions/sync_session_snapshot.h', 'internal_api/public/sync_encryption_handler.cc', 'internal_api/public/sync_encryption_handler.h', - 'internal_api/public/sync_manager_factory.h', 'internal_api/public/sync_manager.cc', 'internal_api/public/sync_manager.h', + 'internal_api/public/sync_manager_factory.h', 'internal_api/public/user_share.h', 'internal_api/public/util/experiments.h', 'internal_api/public/util/immutable.h', diff --git a/sync/sync_tests.gypi b/sync/sync_tests.gypi index 07165af..93d6ed8 100644 --- a/sync/sync_tests.gypi +++ b/sync/sync_tests.gypi @@ -226,6 +226,7 @@ '..', ], 'sources': [ + 'internal_api/public/base/cancelation_signal_unittest.cc', 'internal_api/public/base/enum_set_unittest.cc', 'internal_api/public/base/node_ordinal_unittest.cc', 'internal_api/public/base/ordinal_unittest.cc', diff --git a/sync/test/engine/fake_sync_scheduler.cc b/sync/test/engine/fake_sync_scheduler.cc index 1e0c7e8..5d45494 100644 --- a/sync/test/engine/fake_sync_scheduler.cc +++ b/sync/test/engine/fake_sync_scheduler.cc @@ -13,7 +13,7 @@ FakeSyncScheduler::~FakeSyncScheduler() {} void FakeSyncScheduler::Start(Mode mode) { } -void FakeSyncScheduler::RequestStop() { +void FakeSyncScheduler::Stop() { } void FakeSyncScheduler::ScheduleLocalNudge( diff --git a/sync/test/engine/fake_sync_scheduler.h b/sync/test/engine/fake_sync_scheduler.h index 11a63cc..95bdfa9 100644 --- a/sync/test/engine/fake_sync_scheduler.h +++ b/sync/test/engine/fake_sync_scheduler.h @@ -20,7 +20,7 @@ class FakeSyncScheduler : public SyncScheduler { virtual ~FakeSyncScheduler(); virtual void Start(Mode mode) OVERRIDE; - virtual void RequestStop() OVERRIDE; + virtual void Stop() OVERRIDE; virtual void ScheduleLocalNudge( const base::TimeDelta& desired_delay, ModelTypeSet types, diff --git a/sync/test/engine/mock_connection_manager.cc b/sync/test/engine/mock_connection_manager.cc index 5bf2b4b..6668638 100644 --- a/sync/test/engine/mock_connection_manager.cc +++ b/sync/test/engine/mock_connection_manager.cc @@ -34,7 +34,7 @@ static char kValidAuthToken[] = "AuthToken"; static char kCacheGuid[] = "kqyg7097kro6GSUod+GSg=="; MockConnectionManager::MockConnectionManager(syncable::Directory* directory) - : ServerConnectionManager("unused", 0, false, false), + : ServerConnectionManager("unused", 0, false, false, NULL), server_reachable_(true), conflict_all_commits_(false), conflict_n_commits_(0), diff --git a/sync/test/engine/mock_connection_manager.h b/sync/test/engine/mock_connection_manager.h index 83de59a..d82a1bf 100644 --- a/sync/test/engine/mock_connection_manager.h +++ b/sync/test/engine/mock_connection_manager.h @@ -15,6 +15,7 @@ #include "base/callback.h" #include "base/compiler_specific.h" #include "base/memory/scoped_vector.h" +#include "base/synchronization/lock.h" #include "sync/engine/net/server_connection_manager.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/base/unique_position.h" diff --git a/sync/tools/sync_client.cc b/sync/tools/sync_client.cc index b677531..6526516 100644 --- a/sync/tools/sync_client.cc +++ b/sync/tools/sync_client.cc @@ -27,6 +27,7 @@ #include "net/dns/host_resolver.h" #include "net/http/transport_security_state.h" #include "net/url_request/url_request_test_util.h" +#include "sync/internal_api/public/base/cancelation_signal.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/base_node.h" #include "sync/internal_api/public/engine/passive_model_worker.h" @@ -349,6 +350,7 @@ int SyncClientMain(int argc, char* argv[]) { InternalComponentsFactory::ENCRYPTION_KEYSTORE, InternalComponentsFactory::BACKOFF_NORMAL }; + CancelationSignal cancelation_signal; sync_manager->Init(database_dir.path(), WeakHandle<JsEventHandler>( @@ -368,7 +370,8 @@ int SyncClientMain(int argc, char* argv[]) { &null_encryptor, scoped_ptr<UnrecoverableErrorHandler>( new LoggingUnrecoverableErrorHandler).Pass(), - &LogUnrecoverableErrorContext, false); + &LogUnrecoverableErrorContext, false, + &cancelation_signal); // TODO(akalin): Avoid passing in model parameters multiple times by // organizing handling of model types. invalidator->UpdateCredentials(credentials.email, credentials.sync_token); |