Worker changes to prepare for lock-free shutdown.

sync/internal_api/public/engine/model_safe_worker.cc

Index: sync/internal_api/public/engine/model_safe_worker.cc
diff --git a/sync/internal_api/public/engine/model_safe_worker.cc b/sync/internal_api/public/engine/model_safe_worker.cc
index d7bf906ef9f007feec43925789b22a3367a3e15c..87c04e0f6ea77553fc997585133e203621d8b991 100644
--- a/sync/internal_api/public/engine/model_safe_worker.cc
+++ b/sync/internal_api/public/engine/model_safe_worker.cc
@@ -80,6 +80,57 @@ std::string ModelSafeGroupToString(ModelSafeGroup group) {
   }
 }
 
+ModelSafeWorker::ModelSafeWorker(WorkerLoopDestructionObserver* observer)
+    : stopped_(false),
+      work_done_or_stopped_(false, false),
+      observer_(observer) {}
+
 ModelSafeWorker::~ModelSafeWorker() {}
 
+void ModelSafeWorker::RequestStop() {
+  base::AutoLock al(stopped_lock_);
+
+  // Set stop flag but don't signal work_done_or_stopped_ to unblock sync loop
+  // because the worker may be working and depending on sync command object
+  // living on sync thread.
+  stopped_ = true;
+}
+
+SyncerError ModelSafeWorker::DoWorkAndWaitUntilDone(const WorkCallback& work) {
+  {
+    base::AutoLock al(stopped_lock_);
+    if (stopped_)
+      return CANNOT_DO_WORK;
+
+    // Clear signal before starting work.
+    work_done_or_stopped_.Reset();

[tim (not reviewing), 2013/05/17 20:51:33]

Why are we using manual reset for this?

[haitaol1, 2013/05/17 22:06:57]

It should be unnecessary if worker is not reused, so once it's stopped, it will
be destroyed later. Add a CHECK to make sure.

On 2013/05/17 20:51:33, timsteele wrote:

[tim (not reviewing), 2013/05/20 17:22:38]

My question was phrased a bit incorrectly; I was wondering why we were calling
Reset() despite having *not* passed |true| to manual_reset in the WaitableEvent
ctor.  I agree the Reset shouldn't be necessary, but I guess manual_reset=true
is still necessary since work_done will alternate.

[haitaol1, 2013/05/23 16:07:29]

With manual_reset=true, Reset() becomes necessary because the signal is not
cleared when waking up from done work. So next time, signal needs to be cleared
so that worker can block on task posted to browser thread.

On 2013/05/20 17:22:38, timsteele wrote:

[tim (not reviewing), 2013/05/23 18:10:37]

Right, but you never passed true for manual_reset in this patch, hence my
confusion.  Or I missed it somewhere...

+  }
+
+  return DoWorkAndWaitUntilDoneImpl(work, &work_done_or_stopped_);
+}
+
+bool ModelSafeWorker::IsStopped() {
+  base::AutoLock al(stopped_lock_);
+  return stopped_;
+}
+
+void ModelSafeWorker::WillDestroyCurrentMessageLoop() {
+  {
+    base::AutoLock al(stopped_lock_);
+    stopped_ = true;
+
+    // Safe to signal because pending work (if any) will not be executed. So
+    // there should be no harm even the work holds a pointer to the
+    // ModelChangingSyncerCommand that will be destroyed on sync loop after
+    // the signal is set.
+    work_done_or_stopped_.Signal();
+
+    DLOG(INFO) << ModelSafeGroupToString(GetModelSafeGroup())
+        << " worker stops on destruction of its working thread.";
+  }
+
+  if (observer_)
+    observer_->OnWorkerLoopDestroyed(GetModelSafeGroup());
+}
+
 }  // namespace syncer