TaskScheduler: No BACKGROUND threads when unsupported.

base/task_scheduler/scheduler_worker.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/scheduler_worker.h"
 
 #include <stddef.h>
 
 #include <utility>
 
@@ skipping 31 matching lines @@
     WaitForWork();
 
     while (!outer_->task_tracker_->IsShutdownComplete() &&
            !outer_->ShouldExitForTesting()) {
       DCHECK(outer_);
 
 #if defined(OS_MACOSX)
       mac::ScopedNSAutoreleasePool autorelease_pool;
 #endif
 
-#if !defined(OS_LINUX)
       UpdateThreadPriority(GetDesiredThreadPriority());
-#endif
 
       // Get the sequence containing the next task to execute.
       scoped_refptr<Sequence> sequence = outer_->delegate_->GetWork(outer_);
       if (!sequence) {
         if (outer_->delegate_->CanDetach(outer_)) {
           detached_thread = outer_->Detach();
           if (detached_thread) {
             DCHECK_EQ(detached_thread.get(), this);
             PlatformThread::Detach(thread_handle_);
             outer_ = nullptr;
@@ skipping 59 matching lines @@
     if (sleep_time.is_max()) {
       // Calling TimedWait with TimeDelta::Max is not recommended per
       // http://crbug.com/465948.
       wake_up_event_.Wait();
     } else {
       wake_up_event_.TimedWait(sleep_time);
     }
     wake_up_event_.Reset();
   }
 
-  // Returns the desired thread priority based on the worker priority and the
-  // current shutdown state.
+  // Returns the priority to which the thread should be set based on the
+  // SchedulerWorker's preferred priority, current shutdown state and platform

[gab, 2016/08/03 13:35:24]

oxford comma FTW (", and") :-)

[fdoray, 2016/08/03 18:42:48]

Done.

+  // capabilities.
   ThreadPriority GetDesiredThreadPriority() {
     DCHECK(outer_);
 
-    if (outer_->task_tracker_->HasShutdownStarted() &&
+    const bool worker_priority_below_normal =
         static_cast<int>(outer_->thread_priority_) <
-            static_cast<int>(ThreadPriority::NORMAL)) {
+        static_cast<int>(ThreadPriority::NORMAL);
+
+    if (worker_priority_below_normal &&
+        outer_->task_tracker_->HasShutdownStarted()) {
       return ThreadPriority::NORMAL;
     }
+
+#if defined(OS_POSIX)

[gab, 2016/08/03 13:35:24]

Add a comment here why it's fine on Windows? This isn't captured in lock.h or
anywhere else AFAIK. Ideally this would be mentioned at a higher level than
here... I'd prefer for the call made here to not be OS_POSIX, maybe some kind of
Lock::IsThreadPrioritySafe()? which returns true on Windows (with reasoning) and
returns LockImpl::PriorityInheritanceAvailable() on POSIX)?

[fdoray, 2016/08/03 18:42:49]

n/a now that we have HandlesMultipleThreadPriorities()

+    // To avoid priority inversion, all threads have a NORMAL priority when
+    // priority inheritance isn't available.
+    if (!Lock::PriorityInheritanceAvailable())
+      return ThreadPriority::NORMAL;
+#endif
+
+    // To avoid shutdown hangs caused by BLOCK_SHUTDOWN tasks running on
+    // BACKGROUND threads, there are no BACKGROUND threads when thread priority
+    // can't be increased.
+    if (worker_priority_below_normal &&

[gab, 2016/08/03 13:35:24]

So "worker priority" is just the priority hint right? Not the actual priority? I
guess I find this statement confusing because of that.. renaming it to
|priority_hint_| as suggested above will make that clearer IMO (and this bool to
something like |priority_hint_below_normal|).

[fdoray, 2016/08/03 18:42:48]

Done

+        !PlatformThread::CanIncreaseCurrentThreadPriority()) {
+      return ThreadPriority::NORMAL;
+    }
+
     return outer_->thread_priority_;

[gab, 2016/08/03 13:35:24]

So basically the point of |worker_priority_below_normal| is to avoid returning
NORMAL when the hint is > NORMAL< right?

Would it be cleaner to instead start with

if (static_cast<int>(outer_->priority_hint_) >=
    static_cast<int>(ThreadPriority::NORMAL)) {
  return outer_->priority_hint_;
}

and then doing the rest of the logic assuming that the hint is < NORMAL?

[fdoray, 2016/08/03 18:42:49]

|worker_priority_below_normal| is gone

   }
 
-  // Increasing the thread priority requires the CAP_SYS_NICE capability on
-  // Linux.
-#if !defined(OS_LINUX)
   void UpdateThreadPriority(ThreadPriority desired_thread_priority) {
     if (desired_thread_priority == current_thread_priority_)
       return;
 
     PlatformThread::SetCurrentThreadPriority(desired_thread_priority);
     current_thread_priority_ = desired_thread_priority;
   }
-#endif  // !defined(OS_LINUX)
 
   PlatformThreadHandle thread_handle_;
 
   SchedulerWorker* outer_;
 
   // Event signaled to wake up this thread.
   WaitableEvent wake_up_event_;
 
   // Current priority of this thread. May be different from
-  // |outer_->thread_priority_| during shutdown.
+  // |outer_->thread_priority_|.
   ThreadPriority current_thread_priority_;
 
   DISALLOW_COPY_AND_ASSIGN(Thread);
 };
 
 std::unique_ptr<SchedulerWorker> SchedulerWorker::Create(
     ThreadPriority thread_priority,
     std::unique_ptr<Delegate> delegate,
     TaskTracker* task_tracker,
     InitialState initial_state) {
@@ skipping 78 matching lines @@
   CreateThread();
 }
 
 bool SchedulerWorker::ShouldExitForTesting() const {
   AutoSchedulerLock auto_lock(should_exit_for_testing_lock_);
   return should_exit_for_testing_;
 }
 
 }  // namespace internal
 }  // namespace base