sync: Gracefully handle very early shutdown

sync/engine/net/server_connection_manager.cc

 // Copyright (c) 2012 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/engine/net/server_connection_manager.h"
 
 #include <errno.h>
 
 #include <ostream>
 #include <string>
 #include <vector>
 
 #include "base/metrics/histogram.h"
 #include "build/build_config.h"
 #include "net/base/net_errors.h"
 #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"
 
 namespace syncer {
 
 using std::ostream;
 using std::string;
 using std::vector;
 
@@ skipping 78 matching lines @@
   if (bytes_read != response->content_length) {
     LOG(ERROR) << "Mismatched content lengths, server claimed " <<
         response->content_length << ", but sent " << bytes_read;
     response->server_status = HttpResponse::IO_ERROR;
     return false;
   }
   return true;
 }
 
 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*
 ServerConnectionManager::ScopedConnectionHelper::get() {
   return connection_.get();
 }
 
 namespace {
 
 string StripTrailingSlash(const string& s) {
@@ skipping 36 matching lines @@
 }
 
 ScopedServerStatusWatcher::~ScopedServerStatusWatcher() {
   conn_mgr_->SetServerStatus(response_->server_status);
 }
 
 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) {
     auth_token_.assign(auth_token);
     previously_invalidated_token = std::string();
     return true;
   }
   return false;
 }
 
@@ skipping 44 matching lines @@
   // workaround for M29 blocker to avoid sending RPC to sync with known invalid
   // token but instead to trigger refreshing token in ProfileSyncService. Need
   // to clean it.
   if (auth_token.empty() || auth_token == "credentials_lost") {
     params->response.server_status = HttpResponse::SYNC_AUTH_ERROR;
     return false;
   }
 
   // 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;
   }
 
   // Note that |post| may be aborted by now, which will just cause Init to fail
   // with CONNECTION_UNAVAILABLE.
   bool ok = post.get()->Init(
       path.c_str(), auth_token, params->buffer_in, &params->response);
 
@@ skipping 44 matching lines @@
   DCHECK(thread_checker_.CalledOnValidThread());
   listeners_.AddObserver(listener);
 }
 
 void ServerConnectionManager::RemoveListener(
     ServerConnectionEventListener* listener) {
   DCHECK(thread_checker_.CalledOnValidThread());
   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) {
   s << " Response Code (bogus on error): " << hr.response_code;
   s << " Content-Length (bogus on error): " << hr.content_length;
   s << " Server Status: "
     << HttpResponse::GetServerConnectionCodeString(hr.server_status);
   return s;
 }
 
 }  // namespace syncer