Support FileDescriptorWatcher in TaskScheduler.

base/task_scheduler/task_tracker.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_TASK_TRACKER_H_
 #define BASE_TASK_SCHEDULER_TASK_TRACKER_H_
 
 #include <memory>
 
 #include "base/atomicops.h"
 #include "base/base_export.h"
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_base.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/task_scheduler/scheduler_lock.h"
 #include "base/task_scheduler/sequence.h"
 #include "base/task_scheduler/task.h"
 #include "base/task_scheduler/task_traits.h"
+#include "build/build_config.h"
 
 namespace base {
 
 class ConditionVariable;
+class MessageLoopForIO;
 class SequenceToken;
 
 namespace internal {
 
 // All tasks go through the scheduler's TaskTracker when they are posted and
-// when they are executed. The TaskTracker enforces shutdown semantics and takes
-// care of tracing and profiling. This class is thread-safe.
+// when they are executed. The TaskTracker sets up the environment to run a
+// task, enforces shutdown semantics and takes care of tracing and profiling.
+// This class is thread-safe.
 class BASE_EXPORT TaskTracker {
  public:
-  TaskTracker();
+  // |watch_file_descriptor_message_loop| is used to initialize a
+  // FileDescriptorWatcher in the scope in which a Task runs.
+  TaskTracker(
+#if defined(OS_POSIX) && !defined(OS_NACL_SFI)
+      MessageLoopForIO* watch_file_descriptor_message_loop

[gab, 2016/10/18 14:40:43]

It feels a bit intrusive to add this directly to TaskTracker. To me TaskTracker
is a no-op place used for instrumentation/metrics/tracking/etc, not logic.

What do you think of adding TaskTracker::TaskObserver instead which can be
registered to be called before/after a task runs (with its TaskTraits say)? This
isn't scary like your typical observer IMO because you still need an instance of
TaskTracker which means the task_scheduler code still controls who gets to be
able to register observers.

It also gets rid of all if-defs (which are also a sign to me that it doesn't
belong here) but the single callsite that will register.

And it probably makes tests cleaner as well by not coupling the need for a
|watch_file_descriptor_message_loop_| in production with the TaskTracker logic
(the need to adapt every test is another sign IMO that this is the wrong place).

[fdoray, 2016/10/18 15:30:43]

We already register TaskRunner handles from TaskTracker::RunTask. I think it's a
good thing to put all the work to set up the environment to run a task in
TaskTracker::RunTask because that method could easily be reused from a
SchedulerWorkerPool impl backed by the Windows thread pool.
The environment set up code can still be reused if it's moved to a TaskObserver
invoked by TaskTracker::RunTask. However, we loose the ability to instantiate
objects on the stack for the scope of the task.

i.e.
FileDescriptorWatcher file_descriptor_watcher(message_loop);
becomes
void BeforeRunTask() {
  // Heap allocation :(
  file_descriptor_watcher_ = MakeUnique<FileDescriptorWatcher>();
}
void AfterRunTask() {
  // Extra line that we could forget to add :(
  file_descriptor_watcher_.reset();
}

Instead of having a TaskObserver, WDYT of a TaskTracker::Delegate:

class TaskTracker::Delegate {
 public:
  // Must run the callback in |task|. May perform additional
  // work before and after running the callback.
  void RunTask(std::unique_ptr<Task> task) = 0;
};

Keeping track of the number of tasks blocking shutdown is handled by
TaskTracker. The delegate handles setting up the environment to run the task and
tracing [1].
I agree.

[1] Since tracing and running the task is done by TaskAnnotator::RunTask, it's
would be hard to keep tracing in TaskTracker.

+#endif
+      );
   ~TaskTracker();
 
   // Synchronously shuts down the scheduler. Once this is called, only tasks
   // posted with the BLOCK_SHUTDOWN behavior will be run. Returns when:
   // - All SKIP_ON_SHUTDOWN tasks that were already running have completed their
   //   execution.
   // - All posted BLOCK_SHUTDOWN tasks have completed their execution.
   // CONTINUE_ON_SHUTDOWN tasks still may be running after Shutdown returns.
   // This can only be called once.
   void Shutdown();
@@ skipping 80 matching lines @@
   // Synchronizes access to shutdown related members below.
   mutable SchedulerLock shutdown_lock_;
 
   // Event instantiated when shutdown starts and signaled when shutdown
   // completes.
   std::unique_ptr<WaitableEvent> shutdown_event_;
 
   // Number of BLOCK_SHUTDOWN tasks posted during shutdown.
   HistogramBase::Sample num_block_shutdown_tasks_posted_during_shutdown_ = 0;
 
+#if defined(OS_POSIX) && !defined(OS_NACL_SFI)
+  MessageLoopForIO* const watch_file_descriptor_message_loop_;
+#endif
+
   DISALLOW_COPY_AND_ASSIGN(TaskTracker);
 };
 
 }  // namespace internal
 }  // namespace base
 
 #endif  // BASE_TASK_SCHEDULER_TASK_TRACKER_H_