Modernize base::Thread

base/threading/thread.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 "base/threading/thread.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/lazy_instance.h"
 #include "base/location.h"
@@ skipping 15 matching lines @@
 
 // We use this thread-local variable to record whether or not a thread exited
 // because its Stop method was called.  This allows us to catch cases where
 // MessageLoop::QuitWhenIdle() is called directly, which is unexpected when
 // using a Thread to setup and run a MessageLoop.
 base::LazyInstance<base::ThreadLocalBoolean> lazy_tls_bool =
     LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace
 
-Thread::Options::Options()
-    : message_loop_type(MessageLoop::TYPE_DEFAULT),
-      timer_slack(TIMER_SLACK_NONE),
-      stack_size(0),
-      priority(ThreadPriority::NORMAL) {
-}
+Thread::Options::Options() = default;
 
-Thread::Options::Options(MessageLoop::Type type,
-                         size_t size)
-    : message_loop_type(type),
-      timer_slack(TIMER_SLACK_NONE),
-      stack_size(size),
-      priority(ThreadPriority::NORMAL) {
-}
+Thread::Options::Options(MessageLoop::Type type, size_t size)
+    : message_loop_type(type), stack_size(size) {}
 
 Thread::Options::Options(const Options& other) = default;
 
-Thread::Options::~Options() {
-}
+Thread::Options::~Options() = default;
 
 Thread::Thread(const std::string& name)
-    :
-#if defined(OS_WIN)
-      com_status_(NONE),
-#endif
-      stopping_(false),
-      running_(false),
-      thread_(0),
-      id_(kInvalidThreadId),
-      id_event_(WaitableEvent::ResetPolicy::MANUAL,
+    : id_event_(WaitableEvent::ResetPolicy::MANUAL,
                 WaitableEvent::InitialState::NOT_SIGNALED),
-      message_loop_(nullptr),
-      message_loop_timer_slack_(TIMER_SLACK_NONE),
       name_(name),
       start_event_(WaitableEvent::ResetPolicy::MANUAL,
                    WaitableEvent::InitialState::NOT_SIGNALED) {
+  // Only bind the sequence on Start(): the state is constant between
+  // construction and Start() and it's thus valid for Start() to be called on
+  // another sequence as long as every other operation is then performed on that
+  // sequence.
+  owning_sequence_checker_.DetachFromSequence();
 }
 
 Thread::~Thread() {
   Stop();
 }
 
 bool Thread::Start() {
+  DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
+
   Options options;
 #if defined(OS_WIN)
   if (com_status_ == STA)
     options.message_loop_type = MessageLoop::TYPE_UI;
 #endif
   return StartWithOptions(options);
 }
 
 bool Thread::StartWithOptions(const Options& options) {
+  DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
   DCHECK(!message_loop_);
+  DCHECK(!IsRunning());
 #if defined(OS_WIN)
   DCHECK((com_status_ != STA) ||
       (options.message_loop_type == MessageLoop::TYPE_UI));
 #endif
 
   // Reset |id_| here to support restarting the thread.
   id_event_.Reset();
   id_ = kInvalidThreadId;
 
   SetThreadWasQuitProperly(false);
 
   MessageLoop::Type type = options.message_loop_type;
   if (!options.message_pump_factory.is_null())
     type = MessageLoop::TYPE_CUSTOM;
 
   message_loop_timer_slack_ = options.timer_slack;
-  std::unique_ptr<MessageLoop> message_loop =
+  std::unique_ptr<MessageLoop> message_loop_owned =
       MessageLoop::CreateUnbound(type, options.message_pump_factory);
-  message_loop_ = message_loop.get();
+  message_loop_ = message_loop_owned.get();
   start_event_.Reset();
 
-  // Hold the thread_lock_ while starting a new thread, so that we can make sure
-  // that thread_ is populated before the newly created thread accesses it.
+  // Hold |thread_lock_| while starting the new thread to synchronize with
+  // Stop() while it's not guaranteed to be sequenced (until crbug/629139 is
+  // fixed).
   {
     AutoLock lock(thread_lock_);
     if (!PlatformThread::CreateWithPriority(options.stack_size, this, &thread_,
                                             options.priority)) {
       DLOG(ERROR) << "failed to create thread";
       message_loop_ = nullptr;
       return false;
     }
   }
 
-  // The ownership of message_loop is managemed by the newly created thread
+  // The ownership of |message_loop_| is managed by the newly created thread
   // within the ThreadMain.
-  ignore_result(message_loop.release());
+  ignore_result(message_loop_owned.release());
 
   DCHECK(message_loop_);
   return true;
 }
 
 bool Thread::StartAndWaitForTesting() {
+  DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
   bool result = Start();
   if (!result)
     return false;
   WaitUntilThreadStarted();
   return true;
 }
 
 bool Thread::WaitUntilThreadStarted() const {
+  DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
   if (!message_loop_)
     return false;
   base::ThreadRestrictions::ScopedAllowWait allow_wait;
   start_event_.Wait();
   return true;
 }
 
 void Thread::Stop() {
+  // TODO(gab): Fix improper usage of this API (http://crbug.com/629139) and
+  // enable this check, until then synchronization with Start() via
+  // |thread_lock_| is required...

[Wez, 2016/07/25 21:20:29]

FWIW, this comment as worded implies that callers were mis-using Start()/Stop()
in calling them from different Sequences, where AFAICT  the issue is actually
that you're specifically changing the API contract in this CL.

It would be helpful to have the CL description and/or bug document why imposing
the same-Sequence restriction is preferable to the use of locks here. Right now
there's a cyclical reference between CL and bug rationale. :P

[gab, 2016/07/26 02:48:23]

Generally in Chromium, unless specified otherwise, classes are not to be assumed
as thread-safe. It is good practice nowadays to use a Sequence/ThreadChecker to
explicitly enforce such requirements when non-obvious (especially in base/ APIs
used all over the map). base::Thread being very old was lacking such checks, but
it is clear from its design that this was the intent (no locks or
synchronization on all state -- except on |thread_handle_| but that's probably
the remains of a scheme where it was used more broadly and can go away now). The
previous comment on Thread::StopSoon() makes this clear : "We should only be
called on the same thread that started us.".

Also, on IsRunning() : "Note that stopping_ is touched only on the same thread
so we need no locking here."

Another example that this was intended to be used as such, TestIOThread which
uses the default Thread::Start() and Thread::Stop() documents that :
"|Start()|/|Stop()| should only be called from the main (creation) thread."
(comment just updated in a tangentially related CL but was otherwise there for
ages before : https://codereview.chromium.org/2170953002/).

If you want to propose making this API thread-safe, go ahead, but *that* would
be a change in API IMO. Whereas this CL merely explicitly states what was the
intended API and implicit assumptions of "non-thread-safety unless specified
otherwise", it's a no-op production code wise (adding locks wouldn't be).

+  // DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
   AutoLock lock(thread_lock_);
+
   if (thread_.is_null())
     return;
 
   StopSoon();
 
   // Wait for the thread to exit.
   //
-  // TODO(darin): Unfortunately, we need to keep message_loop_ around until
+  // TODO(darin): Unfortunately, we need to keep |message_loop_| around until
   // the thread exits.  Some consumers are abusing the API.  Make them stop.
   //
   PlatformThread::Join(thread_);
   thread_ = base::PlatformThreadHandle();
 
-  // The thread should nullify message_loop_ on exit.
+  // The thread should nullify |message_loop_| on exit (note: Join() adds an
+  // implicit memory barrier and no lock is thus required for this check).
   DCHECK(!message_loop_);
 
   stopping_ = false;
 }
 
 void Thread::StopSoon() {
-  // We should only be called on the same thread that started us.
-
-  DCHECK_NE(GetThreadId(), PlatformThread::CurrentId());
+  // TODO(gab): Fix improper usage of this API (http://crbug.com/629139) and
+  // enable this check.
+  // DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
 
   if (stopping_ || !message_loop_)
     return;
 
   stopping_ = true;
   task_runner()->PostTask(
       FROM_HERE, base::Bind(&Thread::ThreadQuitHelper, Unretained(this)));
 }
 
 PlatformThreadId Thread::GetThreadId() const {
   // If the thread is created but not started yet, wait for |id_| being ready.
   base::ThreadRestrictions::ScopedAllowWait allow_wait;
   id_event_.Wait();
   return id_;
 }
 
 bool Thread::IsRunning() const {
-  // If the thread's already started (i.e. message_loop_ is non-null) and
-  // not yet requested to stop (i.e. stopping_ is false) we can just return
-  // true. (Note that stopping_ is touched only on the same thread that
-  // starts / started the new thread so we need no locking here.)
+  // TODO(gab): Fix improper usage of this API (http://crbug.com/629139) and
+  // enable this check.
+  // DCHECK(owning_sequence_checker_.CalledOnValidSequencedThread());
+
+  // If the thread's already started (i.e. |message_loop_| is non-null) and not
+  // yet requested to stop (i.e. |stopping_| is false) we can just return true.
+  // (Note that |stopping_| is touched only on the same sequence that starts /
+  // started the new thread so we need no locking here.)
   if (message_loop_ && !stopping_)
     return true;
-  // Otherwise check the running_ flag, which is set to true by the new thread
+  // Otherwise check the |running_| flag, which is set to true by the new thread
   // only while it is inside Run().
   AutoLock lock(running_lock_);
   return running_;
 }
 
 void Thread::Run(RunLoop* run_loop) {
+  // Overridable protected method to be called from our |thread_| only.
+  DCHECK_EQ(id_, PlatformThread::CurrentId());

[Wez, 2016/07/25 21:22:36]

What is this DCHECK intended to protect against? Comment implies you're
concerned about sub-classes calling it on themselves..?

[gab, 2016/07/26 02:48:23]

As is often the case with DCHECKs, it's intended as documentation of the
expected contract. Since every other method uses |owning_sequence_checker_|, I
wanted to make it clear that it wasn't omitted here by accident but rather
because this runs on the underlying thread, not on the owning sequence.

The purpose of this CL is essentially that I had to grok this API inside-and-out
to write https://codereview.chromium.org/2135413003/ and then decided to drop
the knowledge I built doing so in the codebase to help future readers as well as
reinforce discovered assumptions with runtime checks (which indeed caught a few
things as you saw).

+
   run_loop->Run();
 }
 
+// static
 void Thread::SetThreadWasQuitProperly(bool flag) {
   lazy_tls_bool.Pointer()->Set(flag);
 }
 
+// static
 bool Thread::GetThreadWasQuitProperly() {
   bool quit_properly = true;
 #ifndef NDEBUG
   quit_properly = lazy_tls_bool.Pointer()->Get();
 #endif
   return quit_properly;
 }
 
 void Thread::ThreadMain() {
   // First, make GetThreadId() available to avoid deadlocks. It could be called
   // any place in the following thread initialization code.
   id_ = PlatformThread::CurrentId();
   DCHECK_NE(kInvalidThreadId, id_);
   id_event_.Signal();
 
   // Complete the initialization of our Thread object.
   PlatformThread::SetName(name_.c_str());
   ANNOTATE_THREAD_NAME(name_.c_str());  // Tell the name to race detector.
 
-  // Lazily initialize the message_loop so that it can run on this thread.
+  // Lazily initialize the |message_loop| so that it can run on this thread.
   DCHECK(message_loop_);
   std::unique_ptr<MessageLoop> message_loop(message_loop_);
   message_loop_->BindToCurrentThread();
   message_loop_->SetTimerSlack(message_loop_timer_slack_);
 
 #if defined(OS_WIN)
   std::unique_ptr<win::ScopedCOMInitializer> com_initializer;
   if (com_status_ != NONE) {
     com_initializer.reset((com_status_ == STA) ?
         new win::ScopedCOMInitializer() :
@@ skipping 40 matching lines @@
   run_loop_ = nullptr;
 }
 
 void Thread::ThreadQuitHelper() {
   DCHECK(run_loop_);
   run_loop_->QuitWhenIdle();
   SetThreadWasQuitProperly(true);
 }
 
 }  // namespace base