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..4b69425d35610296ae6582fee1dc5b6763516066 100644
--- a/sync/internal_api/public/engine/model_safe_worker.cc
+++ b/sync/internal_api/public/engine/model_safe_worker.cc
@@ -80,6 +80,56 @@ 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.

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

Add to comment:  "This prevents any *further* tasks from being posted to worker
threads (see DoWorkAndWaitUntilDone below), but note that one may already be
posted."

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

Done.

+  stopped_ = true;
+}
+
+SyncerError ModelSafeWorker::DoWorkAndWaitUntilDone(const WorkCallback& work) {
+  {
+    base::AutoLock al(stopped_lock_);
+    if (stopped_)
+      return CANNOT_DO_WORK;
+
+    CHECK(!work_done_or_stopped_.IsSignaled());
+  }
+
+  return DoWorkAndWaitUntilDoneImpl(work);
+}
+
+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.

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

This is a subtle expectation.  For example, it's not true in the case of
UIModelWorker because it manually pumps the final task, meaning if we Signal()
it we won't run the code that does OnTaskCompleted(), for example.  However,
this winds up being OK because by the time WillDestroyCurrentMessageLoop is
called for the UI thread, we should have already called UIModelWorker::Stop().

Rather than just leaving a live task out there with now garbage pointers and
hoping the interactions remain "correct", the better thing to do would be to
make the tasks the workers post to their backend threads (the
"CallDoWorkAndSignal" variants) member functions of the ModelSafeWorker like you
did for UIModelWorker in the previous patchset, but then additionally base::Bind
should be passed a WeakPtr to the worker and then right here in
MSW::WillDestroyCurrentMessageLoop we should call 

weak_ptr_factory_.InvalidateWeakPtrs();

This would be a more graceful way of making the fact that those tasks will not
be executed 100% explicit in the code for all workers, current and future.

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

Resolved per offline discussion. Updated comments.

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

+    work_done_or_stopped_.Signal();
+
+    DVLOG(1) << ModelSafeGroupToString(GetModelSafeGroup())
+        << " worker stops on destruction of its working thread.";
+  }
+
+  if (observer_)
+    observer_->OnWorkerLoopDestroyed(GetModelSafeGroup());
+}
+
 }  // namespace syncer