TaskScheduler [5/9] Task Tracker

base/task_scheduler/shutdown_manager.h

+// 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.
+
+#ifndef BASE_TASK_SCHEDULER_SHUTDOWN_MANAGER_H_
+#define BASE_TASK_SCHEDULER_SHUTDOWN_MANAGER_H_
+
+#include "base/base_export.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/synchronization/condition_variable.h"
+#include "base/task_scheduler/scheduler_lock.h"
+#include "base/task_scheduler/task_traits.h"
+
+namespace base {
+namespace internal {
+
+// Ensures proper shutdown behavior of the task scheduler. Unless otherwise
+// noted, the methods of this class are thread-safe.
+class BASE_EXPORT ShutdownManager {
+ public:
+  ShutdownManager();
+  ~ShutdownManager();
+
+  // Shuts down the task scheduler. After this is called, only tasks posted with
+  // the BLOCK_SHUTDOWN behavior are scheduled. Returns when:
+  // - All SKIP_ON_SHUTDOWN tasks that were already running have completed their
+  //   execution.
+  // - There is no pending BLOCK_SHUTDOWN tasks.

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

s/is/are/

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

Done.

+  // This cannot be called simultaneously on multiple threads.

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

// This should only be called on shutdown from one thread only.

(the previous form makes it sound like it's still okay to call this multiple
times, just not from multiple threads whereas I think we want to articulate this
should only be called once, from one thread)

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

Done. "Must only be called once" implies "from one thread only".

+  void Shutdown();
+
+  // Indicates whether a task with this |shutdown_behavior| should be posted.
+  bool ShouldPostTask(TaskShutdownBehavior shutdown_behavior);

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

This actually does more than return its opinion, it also records the task has
having been posted. With the API as-is it would be correct for an algorithm to
call this multiple times before posting a single task (not sure why but it
wouldn't be outright incorrect).

How about changing this to:

// Informs ShutdownManager that a task with |shutdown_behavior| is about to be
// posted. Returns true if the ShutdownManager approves of this operation (the
// task should otherwise not be posted).
bool RequestPostTask(TaskShutdownBehavior shutdown_behavior);

Another option is (even more explicit):

// Forwards |task| to |post_task_continuation| unless the current shutdown state
// prevents that in which case |task| is dropped.
void PostTask(const base::Callback<scoped_ptr<Task>> post_task_continuation,
              scoped_ptr<Task> task);

WDYT?

I like option 2 except for the fact that it adds ShutdownManager in the direct
task posting graph instead of a side dependency.

Given we also depend on the DidExecuteTask() call I guess sticking to option 1
as a similar "inform me" API is okay.

That brings me to 65/35 in favor of option 1 I think.. ;-)


EDIT:

So I just read the test which made it even more clear to me that this was
required and gave me a third idea (now including ShouldScheduleTask()):

// Forwards |task| to |post_task_continuation| unless the current shutdown state
// prevents that in which case |task| is dropped.
void PostTask(const base::Callback<scoped_ptr<Task>> post_task_continuation,
              scoped_ptr<Task> task);

// Returns a ScopedTask which wraps |task| unless the current shutdown state
// prevents |task|'s execution in which case |task| is dropped and the returned
// pointer is null. ShutdownManager will use the scope of the returned object
// when counting live tasks during shutdown.
ShutdownManager::ScopedTask RequestExecuteTask(scoped_ptr<Task> task);


where:

class ShutdownManager {
  class ScopedTask {
   public:
    ScopedTask(ShutdownManager* outer,
               scoped_ptr<Task> task) : outer_(outer), task_(std::move(task)) {}

    ScopedTask(ScopedTask&& moved) : outer_(moved.outer),
                                     task_(std::move(moved.task_)) {
      moved.outer_ = nullptr;
    }

    ~ScopedTask() {
      if (outer_) {
        // TODO: Decrease |outer_|'s accordingly state post execution.
      }
    }

    const Task* task() { return task.get(); }

   private:
    ShutdownManager* outer_;

    scoped_ptr<Task> task_;

    DISALLOW_COPY_AND_ASSIGN(ScopedTask);
  };

  (...)
};


Once again, looking for your two's input of these 3 options.

Cheers!

[robliao, 2016/02/23 22:47:02]

What I like about the ShutdownManager right now is that it doesn't have to hold
on to tasks, so we should definitely keep that as a goal. All options currently
do that.

I also agree that Should* series of calls should have const marked on the
method. These should not have an effect on the ShutdownManager.

So now it comes down to how we track the executions. 

Option 3 means that the scheduler infrastructure would need to hold on to a
ScopedTask factoried from the ShutdownManager. While this gets the job done, the
relationship of who controls the tasks seems a little different in that the
ShutdownManager always has to be involved in all tasks.

How about Option 4:

The ShutdownManager answers questions about tasks, captured by
    bool ShouldPostTask(Task) const;
    bool ShouldScheduleTask(Task) const;

The ShutdownManger also implements a Scheduler observer (whatever name makes
sense)
    void OnTaskPosted(Task);
    void OnTaskScheduled(Task);

We can either have one listener for now or have a set if we decide other
components need to know about when tasks are posted or scheduled (or more).

[fdoray, 2016/02/24 17:01:06]

OPTION 1: 
Pro: ShutdownManager doesn't have to know about Task (only
TaskShutdownBehavior).
Con: Hard to guarantee that RequestPostTask/RequestScheduleTask aren't called
multiple times for the same Task.

OPTION 2:
Pro: Guarantees that RequestPostTask is called only once per Task.
Con: It's weird that PostTask is done by ShutdownManager (even if it's via a
callback). We will have to release the ShutdownManager lock before invoking the
callback.

OPTION 3: The code to run a task would look like that:

{
  auto scoped_task = shutdown_manager->RequestExecuteTask(std::move(task));
  if (scoped_task.get())
    debug::TaskAnnotator().RunTask("", *scoped_task.get());
}

I think that ScopedTask introduces complexity for no reason. IMO, option 5 is
simpler to understand.

OPTION 4: Checking if a task can be posted/scheduled and incrementing the number
of tasks blocking shutdown has to be done atomically. With option 4, this
problematic scenario can happen:
. Thread 1: Checks if it can schedule a SKIP_ON_SHUTDOWN task.
ShouldScheduleTask returns true because shutdown hasn't started.
. Thread 2: Calls Shutdown(), which returns immediately because there is no
tasks blocking shutdown.
. Thread 1: Starts running its SKIP_ON_SHUTDOWN task :( If the number of tasks
blocking shutdown had been incremented BY ShouldScheduleTask, thread 2 wouldn't
have returned from Shutdown() before the SKIP_ON_SHUTDOWN task had completed its
execution.

**OPTION 5**:

class ShutdownManager {
 public:
  // Forwards |task| to |post_task_continuation| unless the current shutdown
  // state prevents that in which case |task| is dropped.
  void PostTask(const base::Callback<scoped_ptr<Task>> post_task_continuation,
                scoped_ptr<Task> task);

  // Runs |task| unless the current shutdown state prevents that.
  void RunTask(scoped_ptr<Task> task);
};

The code to run a task would look like that (simpler than option 3):
shutdown_manager->RunTask(std::move(task));

+
+  // Indicates whether a task with this |shutdown_behavior| should be scheduled.
+  bool ShouldScheduleTask(TaskShutdownBehavior shutdown_behavior);
+
+  // Notifies the shutdown manager that a task with this |shutdown_behavior|
+  // completed its execution.
+  void DidExecuteTask(TaskShutdownBehavior shutdown_behavior);
+
+  void set_is_shutting_down_for_testing(bool is_shutting_down_for_testing);
+  bool is_shutting_down_for_testing() const;
+
+  bool shutdown_completed() const;

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

Inline impl of lower_case_getters_and_setters() above.

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

Done.

+
+ private:
+  // Lock protecting all members.
+  SchedulerLock lock_;
+
+  // Condition variable signaled when |num_tasks_blocking_shutdown_| reaches
+  // zero.

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

[gab TODO]

Append "while |is_shutting_down_|"?

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

Done.

+  scoped_ptr<ConditionVariable> cv_;
+
+  // Number of tasks blocking shutdown.
+  size_t num_tasks_blocking_shutdown_;
+
+  // True when Shutdown() is waiting for tasks blocking shutdown to complete
+  // their execution.
+  bool is_shutting_down_;
+
+  // True once Shutdown() has returned. No new task can be scheduled after this.
+  bool shutdown_completed_;
+
+  DISALLOW_COPY_AND_ASSIGN(ShutdownManager);
+};
+
+}  // namespace internal
+}  // namespace base
+
+#endif  // BASE_TASK_SCHEDULER_SHUTDOWN_MANAGER_H_