TaskScheduler [5/9] Task Tracker

base/task_scheduler/shutdown_manager.cc

+// Copyright 2016 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/task_scheduler/shutdown_manager.h"
+
+#include "base/atomicops.h"
+
+namespace base {
+namespace internal {
+
+ShutdownManager::ShutdownManager()
+    : cv_(lock_.CreateConditionVariable()),
+      num_tasks_blocking_shutdown_(0),
+      is_shutting_down_(false),
+      shutdown_completed_(false) {}
+
+ShutdownManager::~ShutdownManager() = default;
+
+void ShutdownManager::Shutdown() {
+  AutoSchedulerLock auto_lock(lock_);
+
+  DCHECK(!is_shutting_down_) << "ShutdownManager::Shutdown can't be called "
+                                "simultaneously on multiple threads.";

[gab, 2016/02/23 22:28:22]

How about (as mentioned in header):

DCHECK(!shutdown_completed_ || is_shuttting_down) 
    << "Shutdown() should only be invoked once and from one thread only.";

[fdoray, 2016/02/26 15:53:28]

Done.

+  is_shutting_down_ = true;
+
+  // Wait until the number of tasks blocking shutdown is zero.
+  while (num_tasks_blocking_shutdown_ != 0)
+    cv_->Wait();
+
+  is_shutting_down_ = false;
+  shutdown_completed_ = true;
+}
+
+bool ShutdownManager::ShouldPostTask(TaskShutdownBehavior shutdown_behavior) {
+  AutoSchedulerLock auto_lock(lock_);

[gab, 2016/02/23 22:28:23]

Please highlight in CL description that you have a plan for a lock free atomic
version of ShutdownManager but checking this one in first will make it easier to
bring that as an incremental CL.

[fdoray, 2016/02/26 15:53:28]

Done.

+
+  if (shutdown_completed_)
+    return false;
+
+  if (shutdown_behavior == TaskShutdownBehavior::BLOCK_SHUTDOWN) {
+    ++num_tasks_blocking_shutdown_;
+    return true;
+  }
+
+  return !is_shutting_down_;
+}
+
+bool ShutdownManager::ShouldScheduleTask(
+    TaskShutdownBehavior shutdown_behavior) {
+  AutoSchedulerLock auto_lock(lock_);
+
+  if (shutdown_completed_)
+    return false;
+
+  switch (shutdown_behavior) {
+    case TaskShutdownBehavior::BLOCK_SHUTDOWN: {

[robliao, 2016/02/24 01:22:44]

Nit: We generally don't use braces for switch statements unless we need
additional local variables.

[fdoray, 2016/02/26 15:53:28]

Done.

+      return true;
+    }
+
+    case TaskShutdownBehavior::SKIP_ON_SHUTDOWN: {
+      if (is_shutting_down_)
+        return false;
+      ++num_tasks_blocking_shutdown_;
+      return true;
+    }
+
+    case TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN: {
+      return !is_shutting_down_;
+    }
+  }
+}
+
+void ShutdownManager::DidExecuteTask(TaskShutdownBehavior shutdown_behavior) {
+  if (shutdown_behavior == TaskShutdownBehavior::BLOCK_SHUTDOWN ||
+      shutdown_behavior == TaskShutdownBehavior::SKIP_ON_SHUTDOWN) {
+    AutoSchedulerLock auto_lock(lock_);
+    --num_tasks_blocking_shutdown_;
+    if (num_tasks_blocking_shutdown_ == 0)
+      cv_->Signal();
+  }
+}
+
+void ShutdownManager::set_is_shutting_down_for_testing(bool is_shutting_down) {
+  is_shutting_down_ = is_shutting_down;
+  subtle::MemoryBarrier();
+}
+
+bool ShutdownManager::is_shutting_down_for_testing() const {
+  subtle::MemoryBarrier();

[gab, 2016/02/23 22:28:23]

Locking seems fine in test code (at least for now).

The header for atomicops.h says "If you plan to use these routines, you should
have a good reason, such as solid evidence that performance would otherwise
suffer, or there being no alternative."

and test code clearly doesn't qualify :-).

[fdoray, 2016/02/26 15:53:28]

Done.

+  return is_shutting_down_;
+}
+
+bool ShutdownManager::shutdown_completed() const {
+  subtle::MemoryBarrier();

[gab, 2016/02/23 22:28:22]

I'd say use a lock here for now (until you come up with a lock free version of
the entire thing).

It doesn't seem critical yet as this is only called on the outside of the
WorkerThread's main loop.

[fdoray, 2016/02/26 15:53:28]

Done.

+  return shutdown_completed_;
+}
+
+}  // namespace internal
+}  // namespace base