Modernize base::Thread

base/threading/thread.h

 // 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.
 
 #ifndef BASE_THREADING_THREAD_H_
 #define BASE_THREADING_THREAD_H_
 
 #include <stddef.h>
 
 #include <memory>
 #include <string>
 
 #include "base/base_export.h"
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/timer_slack.h"
+#include "base/sequence_checker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/synchronization/lock.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
 #include "build/build_config.h"
 
 namespace base {
 
 class MessagePump;
 class RunLoop;
 
 // A simple thread abstraction that establishes a MessageLoop on a new thread.
 // The consumer uses the MessageLoop of the thread to cause code to execute on
 // the thread.  When this object is destroyed the thread is terminated.  All
 // pending tasks queued on the thread's message loop will run to completion
 // before the thread is terminated.
 //
 // WARNING! SUBCLASSES MUST CALL Stop() IN THEIR DESTRUCTORS!  See ~Thread().
 //
 // After the thread is stopped, the destruction sequence is:
 //
 //  (1) Thread::CleanUp()
 //  (2) MessageLoop::~MessageLoop
 //  (3.b)    MessageLoop::DestructionObserver::WillDestroyCurrentMessageLoop
+//
+// This API is not thread-safe: unless indicated otherwise it's methods are only
+// valid from the owning sequence (which is the one from which Start() is
+// invoked, should it differ from the one on which it was constructed).
 class BASE_EXPORT Thread : PlatformThread::Delegate {
  public:
   struct BASE_EXPORT Options {
     typedef Callback<std::unique_ptr<MessagePump>()> MessagePumpFactory;
 
     Options();
     Options(MessageLoop::Type type, size_t size);
     Options(const Options& other);
     ~Options();
 
     // Specifies the type of message loop that will be allocated on the thread.
     // This is ignored if message_pump_factory.is_null() is false.
-    MessageLoop::Type message_loop_type;
+    MessageLoop::Type message_loop_type = MessageLoop::TYPE_DEFAULT;
 
     // Specifies timer slack for thread message loop.
-    TimerSlack timer_slack;
+    TimerSlack timer_slack = TIMER_SLACK_NONE;
 
     // Used to create the MessagePump for the MessageLoop. The callback is Run()
     // on the thread. If message_pump_factory.is_null(), then a MessagePump
     // appropriate for |message_loop_type| is created. Setting this forces the
     // MessageLoop::Type to TYPE_CUSTOM.
     MessagePumpFactory message_pump_factory;
 
     // Specifies the maximum stack size that the thread is allowed to use.
     // This does not necessarily correspond to the thread's initial stack size.
     // A value of 0 indicates that the default maximum should be used.
-    size_t stack_size;
+    size_t stack_size = 0;
 
     // Specifies the initial thread priority.
-    ThreadPriority priority;
+    ThreadPriority priority = ThreadPriority::NORMAL;
   };
 
   // Constructor.
   // name is a display string to identify the thread.
   explicit Thread(const std::string& name);
 
   // Destroys the thread, stopping it if necessary.
   //
   // NOTE: ALL SUBCLASSES OF Thread MUST CALL Stop() IN THEIR DESTRUCTORS (or
   // guarantee Stop() is explicitly called before the subclass is destroyed).
@@ skipping 70 matching lines @@
   void StopSoon();
 
   // Returns the message loop for this thread.  Use the MessageLoop's
   // PostTask methods to execute code on the thread.  This only returns
   // non-null after a successful call to Start.  After Stop has been called,
   // this will return nullptr.
   //
   // 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_; }
+  // In addition to this Thread's owner, this can also safely be called from the

[danakj, 2016/07/21 19:43:26]

"Thread's owning sequence" sounds more strictly correct. "owner" sounds more
like memory ownership ie via a unique_ptr.

[gab, 2016/07/22 16:02:55]

Done.

+  // underlying thread itself.
+  MessageLoop* message_loop() const {
+    // This class doesn't provide synchronization around |message_loop_| and as
+    // such only the owner should access it (and the underlying thread which
+    // never sees it before it's set). In practice, many callers are coming from
+    // unrelated threads but provide their own implicit (e.g. memory barriers
+    // from task posting) or explicit (e.g. locks) synchronization making the
+    // access of |message_loop_| safe... Changing all of those callers is
+    // unfeasible; instead verify that they can reliably see
+    // |message_loop_ != nullptr| without synchronization as a proof that their
+    // external synchronization catches the unsynchronized effects of Start().
+    DCHECK(sequence_checker_.CalledOnValidSequencedThread() ||
+           id_ == PlatformThread::CurrentId() || message_loop_)
+        << id_ << " vs " << PlatformThread::CurrentId();
+    return message_loop_;
+  }
 
   // Returns a TaskRunner for this thread. Use the TaskRunner's PostTask
   // methods to execute code on the thread. Returns nullptr if the thread is not
   // running (e.g. before Start or after Stop have been called). Callers can
   // hold on to this even after the thread is gone; in this situation, attempts
   // to PostTask() will fail.
+  //
+  // In addition to this Thread's owner, this can also safely be called from the

[danakj, 2016/07/21 19:43:26]

ditto "owner"

[gab, 2016/07/22 16:02:55]

Done.

+  // underlying thread itself.
   scoped_refptr<SingleThreadTaskRunner> task_runner() const {
+    // Ref. comment on |message_loop()| DCHECK.

[danakj, 2016/07/21 19:43:26]

"Refer to the DCHECK and comment inside |message_loop()|.

[gab, 2016/07/22 16:02:55]

Done.

+    DCHECK(sequence_checker_.CalledOnValidSequencedThread() ||
+           id_ == PlatformThread::CurrentId() || message_loop_)
+        << id_ << " vs " << PlatformThread::CurrentId();
     return message_loop_ ? message_loop_->task_runner() : nullptr;
   }
 
   // Returns the name of this thread (for display in debugger too).
   const std::string& thread_name() const { return name_; }
 
   // Returns the thread ID.  Should not be called before the first Start*()
   // call.  Keeps on returning the same ID even after a Stop() call. The next
   // Start*() call renews the ID.
   //
   // WARNING: This function will block if the thread hasn't started yet.
   //
+  // This method is thread-safe.
   PlatformThreadId GetThreadId() const;
 
   // Returns true if the thread has been started, and not yet stopped.
   bool IsRunning() const;
 
  protected:
   // Called just prior to starting the message loop
   virtual void Init() {}
 
   // Called to start the run loop
   virtual void Run(RunLoop* run_loop);
 
   // Called just after the message loop ends
   virtual void CleanUp() {}
 
   static void SetThreadWasQuitProperly(bool flag);
   static bool GetThreadWasQuitProperly();
 
   void set_message_loop(MessageLoop* message_loop) {
+    DCHECK(sequence_checker_.CalledOnValidSequencedThread());
     message_loop_ = message_loop;
   }
 
  private:
 #if defined(OS_WIN)
   enum ComStatus {
     NONE,
     STA,
     MTA,
   };
 #endif
 
   // PlatformThread::Delegate methods:
   void ThreadMain() override;
 
   void ThreadQuitHelper();
 
 #if defined(OS_WIN)
   // Whether this thread needs to initialize COM, and if so, in what mode.
-  ComStatus com_status_;
+  ComStatus com_status_ = NONE;
 #endif
 
   // If true, we're in the middle of stopping, and shouldn't access
   // |message_loop_|. It may non-nullptr and invalid.
   // Should be written on the thread that created this thread. Also read data
   // could be wrong on other threads.
-  bool stopping_;
+  bool stopping_ = false;
 
   // True while inside of Run().
-  bool running_;
+  bool running_ = false;
   mutable base::Lock running_lock_;  // Protects |running_|.
 
   // The thread's handle.
   PlatformThreadHandle thread_;
-  mutable base::Lock thread_lock_;  // Protects |thread_|.
 
   // The thread's id once it has started.
-  PlatformThreadId id_;
+  PlatformThreadId id_ = kInvalidThreadId;
   mutable WaitableEvent id_event_;  // Protects |id_|.
 
   // The thread's MessageLoop and RunLoop. Valid only while the thread is alive.
   // Set by the created thread.
-  MessageLoop* message_loop_;
-  RunLoop* run_loop_;
+  MessageLoop* message_loop_ = nullptr;
+  RunLoop* run_loop_ = nullptr;
 
   // Stores Options::timer_slack_ until the message loop has been bound to
   // a thread.
-  TimerSlack message_loop_timer_slack_;
+  TimerSlack message_loop_timer_slack_ = TIMER_SLACK_NONE;
 
   // The name of the thread.  Used for debugging purposes.
-  std::string name_;
+  const std::string name_;

[danakj, 2016/07/21 19:43:26]

Yay const

[gab, 2016/07/22 16:02:55]

:-)

 
   // Signaled when the created thread gets ready to use the message loop.
   mutable WaitableEvent start_event_;
 
+  // This class is not thread-safe.

[danakj, 2016/07/21 19:43:26]

...thread-safe, use this to verify access from the owning sequence of the
Thread.

+  SequenceChecker sequence_checker_;

[danakj, 2016/07/21 19:43:26]

nit: maybe call this owning_sequence_checker_? or something u like better, but i
like when checkers say which sequence/thread they are checking for.

[gab, 2016/07/22 16:02:55]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(Thread);
 };
 
 }  // namespace base
 
 #endif  // BASE_THREADING_THREAD_H_