Retry: sync: Gracefully handle early shutdown

chrome/browser/sync/glue/sync_backend_host.h

 // 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.
 
 #ifndef CHROME_BROWSER_SYNC_GLUE_SYNC_BACKEND_HOST_H_
 #define CHROME_BROWSER_SYNC_GLUE_SYNC_BACKEND_HOST_H_
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/thread.h"
 #include "chrome/browser/invalidation/invalidation_service.h"
 #include "chrome/browser/sync/glue/backend_data_type_configurer.h"
 #include "chrome/browser/sync/glue/extensions_activity_monitor.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "google_apis/gaia/google_service_auth_error.h"
+#include "sync/internal_api/public/base/cancelation_signal.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/configure_reason.h"
 #include "sync/internal_api/public/engine/model_safe_worker.h"
 #include "sync/internal_api/public/sessions/sync_session_snapshot.h"
 #include "sync/internal_api/public/sync_encryption_handler.h"
 #include "sync/internal_api/public/sync_manager.h"
 #include "sync/internal_api/public/util/report_unrecoverable_error_function.h"
 #include "sync/internal_api/public/util/unrecoverable_error_handler.h"
 #include "sync/internal_api/public/util/weak_handle.h"
 #include "sync/notifier/invalidation_handler.h"
@@ skipping 178 matching lines @@
 
   // Called on |frontend_loop_| to use the provided passphrase to asynchronously
   // attempt decryption. Returns false immediately if the passphrase could not
   // be used to decrypt a locally cached copy of encrypted keys; returns true
   // otherwise. If new encrypted keys arrive during the asynchronous call,
   // OnPassphraseRequired may be triggered at a later time. It is an error to
   // call this when there are no pending keys.
   bool SetDecryptionPassphrase(const std::string& passphrase)
       WARN_UNUSED_RESULT;
 
-  // Called on |frontend_loop_| to kick off shutdown procedure. After this, no
-  // further sync activity will occur with the sync server and no further
-  // change applications will occur from changes already downloaded.
-  // Furthermore, no notifications will be sent to any invalidation handler.
+  // Called on |frontend_loop_| to kick off shutdown procedure.  Attempts to cut
+  // short any long-lived or blocking sync thread tasks so that the shutdown on
+  // sync thread task that we're about to post won't have to wait very long.
   virtual void StopSyncingForShutdown();
 
   // Called on |frontend_loop_| to kick off shutdown.
   // See the implementation and Core::DoShutdown for details.
   // Must be called *after* StopSyncingForShutdown. Caller should claim sync
   // thread using STOP_AND_CLAIM_THREAD or DISABLE_AND_CLAIM_THREAD if sync
   // backend might be recreated later because otherwise:
   // * sync loop may be stopped on main loop and cause it to be blocked.
   // * new/old backend may interfere with each other if new backend is created
   //   before old one finishes cleanup.
@@ skipping 71 matching lines @@
 
   base::MessageLoop* GetSyncLoopForTesting();
 
  protected:
   // The types and functions below are protected so that test
   // subclasses can use them.
   //
   // TODO(akalin): Figure out a better way for tests to hook into
   // SyncBackendHost.
 
-  typedef base::Callback<scoped_ptr<syncer::HttpPostProviderFactory>(void)>
-      MakeHttpBridgeFactoryFn;
-
   // Utility struct for holding initialization options.
   struct DoInitializeOptions {
     DoInitializeOptions(
         base::MessageLoop* sync_loop,
         SyncBackendRegistrar* registrar,
         const syncer::ModelSafeRoutingInfo& routing_info,
         const std::vector<syncer::ModelSafeWorker*>& workers,
         const scoped_refptr<syncer::ExtensionsActivity>& extensions_activity,
         const syncer::WeakHandle<syncer::JsEventHandler>& event_handler,
         const GURL& service_url,
-        MakeHttpBridgeFactoryFn make_http_bridge_factory_fn,
+        scoped_ptr<syncer::HttpPostProviderFactory> http_bridge_factory,
         const syncer::SyncCredentials& credentials,
         const std::string& invalidator_client_id,
         scoped_ptr<syncer::SyncManagerFactory> sync_manager_factory,
         bool delete_sync_data_folder,
         const std::string& restored_key_for_bootstrapping,
         const std::string& restored_keystore_key_for_bootstrapping,
         scoped_ptr<syncer::InternalComponentsFactory>
             internal_components_factory,
         scoped_ptr<syncer::UnrecoverableErrorHandler>
             unrecoverable_error_handler,
         syncer::ReportUnrecoverableErrorFunction
             report_unrecoverable_error_function,
-        bool use_oauth2_token);
+        bool use_oauth2_token,
+        syncer::CancelationSignal* scm_cancelation_signal);
     ~DoInitializeOptions();
 
     base::MessageLoop* sync_loop;
     SyncBackendRegistrar* registrar;
     syncer::ModelSafeRoutingInfo routing_info;
     std::vector<syncer::ModelSafeWorker*> workers;
     scoped_refptr<syncer::ExtensionsActivity> extensions_activity;
     syncer::WeakHandle<syncer::JsEventHandler> event_handler;
     GURL service_url;
     // Overridden by tests.
-    MakeHttpBridgeFactoryFn make_http_bridge_factory_fn;
+    scoped_ptr<syncer::HttpPostProviderFactory> http_bridge_factory;
     syncer::SyncCredentials credentials;
     const std::string invalidator_client_id;
     scoped_ptr<syncer::SyncManagerFactory> sync_manager_factory;
     std::string lsid;
     bool delete_sync_data_folder;
     std::string restored_key_for_bootstrapping;
     std::string restored_keystore_key_for_bootstrapping;
     scoped_ptr<syncer::InternalComponentsFactory> internal_components_factory;
     scoped_ptr<syncer::UnrecoverableErrorHandler> unrecoverable_error_handler;
     syncer::ReportUnrecoverableErrorFunction
         report_unrecoverable_error_function;
     bool use_oauth2_token;
+    syncer::CancelationSignal* const scm_cancelation_signal;
   };
 
   // Allows tests to perform alternate core initialization work.
   virtual void InitCore(scoped_ptr<DoInitializeOptions> options);
 
   // Request the syncer to reconfigure with the specfied params.
   // Virtual for testing.
   virtual void RequestConfigureSyncer(
       syncer::ConfigureReason reason,
       syncer::ModelTypeSet to_download,
@@ skipping 140 matching lines @@
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) OVERRIDE;
 
   // InvalidationHandler implementation.
   virtual void OnInvalidatorStateChange(
       syncer::InvalidatorState state) OVERRIDE;
   virtual void OnIncomingInvalidation(
       const syncer::ObjectIdInvalidationMap& invalidation_map) OVERRIDE;
 
-  // Handles stopping the core's SyncManager, accounting for whether
-  // initialization is done yet.
-  void StopSyncManagerForShutdown();
-
   base::WeakPtrFactory<SyncBackendHost> weak_ptr_factory_;
 
   content::NotificationRegistrar notification_registrar_;
 
   // A reference to the MessageLoop used to construct |this|, so we know how
   // to safely talk back to the SyncFrontend.
   base::MessageLoop* const frontend_loop_;
 
   Profile* const profile_;
 
@@ skipping 42 matching lines @@
   // Temporary holder of sync manager's initialization results. Set by
   // HandleSyncManagerInitializationOnFrontendLoop, and consumed when we pass
   // it via OnBackendInitialized in the final state of
   // HandleInitializationCompletedOnFrontendLoop.
   syncer::WeakHandle<syncer::JsBackend> js_backend_;
   syncer::WeakHandle<syncer::DataTypeDebugInfoListener> debug_info_listener_;
 
   invalidation::InvalidationService* invalidator_;
   bool invalidation_handler_registered_;
 
+  // These signals are (mostly) owned by the UI thread.  They allow us to send
+  // requests to shut down the HttpBridgeFactory and ServerConnectionManager
+  // without having to wait for those classes to finish initializing first.
+  //
+  // See comments in StopSyncingForShutdown() and Shutdown() for more details.
+  scoped_ptr<syncer::CancelationSignal> factory_cancelation_signal_;

[haitaol1, 2013/09/13 18:43:21]

Maybe move these into SyncBackendRegistrar, which is the last to be destroyed.

[rlarocque, 2013/09/13 20:27:17]

That should work.  However, there is a comment that describes
SyncBackendRegistrar as follows:

// A class that keep track of the workers, change processors, and
// routing info for the enabled sync types, and also routes change
// events to the right processors.

Based on that description, it's hard to justify why such a class should own
these cancelation signals.

Maybe there should be a separate object that owns these signals.  But I don't
think that class should be the one called "SyncBackendRegistrar".

+  scoped_ptr<syncer::CancelationSignal> scm_cancelation_signal_;
+
   DISALLOW_COPY_AND_ASSIGN(SyncBackendHost);
 };
 
 }  // namespace browser_sync
 
 #endif  // CHROME_BROWSER_SYNC_GLUE_SYNC_BACKEND_HOST_H_