Add joinable option to SimpleThread::Options as was just done for Thread.

base/threading/simple_thread.h

Index: base/threading/simple_thread.h
diff --git a/base/threading/simple_thread.h b/base/threading/simple_thread.h
index 3deeb1018cb573c3f5e48d2b17b7c16a428aa476..f4a7b67e760a64cd1b5e417670a661c82d885046 100644
--- a/base/threading/simple_thread.h
+++ b/base/threading/simple_thread.h
@@ -46,8 +46,11 @@
 #include <string>
 #include <vector>
 
+#include "base/atomic_ref_count.h"
 #include "base/base_export.h"
 #include "base/compiler_specific.h"
+#include "base/logging.h"
+#include "base/macros.h"
 #include "base/synchronization/lock.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
@@ -58,25 +61,27 @@ namespace base {
 // virtual Run method, or you can use the DelegateSimpleThread interface.
 class BASE_EXPORT SimpleThread : public PlatformThread::Delegate {
  public:
-  class BASE_EXPORT Options {
+  struct BASE_EXPORT Options {
    public:
-    Options() : stack_size_(0), priority_(ThreadPriority::NORMAL) {}
-    explicit Options(ThreadPriority priority)
-        : stack_size_(0), priority_(priority) {}
-    ~Options() {}
+    Options() = default;
+    explicit Options(ThreadPriority priority_in) : priority(priority_in) {}
+    ~Options() = default;
 
-    // We use the standard compiler-supplied copy constructor.
+    // Allow copies.
+    Options(const Options& other) = default;
+    Options& operator=(const Options& other) = default;
 
     // A custom stack size, or 0 for the system default.
-    void set_stack_size(size_t size) { stack_size_ = size; }
-    size_t stack_size() const { return stack_size_; }
+    size_t stack_size = 0;
 
-    // A custom thread priority.
-    void set_priority(ThreadPriority priority) { priority_ = priority; }
-    ThreadPriority priority() const { return priority_; }
-   private:
-    size_t stack_size_;
-    ThreadPriority priority_;
+    ThreadPriority priority = ThreadPriority::NORMAL;
+
+    // If false, the underlying thread's PlatformThreadHandle will not be kept
+    // around and as such the SimpleThread instance will not be Join()able and
+    // must not be deleted before Run() is invoked. After that, it's up to
+    // the subclass to determine when it is safe to delete itself, see
+    // DelegateSimpleThread for a safe option.
+    bool joinable = true;
   };
 
   // Create a SimpleThread.  |options| should be used to manage any specific
@@ -106,7 +111,7 @@ class BASE_EXPORT SimpleThread : public PlatformThread::Delegate {
   // Return True if Start() has ever been called.
   bool HasBeenStarted();
 
-  // Return True if Join() has evern been called.
+  // Return True if Join() has ever been called.
   bool HasBeenJoined() { return joined_; }
 
   // Overridden from PlatformThread::Delegate:
@@ -116,19 +121,44 @@ class BASE_EXPORT SimpleThread : public PlatformThread::Delegate {
   const std::string name_prefix_;
   std::string name_;
   const Options options_;
-  PlatformThreadHandle thread_;  // PlatformThread handle, invalid after Join!
+  PlatformThreadHandle thread_;  // PlatformThread handle, reset after Join.
   WaitableEvent event_;          // Signaled if Start() was ever called.
-  PlatformThreadId tid_;         // The backing thread's id.
-  bool joined_;                  // True if Join has been called.
+  PlatformThreadId tid_ = kInvalidThreadId;  // The backing thread's id.
+  bool joined_ = false;                      // True if Join has been called.
+
+  DISALLOW_COPY_AND_ASSIGN(SimpleThread);
 };
 
+// A SimpleThread which delegates Run() to its Delegate. The registered Delegate
+// must outlive its Run() invocation(s). As such, non-joinable
+// DelegateSimpleThread are safe to delete after Start() so long as the
+// Delegate's lifetime respects the aforementioned condition.
 class BASE_EXPORT DelegateSimpleThread : public SimpleThread {
  public:
   class BASE_EXPORT Delegate {
    public:
-    Delegate() { }
-    virtual ~Delegate() { }
+    Delegate();
+    virtual ~Delegate();
+
+    // Invokes Run() and manages |active_runners_|.
+    void RunHelper();
+
+   private:
+    // Run() is private pure virtual because only RunHelper() should invoke it,

[danakj, 2016/08/12 18:38:00]

but.... you can just upcast it and call Run() directly tho (even accidentally).

[gab, 2016/08/12 18:45:04]

You mean because the impl can override it as public? I guess but someone
explicitly calling run on a Delegate would be obviously wrong IMO -- even
RunHelper is only intended to be called by DelegateSimpleThread.

This is mostly hinting people further to do the right thing. Even if someone
checks in something terribly wrong, worst case is just that the DCHECKs are
skipped (provided they got everything else right).

[danakj, 2016/08/12 18:55:17]

I mean like

SimpleThread* thread = new DelegateSimpleThread;
thread->Run();

[gab, 2016/08/12 19:05:49]

This is different. The code this comment is on is Delegate::Run().
DelegateSimpleThread::Run() is on line 171. Calling Run() from the owner is
always wrong it's intended to be called by ThreadMain (both for
DelegateSimpleThread and SimpleThread), this CL doesn't change that.

[danakj, 2016/08/12 20:49:10]

Oooh, nested class, I see. rip reading

+    // but it's still up to the implementer to define it.
     virtual void Run() = 0;
+
+#if DCHECK_IS_ON()
+    // Maintains a count of active Run() calls -- there can be more than one
+    // when Delegate is used in a DelegateSimpleThreadPool.
+    AtomicRefCount active_runners_ = 0;
+
+    // Signaled when |active_runners_| is decremented, auto-reset after the
+    // signal is observed.
+    WaitableEvent active_runners_decremented_;
+#endif  // DCHECK_IS_ON()
+
+    DISALLOW_COPY_AND_ASSIGN(Delegate);
   };
 
   DelegateSimpleThread(Delegate* delegate,
@@ -142,6 +172,12 @@ class BASE_EXPORT DelegateSimpleThread : public SimpleThread {
 
  private:
   Delegate* delegate_;
+
+#if DCHECK_IS_ON()
+  bool ran_ = false;
+#endif
+
+  DISALLOW_COPY_AND_ASSIGN(DelegateSimpleThread);
 };
 
 // DelegateSimpleThreadPool allows you to start up a fixed number of threads,
@@ -186,6 +222,8 @@ class BASE_EXPORT DelegateSimpleThreadPool
   std::queue<Delegate*> delegates_;
   base::Lock lock_;            // Locks delegates_
   WaitableEvent dry_;    // Not signaled when there is no work to do.
+
+  DISALLOW_COPY_AND_ASSIGN(DelegateSimpleThreadPool);
 };
 
 }  // namespace base