Worker changes to prepare for lock-free shutdown.

sync/internal_api/public/engine/model_safe_worker.h

 // Copyright 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 SYNC_INTERNAL_API_PUBLIC_ENGINE_MODEL_SAFE_WORKER_H_
 #define SYNC_INTERNAL_API_PUBLIC_ENGINE_MODEL_SAFE_WORKER_H_
 
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
+#include "base/message_loop.h"
+#include "base/synchronization/lock.h"
+#include "base/synchronization/waitable_event.h"
 #include "sync/base/sync_export.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/base/model_type_invalidation_map.h"
 #include "sync/internal_api/public/util/syncer_error.h"
 
 namespace base {
 class DictionaryValue;
 }  // namespace
 
 namespace syncer {
@@ skipping 13 matching lines @@
   GROUP_HISTORY,       // Models that live on history thread and are being
                        // synced.
   GROUP_PASSWORD,      // Models that live on the password thread and are
                        // being synced.  On windows and linux, this runs on the
                        // DB thread.
   MODEL_SAFE_GROUP_COUNT,
 };
 
 SYNC_EXPORT std::string ModelSafeGroupToString(ModelSafeGroup group);
 
+// WorkerObserver is notified when worker can't do more work because the thread
+// it works on is going to be destroyed.
+class WorkerObserver {
+ public:
+  virtual void OnWorkerDisabled(ModelSafeGroup group) = 0;

[tim (not reviewing), 2013/05/14 04:14:29]

OnWorkerThreadDestroyed would be a better name.

[haitaol1, 2013/05/15 23:39:21]

Done.

+};
+
 // The Syncer uses a ModelSafeWorker for all tasks that could potentially
 // modify syncable entries (e.g under a WriteTransaction). The ModelSafeWorker
 // only knows how to do one thing, and that is take some work (in a fully
 // pre-bound callback) and have it performed (as in Run()) from a thread which
 // is guaranteed to be "model-safe", where "safe" refers to not allowing us to
 // cause an embedding application model to fall out of sync with the
-// syncable::Directory due to a race.
+// syncable::Directory due to a race. Each ModelSafeWorker is affiliated with
+// a browser thread and does actual work on that thread. On the destruction
+// of that thread, the affiliated worker is effectively disabled to do more
+// work and will notify its observer (SyncBackendRegistrar).

[tim (not reviewing), 2013/05/14 04:14:29]

Don't mention 'browser' or components from glue like SyncBackendRegistrar in the
sync/ module.  As part of a component, comments here should not assume specifics
about the embedder.

[haitaol1, 2013/05/15 23:39:21]

Done.

 class SYNC_EXPORT ModelSafeWorker
-    : public base::RefCountedThreadSafe<ModelSafeWorker> {
+    : public base::RefCountedThreadSafe<ModelSafeWorker>,
+      public MessageLoop::DestructionObserver {
  public:
+  // Subclass should implement to observer destruction of the loop where
+  // it actually does work.
+  virtual void RegisterForLoopDestruction() = 0;

[tim (not reviewing), 2013/05/14 04:14:29]

Who calls this?  and when?

[haitaol1, 2013/05/15 23:39:21]

SyncBackendHost will call this after creating workers.

+
+  // If not stopped, call DoWorkAndWaitUntilDone() to do work. Otherwise
+  // return CANNOT_DO_WORK.
+  SyncerError HandleWork(const WorkCallback& work);

[tim (not reviewing), 2013/05/14 04:14:29]

DoWorkAndWaitUntilDone is a better, more descriptive name.  The fact that it can
fail doesn't need we need a more abstract name... most functions have a failure
mode :)  I think your comment + return value suffices.

[haitaol1, 2013/05/15 23:39:21]

Done.

+
+  // Soft stop worker by setting stopped_ flag. Called when sync is disabled
+  // or browser is shutting down.
+  void RequestStop();
+
+  // Reset state when sync is enabled.

[tim (not reviewing), 2013/05/14 04:14:29]

When is this used?  Is this for when sync was disabled, but is now being
re-enabled, and the destruction observer hasn't fired? I'd rather be explicit
about what needs to happen than have a restart function here. e.g force the
caller to destroy (or put this MSW in a lame-duck mode and detach), and create a
new MSW.  

That moves the extra complexity for this case to the caller / client of
ModelSafeWorker, which seems right given that we want to remove that case
altogether from the higher layers. If we go with my suggestion we won't have to
make any changes to ModelSafeWorker when that switch happens, and we won't add
complexity here in the meantime.

[haitaol1, 2013/05/15 23:39:21]

Removed. Worker will be destroyed when sync is disabled.

On 2013/05/14 04:14:29, timsteele wrote:

+  void Restart();
+
+  virtual ModelSafeGroup GetModelSafeGroup() = 0;
+
+  // MessageLoop::DestructionObserver implementation.
+  virtual void WillDestroyCurrentMessageLoop() OVERRIDE;
+
+ protected:
+  friend class base::RefCountedThreadSafe<ModelSafeWorker>;
+
+  ModelSafeWorker(WorkerObserver* worker_host);

[tim (not reviewing), 2013/05/14 04:14:29]

Single argument constructors should be explicit.

[haitaol1, 2013/05/15 23:39:21]

Done.

+  virtual ~ModelSafeWorker();
+
   // Any time the Syncer performs model modifications (e.g employing a
   // WriteTransaction), it should be done by this method to ensure it is done
   // from a model-safe thread.
-  virtual SyncerError DoWorkAndWaitUntilDone(const WorkCallback& work) = 0;
+  virtual SyncerError DoWorkAndWaitUntilDone(

[tim (not reviewing), 2013/05/14 04:14:29]

You can call this DoWorkAndWaitIUntilDoneImpl.

[haitaol1, 2013/05/15 23:39:21]

Done.

+      const WorkCallback& work, base::WaitableEvent* done_or_stopped) = 0;
 
-  virtual ModelSafeGroup GetModelSafeGroup() = 0;
-
- protected:
-  virtual ~ModelSafeWorker();
+  // Return true if the worker was stopped. Thread safe.
+  bool Stopped();

[tim (not reviewing), 2013/05/14 04:14:29]

IsStopped()

[haitaol1, 2013/05/15 23:39:21]

Done.

 
  private:
-  friend class base::RefCountedThreadSafe<ModelSafeWorker>;
+  // Whether the worker should/can do more work. Set when sync is disabled or
+  // when the worker's working thread is to be destroyed.
+  base::Lock lock_;
+  bool stopped_;
+
+  // Signal set when work on native thread is finished or when native thread
+  // is to be destroyed so no more work can be done.
+  base::WaitableEvent work_done_or_stopped_;
+
+  // Notified when working thread of the worker is to be destroyed.
+  WorkerObserver* observer_;
 };
 
 // A map that details which ModelSafeGroup each ModelType
 // belongs to.  Routing info can change in response to the user enabling /
 // disabling sync for certain types, as well as model association completions.
 typedef std::map<ModelType, ModelSafeGroup> ModelSafeRoutingInfo;
 
 // Caller takes ownership of return value.
 SYNC_EXPORT_PRIVATE base::DictionaryValue* ModelSafeRoutingInfoToValue(
     const ModelSafeRoutingInfo& routing_info);
@@ skipping 11 matching lines @@
 SYNC_EXPORT ModelTypeSet GetRoutingInfoTypes(
     const ModelSafeRoutingInfo& routing_info);
 
 SYNC_EXPORT ModelSafeGroup GetGroupForModelType(
     const ModelType type,
     const ModelSafeRoutingInfo& routes);
 
 }  // namespace syncer
 
 #endif  // SYNC_INTERNAL_API_PUBLIC_ENGINE_MODEL_SAFE_WORKER_H_