- Add Thread::StopSoon() and remove Thread::NonBlockingStop(). StopSoon() can't be implemented externally of the Thread class where NonBlockingStop() was really just

an helper function solely used in printing.
- Move two member functions access from public to protected.
- Add documentation about which thread modifies which member variable.
- Simplify ThreadStartInfo. This removes one heap allocation.
- Improve unit test coverage.

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@728 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 113 insertions, 138 deletions

diff --git a/base/thread.cc b/base/thread.cc
index ab79a9a..4e626e3 100644
--- a/base/thread.cc
+++ b/base/thread.cc
@@ -27,11 +27,11 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+#include "base/thread.h"
+
 #include <process.h>
 #include <windows.h>
 
-#include "base/thread.h"
-
 #include "base/message_loop.h"
 #include "base/object_watcher.h"
 #include "base/ref_counted.h"
@@ -44,7 +44,7 @@ namespace {
 // This class is used when starting a thread.  It passes information to the
 // thread function.  It is referenced counted so we can cleanup the event
 // object used to synchronize thread startup properly.
-class ThreadStartInfo : public base::RefCountedThreadSafe<ThreadStartInfo> {
+class ThreadStartInfo {
  public:
   Thread* self;
   base::WaitableEvent start_event;
@@ -53,6 +53,8 @@ class ThreadStartInfo : public base::RefCountedThreadSafe<ThreadStartInfo> {
   }
 };
 
+}  // namespace
+
 // This task is used to trigger the message loop to exit.
 class ThreadQuitTask : public Task {
  public:
@@ -62,16 +64,6 @@ class ThreadQuitTask : public Task {
   }
 };
 
-// Once an object is signaled, quits the current inner message loop.
-class QuitOnSignal : public base::ObjectWatcher::Delegate {
- public:
-  virtual void OnObjectSignaled(HANDLE object) {
-    MessageLoop::current()->Quit();
-  }
-};
-
-}  // namespace
-
 Thread::Thread(const char *name)
     : thread_(NULL),
       thread_id_(0),
@@ -144,7 +136,7 @@ bool Thread::Start() {
 bool Thread::StartWithStackSize(size_t stack_size) {
   DCHECK(!thread_id_ && !thread_);
   SetThreadWasQuitProperly(false);
-  scoped_refptr<ThreadStartInfo> info = new ThreadStartInfo(this);
+  ThreadStartInfo info(this);
 
   unsigned int flags = 0;
   if (win_util::GetWinVersion() >= win_util::WINVERSION_XP && stack_size) {
@@ -156,7 +148,7 @@ bool Thread::StartWithStackSize(size_t stack_size) {
       _beginthreadex(NULL,
                      static_cast<unsigned int>(stack_size),
                      ThreadFunc,
-                     info,
+                     &info,
                      flags,
                      &thread_id_));
   if (!thread_) {
@@ -165,23 +157,36 @@ bool Thread::StartWithStackSize(size_t stack_size) {
   }
 
   // Wait for the thread to start and initialize message_loop_
-  info->start_event.Wait();
+  info.start_event.Wait();
   return true;
 }
 
 void Thread::Stop() {
-  InternalStop(false);
-}
+  if (!thread_)
+    return;
+
+  DCHECK_NE(thread_id_, GetCurrentThreadId()) << "Can't call Stop() on itself";
+
+  if (message_loop())
+    message_loop_->PostTask(FROM_HERE, new ThreadQuitTask());
+
+  // Wait for the thread to exit. It should already have terminated but make
+  // sure this assumption is valid.
+  DWORD result = WaitForSingleObject(thread_, INFINITE);
+  DCHECK_EQ(result, WAIT_OBJECT_0);
 
-void Thread::NonBlockingStop() {
-  InternalStop(true);
+  // Reset state.
+  CloseHandle(thread_);
+  thread_ = NULL;
+  thread_id_ = 0;
 }
 
-void Thread::InternalStop(bool run_message_loop) {
-  if (!thread_)
+void Thread::StopSoon() {
+  if (!thread_id_)
     return;
 
-  DCHECK_NE(thread_id_, GetCurrentThreadId()) << "Can't call Stop() on itself";
+  DCHECK_NE(thread_id_, GetCurrentThreadId()) <<
+      "Can't call StopSoon() on itself";
 
   // We had better have a message loop at this point!  If we do not, then it
   // most likely means that the thread terminated unexpectedly, probably due
@@ -190,25 +195,8 @@ void Thread::InternalStop(bool run_message_loop) {
 
   message_loop_->PostTask(FROM_HERE, new ThreadQuitTask());
 
-  if (run_message_loop) {
-    QuitOnSignal quit_on_signal;
-    base::ObjectWatcher signal_watcher;
-    CHECK(signal_watcher.StartWatching(thread_, &quit_on_signal));
-
-    bool old_state = MessageLoop::current()->NestableTasksAllowed();
-    MessageLoop::current()->SetNestableTasksAllowed(true);
-    MessageLoop::current()->Run();
-    // Restore task state.
-    MessageLoop::current()->SetNestableTasksAllowed(old_state);
-  } else {
-    // Wait for the thread to exit.
-    WaitForSingleObject(thread_, INFINITE);
-  }
-  CloseHandle(thread_);
-
-  // Reset state.
-  thread_ = NULL;
-  message_loop_ = NULL;
+  // The thread can't receive messages anymore.
+  thread_id_ = 0;
 }
 
 /*static*/
@@ -218,16 +206,15 @@ unsigned __stdcall Thread::ThreadFunc(void* param) {
 
   Thread* self;
 
-  // Complete the initialization of our Thread object.  We grab an owning
-  // reference to the ThreadStartInfo object to ensure that the start_event
-  // object is not prematurely closed.
+  // Complete the initialization of our Thread object.
   {
-    scoped_refptr<ThreadStartInfo> info = static_cast<ThreadStartInfo*>(param);
+    ThreadStartInfo* info = static_cast<ThreadStartInfo*>(param);
     self = info->self;
     self->message_loop_ = &message_loop;
     SetThreadName(self->thread_name().c_str(), GetCurrentThreadId());
     message_loop.SetThreadName(self->thread_name());
     info->start_event.Signal();
+    // info can't be touched anymore since the starting thread is now unlocked.
   }
 
   // Let the thread do extra initialization.
@@ -241,13 +228,8 @@ unsigned __stdcall Thread::ThreadFunc(void* param) {
   // Assert that MessageLoop::Quit was called by ThreadQuitTask.
   DCHECK(Thread::GetThreadWasQuitProperly());
 
-  // Clearing this here should be unnecessary since we should only be here if
-  // Thread::Stop was called (the above DCHECK asserts that).  However, to help
-  // make it easier to track down problems in release builds, we null out the
-  // message_loop_ pointer.
+  // We can't receive messages anymore.
   self->message_loop_ = NULL;
 
-  // We can't receive messages anymore.
-  self->thread_id_ = 0;
   return 0;
 }
diff --git a/base/thread.h b/base/thread.h
index 1780d85..bb8502e 100644
--- a/base/thread.h
+++ b/base/thread.h
@@ -27,12 +27,11 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-#ifndef BASE_THREAD_H__
-#define BASE_THREAD_H__
+#ifndef BASE_THREAD_H_
+#define BASE_THREAD_H_
 
 #include <string>
 
-#include "base/basictypes.h"
 #include "base/thread_local_storage.h"
 
 class MessageLoop;
@@ -65,11 +64,18 @@ class Thread {
   // otherwise, returns false.  Upon successful return, the message_loop()
   // getter will return non-null.
   //
+  // Note: This function can't be called on Windows with the loader lock held;
+  // i.e. during a DllMain, global object construction or destruction, atexit()
+  // callback.
   bool Start();
 
   // Starts the thread. Behaves exactly like Start in addition to allow to
   // override the default process stack size. This is not the initial stack size
   // but the maximum stack size that thread is allowed to use.
+  //
+  // Note: This function can't be called on Windows with the loader lock held;
+  // i.e. during a DllMain, global object construction or destruction, atexit()
+  // callback.
   bool StartWithStackSize(size_t stack_size);
 
   // Signals the thread to exit and returns once the thread has exited.  After
@@ -85,16 +91,18 @@ class Thread {
   //
   void Stop();
 
-  // Signals the thread to exit and returns once the thread has exited.
-  // Meanwhile, a message loop is run. After this method returns, the Thread
-  // object is completely reset and may be used as if it were newly constructed
-  // (i.e., Start may be called again).
+  // Signals the thread to exit in the near future.
   //
-  // NonBlockingStop may be called multiple times and is simply ignored if the
-  // thread is already stopped.
+  // WARNING: This function is not meant to be commonly used. Use at your own
+  // risk. Calling this function will cause message_loop() being invalidated in
+  // the near future. This function was created to workaround a specific
+  // deadlock on Windows with printer worker thread. In any other case, Stop()
+  // should be used.
   //
-  // NOTE: The behavior is the same as Stop() except that pending tasks are run.
-  void NonBlockingStop();
+  // StopSoon should not called multiple times and is risky to do so. It could
+  // cause a timing issue in message_loop() access. Call Stop() to reset the
+  // thread object once it is known that the thread has quit.
+  void StopSoon();
 
   // Returns the message loop for this thread.  Use the MessageLoop's
   // PostTask methods to execute code on the thread.  This only returns
@@ -104,13 +112,18 @@ class Thread {
   // NOTE: You must not call this MessageLoop's Quit method directly.  Use
   // the Thread's Stop method instead.
   //
-  MessageLoop* message_loop() const { return message_loop_; }
+  MessageLoop* message_loop() const {
+    if (thread_id_)
+      return message_loop_;
+    return NULL;
+  }
 
   // Set the name of this thread (for display in debugger too).
   const std::string &thread_name() { return name_; }
 
-  static void SetThreadWasQuitProperly(bool flag);
-  static bool GetThreadWasQuitProperly();
+#if defined(OS_WIN)
+  HANDLE thread_handle() { return thread_; }
+#endif
 
   // Sets the thread name if a debugger is currently attached. Has no effect
   // otherwise. To set the name of the current thread, pass GetCurrentThreadId()
@@ -124,21 +137,31 @@ class Thread {
   // Called just after the message loop ends
   virtual void CleanUp() { }
 
-  // Stops the thread. Called by either Stop() or NonBlockingStop().
-  void InternalStop(bool run_message_loop);
+  static void SetThreadWasQuitProperly(bool flag);
+  static bool GetThreadWasQuitProperly();
 
  private:
-#ifdef OS_WIN
+#if defined(OS_WIN)
   static unsigned __stdcall ThreadFunc(void* param);
+  // The thread's handle. Modified by the root thread.  
   HANDLE thread_;
 #endif
-  
+
+  // The thread's id. Modified by the root thread. Set to 0 when the thread is
+  // not ready to receive messages.
   ThreadId thread_id_;
+
+  // The thread's message loop. Valid only while the thread is alive. Modified
+  // by the created thread.
   MessageLoop* message_loop_;
-  std::string name_;
+
+  const std::string name_;
+
   static TLSSlot tls_index_;
 
-  DISALLOW_EVIL_CONSTRUCTORS(Thread);
+  friend class ThreadQuitTask;
+
+  DISALLOW_COPY_AND_ASSIGN(Thread);
 };
 
-#endif  // BASE_THREAD_H__
+#endif  // BASE_THREAD_H_
diff --git a/base/thread_unittest.cc b/base/thread_unittest.cc
index ba91005..c4e8be7 100644
--- a/base/thread_unittest.cc
+++ b/base/thread_unittest.cc
@@ -34,9 +34,6 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
-  class ThreadTest : public testing::Test {
-  };
-}
 
 class ToggleValue : public Task {
  public:
@@ -51,30 +48,38 @@ class ToggleValue : public Task {
 
 class SleepSome : public Task {
  public:
-  explicit SleepSome(DWORD msec) : msec_(msec) {
+  explicit SleepSome(int msec) : msec_(msec) {
   }
   virtual void Run() {
     Sleep(msec_);
   }
  private:
-  DWORD msec_;
+  int msec_;
 };
 
-TEST(ThreadTest, BasicTest1) {
-  Thread a("BasicTest1");
-  a.Start();
+}  // namespace
+
+TEST(ThreadTest, Restart) {
+  Thread a("Restart");
+  a.Stop();
+  EXPECT_FALSE(a.message_loop());
+  EXPECT_TRUE(a.Start());
   EXPECT_TRUE(a.message_loop());
   a.Stop();
   EXPECT_FALSE(a.message_loop());
-  a.Start();
+  EXPECT_TRUE(a.Start());
   EXPECT_TRUE(a.message_loop());
   a.Stop();
   EXPECT_FALSE(a.message_loop());
+  a.Stop();
+  EXPECT_FALSE(a.message_loop());
 }
 
-TEST(ThreadTest, BasicTest2) {
-  Thread a("BasicTest2");
-  a.Start();
+TEST(ThreadTest, StartWithStackSize) {
+  Thread a("StartWithStackSize");
+  // Ensure that the thread can work with only 12 kb and still process a
+  // message.
+  EXPECT_TRUE(a.StartWithStackSize(12*1024));
   EXPECT_TRUE(a.message_loop());
 
   bool was_invoked = false;
@@ -83,75 +88,40 @@ TEST(ThreadTest, BasicTest2) {
   // wait for the task to run (we could use a kernel event here
   // instead to avoid busy waiting, but this is sufficient for
   // testing purposes).
-  for (int i = 50; i >= 0 && !was_invoked; --i) {
-    Sleep(20);
+  for (int i = 100; i >= 0 && !was_invoked; --i) {
+    Sleep(10);
   }
   EXPECT_TRUE(was_invoked);
 }
 
-TEST(ThreadTest, BasicTest3) {
+TEST(ThreadTest, TwoTasks) {
   bool was_invoked = false;
   {
-    Thread a("BasicTest3");
-    a.Start();
+    Thread a("TwoTasks");
+    EXPECT_TRUE(a.Start());
     EXPECT_TRUE(a.message_loop());
 
     // Test that all events are dispatched before the Thread object is
     // destroyed.  We do this by dispatching a sleep event before the
     // event that will toggle our sentinel value.
-    a.message_loop()->PostTask(FROM_HERE, new SleepSome(500));
+    a.message_loop()->PostTask(FROM_HERE, new SleepSome(20));
     a.message_loop()->PostTask(FROM_HERE, new ToggleValue(&was_invoked));
   }
   EXPECT_TRUE(was_invoked);
 }
 
-namespace {
-  class DummyTask : public Task {
-   public:
-    explicit DummyTask(int* counter) : counter_(counter) {
-    }
-    void Run() {
-      // Let's make sure that no other thread is running while we
-      // are executing this code.  The sleeps help us assert that.
-      int initial_value = *counter_;
-      Sleep(1);
-      ++(*counter_);
-      Sleep(1);
-      int final_value = *counter_;
-      DCHECK(final_value == initial_value + 1);
-    }
-   private:
-    int* counter_;
-  };
+TEST(ThreadTest, StopSoon) {
+  Thread a("StopSoon");
+  EXPECT_TRUE(a.Start());
+  EXPECT_TRUE(a.message_loop());
+  a.StopSoon();
+  EXPECT_FALSE(a.message_loop());
+  a.StopSoon();
+  EXPECT_FALSE(a.message_loop());
 }
 
-namespace {
-  // This task just continuously posts events to its thread, keeping it well
-  // saturated with work.  If our thread interlocking is not fair, then we will
-  // never exit.
-  class BusyTask : public Task {
-   public:
-    explicit BusyTask(HANDLE quit_event) : quit_event_(quit_event) {
-    }
-    void Run() {
-      if (WaitForSingleObject(quit_event_, 0) != WAIT_OBJECT_0)
-        MessageLoop::current()->PostTask(FROM_HERE, new BusyTask(quit_event_));
-    }
-   private:
-    HANDLE quit_event_;
-  };
-
-  // This task just tries to set the quit sentinel to signal the busy thread
-  // to stop doing work.
-  class InterruptBusyTask : public Task {
-   public:
-    explicit InterruptBusyTask(HANDLE quit_event) : quit_event_(quit_event) {
-    }
-    void Run() {
-      SetEvent(quit_event_);
-    }
-   private:
-    HANDLE quit_event_;
-  };
+TEST(ThreadTest, ThreadName) {
+  Thread a("ThreadName");
+  EXPECT_TRUE(a.Start());
+  EXPECT_EQ("ThreadName", a.thread_name());
 }
-