Enables clean VM shutdown.

runtime/vm/thread_pool.h

Index: runtime/vm/thread_pool.h
diff --git a/runtime/vm/thread_pool.h b/runtime/vm/thread_pool.h
index 792aef7b23898d4cda77cf6345f11b6dfe0ff770..931dd760ee913e41c50d8af623ecfef21aade3cf 100644
--- a/runtime/vm/thread_pool.h
+++ b/runtime/vm/thread_pool.h
@@ -10,7 +10,7 @@
 
 namespace dart {
 
-class ThreadPool {
+class ThreadPool : public AllStatic {

[turnidge, 2015/06/30 22:15:47]

Is there a motivating reason to make this AllStatic?  I prefer to keep it
non-static.  This allows us to have mock thread pools, alternate/experimental
thread pools, or multiple/dedicated thread pools in the future.

I see statics as kind of a global variable that we use when necessary but I
don't see any reason to here.  I see that we save some parameters on some calls,
but that's true of any global.

[zra, 2015/07/20 22:23:39]

I changed to AllStatic to enforce having only one thread pool because I was
worried that having multiple thread pools without a clear design of what to do
with them could be dangerous and/or confusing. Examples: Which thread pool do
you assign work to? What do you do when one thread pool has lots of idle
threads, but work keeps being assigned to some other thread pool? I think if we
don't have answers to questions like that, we should make sure we only have one.

It could make sense for testing, but isn't really necessary.

  public:
   // Subclasses of Task are able to run on a ThreadPool.
   class Task {
@@ -27,27 +27,28 @@ class ThreadPool {
     DISALLOW_COPY_AND_ASSIGN(Task);
   };
 
-  ThreadPool();
-
   // Shuts down this thread pool.  Causes workers to terminate
   // themselves when they are active again.
-  ~ThreadPool();
+  static bool Shutdown();
 
   // Runs a task on the thread pool.
-  void Run(Task* task);
+  static void Run(Task* task);
 
   // Some simple stats.
-  uint64_t workers_running() const { return count_running_; }
-  uint64_t workers_idle() const { return count_idle_; }
-  uint64_t workers_started() const { return count_started_; }
-  uint64_t workers_stopped() const { return count_stopped_; }
+  static uint64_t workers_running() { return count_running_; }
+  static uint64_t workers_idle() { return count_idle_; }
+  static uint64_t workers_started() { return count_started_; }
+  static uint64_t workers_stopped() { return count_stopped_; }
 
- private:
-  friend class ThreadPoolTestPeer;
+  static bool shutdown_timeout_fatal() { return shutdown_timeout_fatal_; }
+  static void set_shutdown_timeout_fatal(bool shutdown_timeout_fatal) {
+    shutdown_timeout_fatal_ = shutdown_timeout_fatal;
+  }
 
+ private:
   class Worker {
    public:
-    explicit Worker(ThreadPool* pool);
+    Worker();
 
     // Sets a task on the worker.
     void SetTask(Task* task);
@@ -56,24 +57,30 @@ class ThreadPool {
     // after a task has been set by the initial call to SetTask().
     void StartThread();
 
-    // Main loop for a worker.
-    void Loop();
+    // Main loop for a worker. Returns true if worker is removed from thread
+    // lists, false otherwise.
+    bool Loop();
 
     // Causes worker to terminate eventually.
     void Shutdown();
 
+    // Get the Worker's thread id.
+    ThreadId id() { return id_; }
+
    private:
     friend class ThreadPool;
 
     // The main entry point for new worker threads.
     static void Main(uword args);
 
-    bool IsDone() const { return pool_ == NULL; }
+    bool IsDone() const { return done_; }
 
     // Fields owned by Worker.
     Monitor monitor_;
-    ThreadPool* pool_;
+    bool done_;
     Task* task_;
+    ThreadId id_;
+    bool started_;
 
     // Fields owned by ThreadPool.  Workers should not look at these
     // directly.  It's like looking at the sun.
@@ -81,34 +88,39 @@ class ThreadPool {
     Worker* all_next_;   // Protected by ThreadPool::mutex_
     Worker* idle_next_;  // Protected by ThreadPool::mutex_
 
+    Worker* shutdown_next_;  // Protected by ThreadPool::exit_monitor
+
     DISALLOW_COPY_AND_ASSIGN(Worker);
   };
 
-  void Shutdown();
-
   // Expensive.  Use only in assertions.
-  bool IsIdle(Worker* worker);
+  static bool IsIdle(Worker* worker);
+
+  static bool RemoveWorkerFromIdleList(Worker* worker);
+  static bool RemoveWorkerFromAllList(Worker* worker);
 
-  bool RemoveWorkerFromIdleList(Worker* worker);
-  bool RemoveWorkerFromAllList(Worker* worker);
+  static void AddWorkerToShutdownList(Worker* worker);
+  static bool RemoveWorkerFromShutdownList(Worker* worker);
 
   // Worker operations.
-  void SetIdle(Worker* worker);
-  bool ReleaseIdleWorker(Worker* worker);
-
-  Mutex mutex_;
-  bool shutting_down_;
-  Worker* all_workers_;
-  Worker* idle_workers_;
-  uint64_t count_started_;
-  uint64_t count_stopped_;
-  uint64_t count_running_;
-  uint64_t count_idle_;
-
-  static Monitor* exit_monitor_;  // Used only in testing.
-  static int* exit_count_;        // Used only in testing.
-
-  DISALLOW_COPY_AND_ASSIGN(ThreadPool);
+  static void SetIdle(Worker* worker);
+  static bool ReleaseIdleWorker(Worker* worker);
+
+  static Mutex mutex_;
+  static bool shutting_down_;
+  static Worker* all_workers_;
+  static Worker* idle_workers_;
+  static uint64_t count_started_;
+  static uint64_t count_stopped_;
+  static uint64_t count_running_;
+  static uint64_t count_idle_;
+  static bool shutdown_timeout_fatal_;
+
+  static Monitor exit_monitor_;
+  static Worker* shutting_down_workers_;
+
+  // For testing when shutdown_timeout_fatal_ == false;
+  static bool shutdown_timeout_;
 };
 
 }  // namespace dart