sync: take 2 at aborting active HTTP requests.

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

 // Copyright (c) 2011 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 "chrome/browser/sync/engine/net/server_connection_manager.h"
 
 #include <errno.h>
 
 #include <ostream>
 #include <string>
@@ skipping 40 matching lines @@
     ENUM_CASE(SYNC_AUTH_ERROR);
     ENUM_CASE(SERVER_CONNECTION_OK);
     ENUM_CASE(RETRY);
   }
   NOTREACHED();
   return "";
 }
 
 #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) {
   if (RC_REQUEST_OK != response->response_code) {
     response->server_status = HttpResponse::SYNC_SERVER_ERROR;
     return false;
   }
 
   if (require_response && (1 > response->content_length))
     return false;
 
   const int64 bytes_read = ReadResponse(buffer_out,
       static_cast<int>(response->content_length));
   if (bytes_read != response->content_length) {
     response->server_status = HttpResponse::IO_ERROR;
     return false;
   }
   return true;
 }
 
-bool ServerConnectionManager::Post::ReadDownloadResponse(
+bool ServerConnectionManager::Connection::ReadDownloadResponse(
     HttpResponse* response,
     string* buffer_out) {
   const int64 bytes_read = ReadResponse(buffer_out,
       static_cast<int>(response->content_length));
 
   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) {}
+
+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) {
   int stripped_end_pos = s.size();
   if (s.at(stripped_end_pos - 1) == '/') {
     stripped_end_pos = stripped_end_pos - 1;
   }
 
   return s.substr(0, stripped_end_pos);
 }
 
 }  // 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 {
   string connection_url = (use_ssl ? "https://" : "http://");
   connection_url += sync_server;
   connection_url = StripTrailingSlash(connection_url);
   connection_url += path;
 
   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_);
   return bytes_read;
 }
 
 ScopedServerStatusWatcher::ScopedServerStatusWatcher(
     ServerConnectionManager* conn_mgr, HttpResponse* response)
     : conn_mgr_(conn_mgr),
       response_(response),
@@ skipping 18 matching lines @@
     bool use_ssl,
     const string& user_agent)
     : sync_server_(server),
       sync_server_port_(port),
       user_agent_(user_agent),
       use_ssl_(use_ssl),
       proto_sync_path_(kSyncServerSyncPath),
       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_)));
 }
 
 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);
 }
 
 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()) {
     params->response.server_status = HttpResponse::SYNC_AUTH_ERROR;
     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();
   }
 
   if (!ok || RC_REQUEST_OK != params->response.response_code) {
     IncrementErrorCount();
     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;
   }
   return false;
 }
 
 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.
   HttpResponse response;
   ScopedServerStatusWatcher watcher(this, &response);
   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 =
         MakeSyncServerPath(kSyncServerGetTimePath, post_body);
     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";
       continue;
     }
     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:"
             << time_response;
       continue;
     }
     *out_time = atoi(time_response.c_str());
     VLOG(1) << "Server was reachable.";
     return true;
   }
   IncrementErrorCount();
   return false;
 }
 
 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;
     NotifyStatusChanged();
   }
   return server_is_reachable;
 }
 
 bool ServerConnectionManager::IncrementErrorCount() {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   error_count_++;
 
   if (error_count_ > kMaxConnectionErrorsBeforeReset) {
     error_count_ = 0;
 
     if (!IsServerReachable()) {
       LOG(WARNING) << "Too many connection failures, server is not reachable. "
                    << "Resetting connections.";
     } else {
       LOG(WARNING) << "Multiple connection failures while server is reachable.";
     }
     return false;
   }
 
   return true;
 }
 
 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;
 }
 
 // Returns the current server parameters in server_url and port.
 void ServerConnectionManager::GetServerParameters(string* server_url,
                                                   int* port,
                                                   bool* use_ssl) const {
   if (server_url != NULL)
@@ skipping 14 matching lines @@
     return std::string();
   // We just want the hostname, so we don't need to switch on use_ssl.
   server_url = "http://" + server_url;
   GURL gurl(server_url);
   DCHECK(gurl.is_valid()) << gurl;
   return gurl.host();
 }
 
 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) {
   syncable::ScopedDirLookup dir(manager, share);
   if (!dir.good()) {
     VLOG(1) << "Dir lookup failed";
     return false;
   }
   string birthday = dir->store_birthday();
   if (!birthday.empty())
     csm->set_store_birthday(birthday);
   csm->set_share(share);
   return true;
 }
 
 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: " << hr.server_status;
   return s;
 }
 
 }  // namespace browser_sync