sync: Gracefully handle very early shutdown

sync/engine/net/server_connection_manager.cc

Index: sync/engine/net/server_connection_manager.cc
diff --git a/sync/engine/net/server_connection_manager.cc b/sync/engine/net/server_connection_manager.cc
index 0076543374fcdcedae2a86bfe62f2a1491ecb5e0..b3c78476d2a4673b2fe15db33e1cfc0fbf29898a 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,28 @@ 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()) {
+  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.

[tim (not reviewing), 2013/09/06 21:47:34]

Is this true in tests as well?  The SCM::MakeConnection returns NULL?

[rlarocque, 2013/09/06 22:42:53]

Good point.  I wasn't thinking about the tests.  I've added a special case to
the constructor to handle it.

Ideally, production code shouldn't have to care about test behavior, but let's
save that issue for another day.

+  if (connection_) {

[tim (not reviewing), 2013/09/06 21:47:34]

I'd rather use connection_.get() so the scoped_ptr is clear or (better, I think)
explicitly do connection_.reset() below, it's just less subtlety in already
tricky code.

[rlarocque, 2013/09/06 22:42:53]

I've changed this to connection_.get().

What do you mean by explicit connection_.reset()?  I don't see anything wrong
with having the scoped_ptr cleared in the destructor.

+    // 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 +193,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 +271,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 +336,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) {