summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorrlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2013-09-10 00:39:48 +0000
committerrlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2013-09-10 00:39:48 +0000
commit33c1f53914a7a15b51cb226f572f5439e8ef9249 (patch)
tree553286bc9bc1966e753bbc6e1f703fbaab96f886
parent98f936181964f99cd0c73707cfd9027d0c528702 (diff)
downloadchromium_src-33c1f53914a7a15b51cb226f572f5439e8ef9249.zip
chromium_src-33c1f53914a7a15b51cb226f572f5439e8ef9249.tar.gz
chromium_src-33c1f53914a7a15b51cb226f572f5439e8ef9249.tar.bz2
sync: Gracefully handle very early shutdown
Introduce a new object to communicate cross-thread cancellation signals. This new object, the CancellationSignal, is protected by a lock. It allows the receiving thread to query whether or not a stop has been requested. It also allows the receiving thread to safely register a cross-thread callback to be invoked immediately when a stop is requested. This class is used to reimplement the UI thread to sync thread early shutdown signal. Previously, the UI thread would try to call in to objects owned by the sync thread. This required a workaround if the signal arrived very early, since we couldn't guarantee the sync thread had actually created those objects until later. The CancellationSignal is owned by the UI thread, so it is safe to call its RequestStop() at any point during sync initialization. The sync thread will receive the signal when it's ready. The new scheme has a few advantages over the old: - Thread ownership is simpler. The SyncBackendHost::Core, SyncManager, ServerConnectionManager, SyncScheduler and Syncer can now claim that all their member functions run on the sync thread. - We no longer need to implement special case logic for when a shutdown is requested before the SyncManager has initialized. - In a future CL, we can take advantage of the fact that we no longer require the special case to reduce inter-thread communication during sync startup. This will make startup simpler and, in some cases, improve sync startup time by as much as a few hundred milliseconds. - This will make it easier to address crbug.com/236451. BUG=236451 Review URL: https://chromiumcodereview.appspot.com/23189021 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@222154 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r--chrome/browser/sync/glue/sync_backend_host.cc41
-rw-r--r--chrome/browser/sync/glue/sync_backend_host.h18
-rw-r--r--sync/engine/net/server_connection_manager.cc77
-rw-r--r--sync/engine/net/server_connection_manager.h59
-rw-r--r--sync/engine/sync_scheduler.h8
-rw-r--r--sync/engine/sync_scheduler_impl.cc18
-rw-r--r--sync/engine/sync_scheduler_impl.h5
-rw-r--r--sync/engine/sync_scheduler_unittest.cc4
-rw-r--r--sync/engine/syncer.cc13
-rw-r--r--sync/engine/syncer.h10
-rw-r--r--sync/engine/syncer_proto_util_unittest.cc2
-rw-r--r--sync/engine/syncer_unittest.cc4
-rw-r--r--sync/internal_api/internal_components_factory_impl.cc11
-rw-r--r--sync/internal_api/public/base/cancelation_observer.cc13
-rw-r--r--sync/internal_api/public/base/cancelation_observer.h25
-rw-r--r--sync/internal_api/public/base/cancelation_signal.cc52
-rw-r--r--sync/internal_api/public/base/cancelation_signal.h72
-rw-r--r--sync/internal_api/public/base/cancelation_signal_unittest.cc168
-rw-r--r--sync/internal_api/public/internal_components_factory.h4
-rw-r--r--sync/internal_api/public/internal_components_factory_impl.h3
-rw-r--r--sync/internal_api/public/sync_manager.h15
-rw-r--r--sync/internal_api/public/test/fake_sync_manager.h4
-rw-r--r--sync/internal_api/public/test/test_internal_components_factory.h3
-rw-r--r--sync/internal_api/sync_manager_impl.cc16
-rw-r--r--sync/internal_api/sync_manager_impl.h4
-rw-r--r--sync/internal_api/sync_manager_impl_unittest.cc8
-rw-r--r--sync/internal_api/syncapi_server_connection_manager.cc14
-rw-r--r--sync/internal_api/syncapi_server_connection_manager.h9
-rw-r--r--sync/internal_api/syncapi_server_connection_manager_unittest.cc33
-rw-r--r--sync/internal_api/test/fake_sync_manager.cc6
-rw-r--r--sync/internal_api/test/test_internal_components_factory.cc4
-rw-r--r--sync/sync_internal_api.gypi6
-rw-r--r--sync/sync_tests.gypi1
-rw-r--r--sync/test/engine/fake_sync_scheduler.cc2
-rw-r--r--sync/test/engine/fake_sync_scheduler.h2
-rw-r--r--sync/test/engine/mock_connection_manager.cc2
-rw-r--r--sync/test/engine/mock_connection_manager.h1
-rw-r--r--sync/tools/sync_client.cc5
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, &params.response);
+
+ bool result = server.PostBufferToPath(
+ &params, "/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, &params.response);
- server.TerminateAllIO();
+ signal.RequestStop();
bool result = server.PostBufferToPath(
&params, "/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, &params.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);