Skip to content

Commit

Permalink
sync: Gracefully handle very early shutdown
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
rlarocque@chromium.org committed Sep 10, 2013
1 parent 98f9361 commit 33c1f53
Show file tree
Hide file tree
Showing 38 changed files with 531 additions and 211 deletions.
41 changes: 12 additions & 29 deletions chrome/browser/sync/glue/sync_backend_host.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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());
}

Expand Down Expand Up @@ -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;
Expand All @@ -521,7 +506,7 @@ void SyncBackendHost::StopSyncingForShutdown() {

registrar_->RequestWorkerStopOnUIThread();

StopSyncManagerForShutdown();
cancelation_signal_.RequestStop();
}

scoped_ptr<base::Thread> SyncBackendHost::Shutdown(ShutdownOption option) {
Expand Down Expand Up @@ -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),
Expand All @@ -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() {}
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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.
Expand Down
18 changes: 9 additions & 9 deletions chrome/browser/sync/glue/sync_backend_host.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand All @@ -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.
Expand Down Expand Up @@ -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_;
Expand Down Expand Up @@ -589,6 +587,8 @@ class SyncBackendHost
invalidation::InvalidationService* invalidator_;
bool invalidation_handler_registered_;

syncer::CancelationSignal cancelation_signal_;

DISALLOW_COPY_AND_ASSIGN(SyncBackendHost);
};

Expand Down
77 changes: 32 additions & 45 deletions sync/engine/net/server_connection_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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*
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
Loading

0 comments on commit 33c1f53

Please sign in to comment.