sync: Gracefully handle very early shutdown

sync/engine/net/server_connection_manager.h

Index: sync/engine/net/server_connection_manager.h
diff --git a/sync/engine/net/server_connection_manager.h b/sync/engine/net/server_connection_manager.h
index 3bd533e78fdd07d555e54d296e9b70465fd5bbf6..f20542bd5ee334c80bcd40953fb4aea877546235 100644
--- a/sync/engine/net/server_connection_manager.h
+++ b/sync/engine/net/server_connection_manager.h
@@ -8,14 +8,13 @@
 #include <iosfwd>
 #include <string>
 
-#include "base/atomicops.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
 #include "base/strings/string_util.h"
-#include "base/synchronization/lock.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/threading/thread_checker.h"
 #include "sync/base/sync_export.h"
+#include "sync/internal_api/public/base/cancellation_observer.h"
 #include "sync/syncable/syncable_id.h"
 
 namespace sync_pb {
@@ -24,6 +23,8 @@ class ClientToServerMessage;
 
 namespace syncer {
 
+class CancellationSignal;
+
 namespace syncable {
 class Directory;
 }
@@ -183,7 +184,8 @@ class SYNC_EXPORT_PRIVATE ServerConnectionManager {
   ServerConnectionManager(const std::string& server,
                           int port,
                           bool use_ssl,
-                          bool use_oauth2_token);
+                          bool use_oauth2_token,
+                          CancellationSignal* cancellation_signal);
 
   virtual ~ServerConnectionManager();
 
@@ -215,12 +217,6 @@ class SYNC_EXPORT_PRIVATE ServerConnectionManager {
   // communication with the server.
   virtual Connection* MakeConnection();
 
-  // Aborts any active HTTP POST request.
-  // We expect this to get called on a different thread than the valid
-  // ThreadChecker thread, as we want to kill any pending http traffic without
-  // having to wait for the request to complete.
-  void TerminateAllIO();
-
   void set_client_id(const std::string& client_id) {
     DCHECK(thread_checker_.CalledOnValidThread());
     DCHECK(client_id_.empty());
@@ -308,34 +304,31 @@ class SYNC_EXPORT_PRIVATE ServerConnectionManager {
 
   base::ThreadChecker thread_checker_;
 
-  // Protects all variables below to allow bailing out of active connections.
-  base::Lock terminate_connection_lock_;
-
-  // If true, we've been told to terminate IO and expect to be destroyed
-  // shortly.  No future network requests will be made.
-  bool terminated_;
-
-  // A non-owning pointer to any active http connection, so that we can abort
-  // it if necessary.
-  Connection* active_connection_;
-
+  CancellationSignal* cancellation_signal_;

[akalin, 2013/09/05 16:06:35]

const?

[rlarocque, 2013/09/05 22:37:06]

Done.

  private:
   friend class Connection;
   friend class ScopedServerStatusWatcher;
 
-  // A class to help deal with cleaning up active Connection objects when (for
-  // ex) multiple early-exits are present in some scope. ScopedConnectionHelper
-  // informs the ServerConnectionManager before the Connection object it takes
-  // ownership of is destroyed.
-  class ScopedConnectionHelper {
+  // A class to help manage the active connection.  It handles the registration
+  // and unregistration with the CancellationSignal.  It also takes ownership of
+  // the connection and will delete it if the abort signal was sent early or
+  // when this class goes out of scope.
+  class ScopedConnectionHelper : public CancellationObserver {
    public:
-    // |manager| must outlive this. Takes ownership of |connection|.
-    ScopedConnectionHelper(ServerConnectionManager* manager,
+    // Takes ownership of |connection|.

[akalin, 2013/09/05 16:06:35]

make this take scoped_ptr<Connection> instead of the comment?

[rlarocque, 2013/09/05 22:37:06]

Done.

+    ScopedConnectionHelper(CancellationSignal* cancellation_signal,
                            Connection* connection);
-    ~ScopedConnectionHelper();
+    virtual ~ScopedConnectionHelper();
     Connection* get();
+
+    // Called from a different thread when the CancellationObserver's
+    // RequestStop() is called and this class has been registered as a handler.
+    //
+    // Marked final because there's no way to safely override it.  See comment
+    // in this class' destructor.
+    virtual void OnStopRequested() OVERRIDE FINAL;

[akalin, 2013/09/05 16:06:35]

newline before private:

[rlarocque, 2013/09/05 22:37:06]

Done.

    private:
-    ServerConnectionManager* manager_;
+    CancellationSignal* cancellation_signal_;

[akalin, 2013/09/05 16:06:35]

const?

[rlarocque, 2013/09/05 22:37:06]

Done.

     scoped_ptr<Connection> connection_;
     DISALLOW_COPY_AND_ASSIGN(ScopedConnectionHelper);

[akalin, 2013/09/05 16:06:35]

i believe there should be a newline before DISALLOW

[rlarocque, 2013/09/05 22:37:06]

Done.

   };