sync: abort active HTTP requests on shutdown.

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 8f00369cdb3e1fa6bfcf11af6004d8b81b216abe..d4e01386e3184990611135cd058bd9a1e8076fc9 100644
--- a/chrome/browser/sync/engine/net/server_connection_manager.cc
+++ b/chrome/browser/sync/engine/net/server_connection_manager.cc
@@ -10,6 +10,7 @@
 #include <string>
 #include <vector>
 
+#include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "build/build_config.h"
 #include "chrome/browser/sync/engine/net/url_translator.h"
@@ -58,7 +59,14 @@ const char* HttpResponse::GetServerConnectionCodeString(
 
 #undef ENUM_CASE
 
-bool ServerConnectionManager::Post::ReadBufferResponse(
+ServerConnectionManager::Connection::Connection(
+    ServerConnectionManager* scm) : scm_(scm), timing_info_(0) {
+}
+
+ServerConnectionManager::Connection::~Connection() {
+}
+
+bool ServerConnectionManager::Connection::ReadBufferResponse(
     string* buffer_out,
     HttpResponse* response,
     bool require_response) {
@@ -79,7 +87,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,
@@ -108,7 +116,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 +128,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 +168,16 @@ ServerConnectionManager::ServerConnectionManager(
       get_time_path_(kSyncServerGetTimePath),
       error_count_(0),
       server_status_(HttpResponse::NONE),
-      server_reachable_(false) {
+      server_reachable_(false),
+      terminated_(false),
+      active_connection_(false) {
 }
 
 ServerConnectionManager::~ServerConnectionManager() {
 }
 
 void ServerConnectionManager::NotifyStatusChanged() {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   FOR_EACH_OBSERVER(ServerConnectionEventListener, listeners_,
      OnServerConnectionEvent(
          ServerConnectionEvent(server_status_, server_reachable_)));
@@ -175,7 +185,7 @@ void ServerConnectionManager::NotifyStatusChanged() {
 
 bool ServerConnectionManager::PostBufferWithCachedAuth(
     const 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 +194,7 @@ bool ServerConnectionManager::PostBufferWithCachedAuth(
 bool ServerConnectionManager::PostBufferToPath(const 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,10 +202,29 @@ bool ServerConnectionManager::PostBufferToPath(const PostBufferParams* params,
     return false;
   }
 
-  scoped_ptr<Post> post(MakePost());
-  post->set_timing_info(params->timing_info);
-  bool ok = post->Init(path.c_str(), auth_token, params->buffer_in,
-                       params->response);
+  // The AutoReset is to temporarily bind active_connection_ to the local
+  // |post|. It's done this way to protect against a race between a call
+  // to Abort and assignment of active_connection_.

[akalin, 2011/08/31 07:20:39]

I don't get it.  What race?  This still seems racy, since the AutoReset goes out
of scope without the lock being held.

[tim (not reviewing), 2011/08/31 14:46:17]

We have to make sure that if terminate is called, we either have an
active_connection_ set and we're using it in here in an aborted state, or we
have not yet created it so we can bail out before we create.  The AutoReset was
so that I didn't have to add a Clear / reset... call at every early return point
from this function, which you'd have to do if you had a ClearActiveConnection. 
You could maybe just call that after Init, but we still interact with the post
after that point already so that seems weird.

An alternative is wiring up a call to Clear... in the dtor for Connection.  I
can look into that.

+  scoped_ptr<Connection> post;
+  scoped_ptr<AutoReset<Connection*> > active_connection_resetter;
+  {

[akalin, 2011/08/31 07:20:39]

I suggest making MakeConnection() (or some non-virtual wrapper function):

- Hold the lock
- Check the terminated flag, and return NULL if terminated
- Otherwise, create a post and set active connection to it
- Return the post

also have a ClearActiveConnection() helper function that:
- holds the lock
- Clears active_connection_

Then you can just do:

scoped_ptr<Connection> post(MakeActiveConnection());

if (!post.get()) {
  ...
  return false;
}

bool ok = post->Init(...);

...

+    base::AutoLock lock(terminate_connection_lock_);
+    if (terminated_) {
+      params->response->server_status = HttpResponse::CONNECTION_UNAVAILABLE;
+      return false;
+    }
+
+    DCHECK(!active_connection_);
+    post.reset(MakeConnection());
+    active_connection_resetter.reset(
+        new AutoReset<Connection*>(&active_connection_, post.get()));
+    post->set_timing_info(params->timing_info);
+  }
+
+  // Note that |post| may be aborted by now, which will just cause Init to fail
+  // with CONNECTION_UNAVAILABLE.
+  bool ok = active_connection_->Init(

[akalin, 2011/08/31 07:20:39]

clearer to do post->Init() here, I think

+      path.c_str(), auth_token, params->buffer_in, params->response);
 
   if (params->response->server_status == HttpResponse::SYNC_AUTH_ERROR) {
     InvalidateAndClearAuthToken();
@@ -215,7 +244,8 @@ bool ServerConnectionManager::PostBufferToPath(const 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.
@@ -224,7 +254,17 @@ bool ServerConnectionManager::CheckTime(int32* out_time) {
   string post_body = "command=get_time";
 
   for (int i = 0 ; i < 3;  i++) {
-    scoped_ptr<Post> post(MakePost());
+    scoped_ptr<Connection> post;

[akalin, 2011/08/31 07:20:39]

similarly as above, you can just do:

scoped_ptr<Connection> post(MakeActiveConnection());
if (!post.get()) {
  break;
}

bool ok = post->Init(...);

+    scoped_ptr<AutoReset<Connection*> > active_connection_resetter;
+    {
+      base::AutoLock lock(terminate_connection_lock_);
+      if (terminated_)
+        break;
+      DCHECK(!active_connection_);
+      post.reset(MakeConnection());
+      active_connection_resetter.reset(
+          new AutoReset<Connection*>(&active_connection_, post.get()));
+    }
 
     // Note that the server's get_time path doesn't require authentication.
     string get_time_path =
@@ -257,18 +297,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;
@@ -278,7 +318,7 @@ bool ServerConnectionManager::CheckServerReachable() {
 }
 
 bool ServerConnectionManager::IncrementErrorCount() {
-  DCHECK(CalledOnValidThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   error_count_++;
 
   if (error_count_ > kMaxConnectionErrorsBeforeReset) {
@@ -299,7 +339,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;
@@ -334,20 +374,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) {