sync: take 2 at aborting active HTTP requests.

chrome/browser/sync/engine/net/server_connection_manager.cc

Index: chrome/browser/sync/engine/net/server_connection_manager.cc
diff --git a/chrome/browser/sync/engine/net/server_connection_manager.cc b/chrome/browser/sync/engine/net/server_connection_manager.cc
index 9f0b5518e63878e8c9b34964d66da28b0d4d3e5e..bceb18713f1e6e192b78d169917e8f2cdb6e9edd 100644
--- a/chrome/browser/sync/engine/net/server_connection_manager.cc
+++ b/chrome/browser/sync/engine/net/server_connection_manager.cc
@@ -58,7 +58,14 @@ const char* HttpResponse::GetServerConnectionCodeString(
 
 #undef ENUM_CASE
 
-bool ServerConnectionManager::Post::ReadBufferResponse(
+ServerConnectionManager::Connection::Connection(
+    ServerConnectionManager* scm) : scm_(scm) {
+}
+
+ServerConnectionManager::Connection::~Connection() {
+}
+
+bool ServerConnectionManager::Connection::ReadBufferResponse(
     string* buffer_out,
     HttpResponse* response,
     bool require_response) {
@@ -79,7 +86,7 @@ bool ServerConnectionManager::Post::ReadBufferResponse(
   return true;
 }
 
-bool ServerConnectionManager::Post::ReadDownloadResponse(
+bool ServerConnectionManager::Connection::ReadDownloadResponse(
     HttpResponse* response,
     string* buffer_out) {
   const int64 bytes_read = ReadResponse(buffer_out,
@@ -94,6 +101,21 @@ bool ServerConnectionManager::Post::ReadDownloadResponse(
   return true;
 }
 
+ServerConnectionManager::ScopedConnectionHelper::ScopedConnectionHelper(
+    ServerConnectionManager* manager, Connection* connection)
+    : manager_(manager), connection_(connection) {}
+
+ServerConnectionManager::ScopedConnectionHelper::~ScopedConnectionHelper() {
+  if (connection_.get())
+    manager_->OnConnectionDestroyed(connection_.get());
+  connection_.reset();
+}
+
+ServerConnectionManager::Connection*
+ServerConnectionManager::ScopedConnectionHelper::get() {
+  return connection_.get();
+}
+
 namespace {
 
 string StripTrailingSlash(const string& s) {
@@ -108,7 +130,7 @@ string StripTrailingSlash(const string& s) {
 }  // namespace
 
 // TODO(chron): Use a GURL instead of string concatenation.
-string ServerConnectionManager::Post::MakeConnectionURL(
+string ServerConnectionManager::Connection::MakeConnectionURL(
     const string& sync_server,
     const string& path,
     bool use_ssl) const {
@@ -120,8 +142,8 @@ string ServerConnectionManager::Post::MakeConnectionURL(
   return connection_url;
 }
 
-int ServerConnectionManager::Post::ReadResponse(string* out_buffer,
-                                                int length) {
+int ServerConnectionManager::Connection::ReadResponse(string* out_buffer,
+                                                      int length) {
   int bytes_read = buffer_.length();
   CHECK(length <= bytes_read);
   out_buffer->assign(buffer_);
@@ -160,14 +182,39 @@ ServerConnectionManager::ServerConnectionManager(
       get_time_path_(kSyncServerGetTimePath),
       error_count_(0),
       server_status_(HttpResponse::NONE),
-      server_reachable_(false) {
+      server_reachable_(false),
+      terminated_(false),
+      active_connection_(NULL) {
 }
 
 ServerConnectionManager::~ServerConnectionManager() {
 }
 
+ServerConnectionManager::Connection*
+ServerConnectionManager::MakeActiveConnection() {
+  base::AutoLock lock(terminate_connection_lock_);
+  DCHECK(!active_connection_);
+  if (terminated_)
+    return NULL;
+
+  active_connection_ = MakeConnection();

[jar (doing other things), 2011/09/07 06:47:41]

It is usually best practice to use locks to only protect data, and not to
protect code (such as this call to MakeConnection()). 

Calling code (while a lock is held) can often lead to a deadlock.  To prevent
such accidents, when you absolutely have to call code, it is good to note in the
code name that you ahve a lock held.  This prevents accidental efforts to
re-acquire the lock... and makes it hard to call arbitrary code (leading to
deadlocks).

Can you comment on this call, explaining why it is safe to make while the lock
is held, and why it doesn't have any potential for blocking on anything else?

[tim (not reviewing), 2011/09/07 14:46:56]

Thanks for your review Jim.  I get the concerns with deadlocks particularly in
this release.  I'll address some of your fears by saying that this particular
case is quite well understood by this point (it has been around since M5) and is
not the likely culprit for the hangs of doom on m14 (with the history thread /
pref writing etc).  I'll try to address the rest of your fears with more context
-

The lock protects two pieces of data: active_connection_ and terminated_. This
is so that we can safely abort active_connection_, which means assigning to it
when a new connection is created / put in use, and nulling it out when the
connection is done / destroyed.

If you peek at http://codereview.chromium.org/7792022/,  first review comment,
you'll notice that in the original patch the lock was explicitly around setting
/ checking those pieces of data, but the suggestion was to move it into
MakeConnection so that we didn't have to write the same locking logic in two
places.

The only place where we're protecting a call is when using the protected data
(active_connection_) for the actual call to Abort, because that object could
wind up being destroyed if we release the lock.  In this case, the object in
question is the Connection.  That object is designed to have clear and scoped
use by only the ServerConnectionManager, and the public APIs of both of those
objects are in general to be used only from one thread (see thread_checker_ use)
with the exception of aborting.  There is only ever zero or one Connection
object in in existence, and it's lifetime is very clear / well known.  Speaking
as someone who works on this code frequently, adding ref counting and making the
destruction of Connection nondeterministic would have made this code much harder
to follow.

+  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;
+}
+
 void ServerConnectionManager::NotifyStatusChanged() {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   FOR_EACH_OBSERVER(ServerConnectionEventListener, listeners_,
      OnServerConnectionEvent(
          ServerConnectionEvent(server_status_, server_reachable_)));
@@ -175,7 +222,7 @@ void ServerConnectionManager::NotifyStatusChanged() {
 
 bool ServerConnectionManager::PostBufferWithCachedAuth(
     PostBufferParams* params, ScopedServerStatusWatcher* watcher) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   string path =
       MakeSyncServerPath(proto_sync_path(), MakeSyncQueryString(client_id_));
   return PostBufferToPath(params, path, auth_token(), watcher);
@@ -184,7 +231,7 @@ bool ServerConnectionManager::PostBufferWithCachedAuth(
 bool ServerConnectionManager::PostBufferToPath(PostBufferParams* params,
     const string& path, const string& auth_token,
     ScopedServerStatusWatcher* watcher) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(watcher != NULL);
 
   if (auth_token.empty()) {
@@ -192,9 +239,18 @@ bool ServerConnectionManager::PostBufferToPath(PostBufferParams* params,
     return false;
   }
 
-  scoped_ptr<Post> post(MakePost());
-  bool ok = post->Init(path.c_str(), auth_token, params->buffer_in,
-                       &params->response);
+  // When our connection object falls out of scope, it clears itself from
+  // active_connection_.
+  ScopedConnectionHelper post(this, MakeActiveConnection());
+  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);
 
   if (params->response.server_status == HttpResponse::SYNC_AUTH_ERROR) {
     InvalidateAndClearAuthToken();
@@ -205,7 +261,8 @@ bool ServerConnectionManager::PostBufferToPath(PostBufferParams* params,
     return false;
   }
 
-  if (post->ReadBufferResponse(&params->buffer_out, &params->response, true)) {
+  if (post.get()->ReadBufferResponse(
+      &params->buffer_out, &params->response, true)) {
     params->response.server_status = HttpResponse::SERVER_CONNECTION_OK;
     server_reachable_ = true;
     return true;
@@ -214,7 +271,8 @@ bool ServerConnectionManager::PostBufferToPath(PostBufferParams* params,
 }
 
 bool ServerConnectionManager::CheckTime(int32* out_time) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   // Verify that the server really is reachable by checking the time. We need
   // to do this because of wifi interstitials that intercept messages from the
   // client and return HTTP OK instead of a redirect.
@@ -223,7 +281,9 @@ bool ServerConnectionManager::CheckTime(int32* out_time) {
   string post_body = "command=get_time";
 
   for (int i = 0 ; i < 3;  i++) {
-    scoped_ptr<Post> post(MakePost());
+    ScopedConnectionHelper post(this, MakeActiveConnection());
+    if (!post.get())
+      break;
 
     // Note that the server's get_time path doesn't require authentication.
     string get_time_path =
@@ -231,7 +291,7 @@ bool ServerConnectionManager::CheckTime(int32* out_time) {
     VLOG(1) << "Requesting get_time from:" << get_time_path;
 
     string blank_post_body;
-    bool ok = post->Init(get_time_path.c_str(), blank_post_body,
+    bool ok = post.get()->Init(get_time_path.c_str(), blank_post_body,
         blank_post_body, &response);
     if (!ok) {
       VLOG(1) << "Unable to check the time";
@@ -240,7 +300,7 @@ bool ServerConnectionManager::CheckTime(int32* out_time) {
     string time_response;
     time_response.resize(
         static_cast<string::size_type>(response.content_length));
-    ok = post->ReadDownloadResponse(&response, &time_response);
+    ok = post.get()->ReadDownloadResponse(&response, &time_response);
     if (!ok || string::npos !=
         time_response.find_first_not_of("0123456789")) {
       LOG(ERROR) << "unable to read a non-numeric response from get_time:"
@@ -256,18 +316,18 @@ bool ServerConnectionManager::CheckTime(int32* out_time) {
 }
 
 bool ServerConnectionManager::IsServerReachable() {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   int32 time;
   return CheckTime(&time);
 }
 
 bool ServerConnectionManager::IsUserAuthenticated() {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   return IsGoodReplyFromServer(server_status_);
 }
 
 bool ServerConnectionManager::CheckServerReachable() {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   const bool server_is_reachable = IsServerReachable();
   if (server_reachable_ != server_is_reachable) {
     server_reachable_ = server_is_reachable;
@@ -277,7 +337,7 @@ bool ServerConnectionManager::CheckServerReachable() {
 }
 
 bool ServerConnectionManager::IncrementErrorCount() {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   error_count_++;
 
   if (error_count_ > kMaxConnectionErrorsBeforeReset) {
@@ -298,7 +358,7 @@ bool ServerConnectionManager::IncrementErrorCount() {
 void ServerConnectionManager::SetServerParameters(const string& server_url,
                                                   int port,
                                                   bool use_ssl) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   sync_server_ = server_url;
   sync_server_port_ = port;
   use_ssl_ = use_ssl;
@@ -333,20 +393,32 @@ std::string ServerConnectionManager::GetServerHost() const {
 
 void ServerConnectionManager::AddListener(
     ServerConnectionEventListener* listener) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   listeners_.AddObserver(listener);
 }
 
 void ServerConnectionManager::RemoveListener(
     ServerConnectionEventListener* listener) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   listeners_.RemoveObserver(listener);
 }
 
-ServerConnectionManager::Post* ServerConnectionManager::MakePost() {
+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;
+}
+
 bool FillMessageWithShareDetails(sync_pb::ClientToServerMessage* csm,
                                  syncable::DirectoryManager* manager,
                                  const std::string& share) {