Worker changes to prepare for lock-free shutdown.

chrome/browser/sync/glue/ui_model_worker.cc

 // 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.
 
 #include "chrome/browser/sync/glue/ui_model_worker.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/message_loop.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "base/threading/thread_restrictions.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
 
 namespace browser_sync {
 
-namespace {
-
-// A simple callback to signal a waitable event after running a closure.
-void CallDoWorkAndSignalCallback(const syncer::WorkCallback& work,
-                                 base::WaitableEvent* work_done,
-                                 UIModelWorker* const scheduler,
-                                 syncer::SyncerError* error_info) {
-  if (work.is_null()) {
-    // This can happen during tests or cases where there are more than just the
-    // default UIModelWorker in existence and it gets destroyed before
-    // the main UI loop has terminated.  There is no easy way to assert the
-    // loop is running / not running at the moment, so we just provide cancel
-    // semantics here and short-circuit.
-    // TODO(timsteele): Maybe we should have the message loop destruction
-    // observer fire when the loop has ended, just a bit before it
-    // actually gets destroyed.
-    return;
-  }
-
-  *error_info = work.Run();
-
-  // Notify the UIModelWorker that scheduled us that we have run
-  // successfully.
-  scheduler->OnTaskCompleted();
-  work_done->Signal();  // Unblock the syncer thread that scheduled us.
+UIModelWorker::UIModelWorker(syncer::WorkerLoopDestructionObserver* observer)
+    : syncer::ModelSafeWorker(observer),
+      state_(WORKING),
+      syncapi_has_shutdown_(false),
+      syncapi_event_(&lock_) {
 }
 
-}  // namespace
-
-UIModelWorker::UIModelWorker()
-    : state_(WORKING),
-      syncapi_has_shutdown_(false),
-      syncapi_event_(&lock_) {
+void UIModelWorker::RegisterForLoopDestruction() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  MessageLoop::current()->AddDestructionObserver(this);
 }
 
 void UIModelWorker::Stop() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   base::AutoLock lock(lock_);
   DCHECK_EQ(state_, WORKING);
 
   // We're on our own now, the beloved UI MessageLoop is no longer running.
   // Any tasks scheduled or to be scheduled on the UI MessageLoop will not run.
   state_ = RUNNING_MANUAL_SHUTDOWN_PUMP;
 
   // Drain any final tasks manually until the SyncerThread tells us it has
   // totally finished. There should only ever be 0 or 1 tasks Run() here.
   while (!syncapi_has_shutdown_) {
     if (!pending_work_.is_null())
       pending_work_.Run();  // OnTaskCompleted will set reset |pending_work_|.
 
     // http://crbug.com/19757
     base::ThreadRestrictions::ScopedAllowWait allow_wait;
     // Wait for either a new task or SyncerThread termination.
     syncapi_event_.Wait();
   }
 
   state_ = STOPPED;
 }
 
-syncer::SyncerError UIModelWorker::DoWorkAndWaitUntilDone(
-    const syncer::WorkCallback& work) {
+syncer::SyncerError UIModelWorker::DoWorkAndWaitUntilDoneImpl(
+    const syncer::WorkCallback& work, base::WaitableEvent* done) {
   // In most cases, this method is called in WORKING state. It is possible this
   // gets called when we are in the RUNNING_MANUAL_SHUTDOWN_PUMP state, because
   // the UI loop has initiated shutdown but the syncer hasn't got the memo yet.
   // This is fine, the work will get scheduled and run normally or run by our
   // code handling this case in Stop(). Note there _no_ way we can be in here
   // with state_ = STOPPED, so it is safe to read / compare in this case.
   CHECK_NE(ANNOTATE_UNPROTECTED_READ(state_), STOPPED);
   syncer::SyncerError error_info;
   if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     DLOG(WARNING) << "DoWorkAndWaitUntilDone called from "
       << "ui_loop_. Probably a nested invocation?";
     return work.Run();
   }
 
-  // Create an unsignaled event to wait on.
-  base::WaitableEvent work_done(false, false);
   {
     // We lock only to avoid PostTask'ing a NULL pending_work_ (because it
     // could get Run() in Stop() and call OnTaskCompleted before we post).
     // The task is owned by the message loop as per usual.
     base::AutoLock lock(lock_);
     DCHECK(pending_work_.is_null());
-    pending_work_ = base::Bind(&CallDoWorkAndSignalCallback, work, &work_done,
-                               base::Unretained(this), &error_info);
+    pending_work_ = base::Bind(&UIModelWorker::CallDoWorkAndSignalCallback,
+                               this, work, done, &error_info);
     if (!BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, pending_work_)) {
       DLOG(WARNING) << "Could not post work to UI loop.";
       error_info = syncer::CANNOT_DO_WORK;
       pending_work_.Reset();
       syncapi_event_.Signal();
       return error_info;
     }
   }
   syncapi_event_.Signal();  // Notify that the syncapi produced work for us.
-  work_done.Wait();
+  done->Wait();

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

Do we need to pass the event around?  It's a member of this class, and having
multiple event*'s floating around really makes things complex to follow.  I'd
prefer a protected function, either returning a pointer or for waiting, e.g

work_done_or_stopped()->Wait()
WaitForWorkDoneOrStopped();

Although given the multiple possible signals and multiple layers of locks we're
using to protect waitable events (which is always something to question) I'm
wondering if it wouldn't be clearer using ConditionVariable,

while (work_is_pending_ && !stopped()) 
  not_processing_work_.Wait()

but given that I don't think |this|->lock_ will be necessary in a subsequent CL
(assuming we can remove pending_work_ as I discussed in a comment @line 116),
maybe it isn't worth rocking the boat too much yet.

It's just that various levels of locks make it extremely difficult to review
from a perspective of ensuring no re-entrancy or re-acquisition issues (e.g
someone Signal()ing holding a lock that tries to get re-acquired). 

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

Added protected accessor.

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

   return error_info;
 }
 
+void UIModelWorker::CallDoWorkAndSignalCallback(
+    const syncer::WorkCallback& work,
+    base::WaitableEvent* work_done,
+    syncer::SyncerError* error_info) {
+  if (work.is_null()) {
+    // This can happen during tests or cases where there are more than just the
+    // default UIModelWorker in existence and it gets destroyed before

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

You're not passing UIModelWorker* to a static function anymore -- this is a
member function, so you should not end up here if UIModelWorker was destroyed.  

I also think we're about to run into another downside with ref-counted objects. 
Making this a member function of a ref-counted class means your base::Bind
invocation will implicitly keep the UIModelWorker alive while the posted task
lives.  This is logically wrong, but it will work technically due to
refcounting.  Instead of that, if you want to make this a member function, then
the UIModelWorker should have a weak_ptr_factory_ and vend a WeakPtr to Bind
(See SyncSchedulerImpl for a nearby example.)

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

The UI worker (or any worker) will not be destroyed until it returns from the
work posted to it. So explicit reference is held by registrar. The ref-counting
of worker already exists, which seems unnecessary.


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

+    // the main UI loop has terminated.  There is no easy way to assert the
+    // loop is running / not running at the moment, so we just provide cancel
+    // semantics here and short-circuit.
+    // TODO(timsteele): Maybe we should have the message loop destruction
+    // observer fire when the loop has ended, just a bit before it
+    // actually gets destroyed.
+    return;
+  }
+
+  if (!IsStopped())

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

This is hard to follow because RequestStop sets stopped_ but doesn't transition
the state to STOPPED, which is what the UIModelWorker currently uses to
determine whether it is WORKING, STOPPED, or RUNNING_MANUAL_SHUTDOWN_PUMP. Also,
It's not clear from the naming when Stop() will be called relative to
RequestStop() (which actually has nothing to do with Stop(), despite the name),
or WillDestroyMessageLoop.  

Perhaps this clears up in subsequent patches, but I don't know if I can approve
this with two notions of Stop going on in the same class.  I'd like to try and
clear things up, or at the very least ensure one of the Stop-paths (methods and
state) are clearly annotated (so maybe Stop() -> StopNowDeprecated(), and a
comment when we assign pending_work_ that this is only temporarily needed while
we remove the MANUAL_SHUTDOWN_PUMP state, for starters).

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

Reverted.

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

+    *error_info = work.Run();
+  else
+    *error_info = syncer::CANNOT_DO_WORK;
+
+  // Notify the UIModelWorker that scheduled us that we have run
+  // successfully.
+  OnTaskCompleted();

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

OnTaskCompleted is now notifying |this| instead of "the UIModelWorker that
scheduled us") and should really be inlined, all it does is reset pending_work_.

e.g Are you planning to remove pending_work_ altogether in a follow up CL,
because we don't need to manually flush it? What should happen if UIModelWorker
is destroyed with a dangling CallDoWork? WeakPtrs as proposed above would
gracefully "cancel" the task, because the bound object will no longer exist when
the callback code tries to invoke us.  

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

Yes
That shouldn't happen because syncer is blocked on worker and only unblocked
when work finish or worker loop is destroyed. Worker can only destroyed after
syncer stops.

WeakPtrs as proposed above would

+  work_done->Signal();  // Unblock the syncer thread that scheduled us.
+}
+
 syncer::ModelSafeGroup UIModelWorker::GetModelSafeGroup() {
   return syncer::GROUP_UI;
 }
 
 void UIModelWorker::OnSyncerShutdownComplete() {
   base::AutoLock lock(lock_);
   // The SyncerThread has terminated and we are no longer needed by syncapi.
   // The UI loop initiated shutdown and is (or will be) waiting in Stop().
   // We could either be WORKING or RUNNING_MANUAL_SHUTDOWN_PUMP, depending
   // on where we timeslice the UI thread in Stop; but we can't be STOPPED,
   // because that would imply OnSyncerShutdownComplete already signaled.
   DCHECK_NE(state_, STOPPED);
 
   syncapi_has_shutdown_ = true;
   syncapi_event_.Signal();
 }
 
 UIModelWorker::~UIModelWorker() {
   DCHECK_EQ(state_, STOPPED);
 }
 
 }  // namespace browser_sync