Run PrioritizedTaskRunner tasks with the same priority in posting order

What:
If two tasks have the same priority, they should run in posting order.

A second thing this CL does is fix a race in the unittests. I meant to block
the task runner before queueing up requests but failed to. Fixed here.

Why:
This can prevent hypothetical task starvation. It's also necessary for the
SimpleCachePrioritization control group to ensure that we maintain old
behavior.

Bug: 850085
Change-Id: I1f259296604bf04ea33ecbee9ad3180f064c4266
Reviewed-on: https://chromium-review.googlesource.com/1088759
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564933}

net/base/prioritized_task_runner.cc

@@ -15,11 +15,13 @@ namespace net {
 PrioritizedTaskRunner::Job::Job(const base::Location& from_here,
                                 base::OnceClosure task,
                                 base::OnceClosure reply,
-                                uint32_t priority)
+                                uint32_t priority,
+                                uint32_t task_count)
     : from_here(from_here),
       task(std::move(task)),
       reply(std::move(reply)),
-      priority(priority) {}
+      priority(priority),
+      task_count(task_count) {}
 
 PrioritizedTaskRunner::Job::~Job() = default;
 PrioritizedTaskRunner::Job::Job(Job&& other) = default;
@@ -34,7 +36,8 @@ void PrioritizedTaskRunner::PostTaskAndReply(const base::Location& from_here,
                                              base::OnceClosure task,
                                              base::OnceClosure reply,
                                              uint32_t priority) {
-  Job job(from_here, std::move(task), std::move(reply), priority);
+  Job job(from_here, std::move(task), std::move(reply), priority,
+          task_count_++);
   {
     base::AutoLock lock(job_heap_lock_);
     job_heap_.push_back(std::move(job));

net/base/prioritized_task_runner.h

@@ -48,7 +48,7 @@ class NET_EXPORT_PRIVATE PrioritizedTaskRunner
   // Similar to TaskRunner::PostTaskAndReply, except that the task runs at
   // |priority|. Priority 0 is the highest priority and will run before other
   // priority values. Multiple tasks with the same |priority| value are run in
-  // arbitrary order.
+  // order of posting.
   void PostTaskAndReply(const base::Location& from_here,
                         base::OnceClosure task,
                         base::OnceClosure reply,
@@ -80,7 +80,8 @@ class NET_EXPORT_PRIVATE PrioritizedTaskRunner
     Job(const base::Location& from_here,
         base::OnceClosure task,
         base::OnceClosure reply,
-        uint32_t priority);
+        uint32_t priority,
+        uint32_t task_count);
     ~Job();
 
     Job(Job&& other);
@@ -90,6 +91,7 @@ class NET_EXPORT_PRIVATE PrioritizedTaskRunner
     base::OnceClosure task;
     base::OnceClosure reply;
     uint32_t priority;
+    uint32_t task_count;
     scoped_refptr<base::TaskRunner> reply_task_runner =
         base::ThreadTaskRunnerHandle::Get();
 
@@ -99,6 +101,8 @@ class NET_EXPORT_PRIVATE PrioritizedTaskRunner
 
   struct JobComparer {
     bool operator()(const Job& left, const Job& right) {
+      if (left.priority == right.priority)
+        return left.task_count > right.task_count;
       return left.priority > right.priority;
     }
   };
@@ -120,6 +124,11 @@ class NET_EXPORT_PRIVATE PrioritizedTaskRunner
   // Accessed on Job::reply_task_runner.
   scoped_refptr<base::TaskRunner> task_runner_;
 
+  // Used to preserve order of jobs of equal priority. This can overflow and
+  // cause periodic priority inversion. This should be infrequent enough to be
+  // of negligible impact.
+  uint32_t task_count_;
+
   DISALLOW_COPY_AND_ASSIGN(PrioritizedTaskRunner);
 };
 

net/base/prioritized_task_runner_unittest.cc

@@ -4,18 +4,22 @@
 
 #include "net/base/prioritized_task_runner.h"
 
+#include <algorithm>
 #include <limits>
 #include <vector>
 
 #include "base/bind_helpers.h"
 #include "base/location.h"
+#include "base/rand_util.h"
 #include "base/run_loop.h"
 #include "base/sequenced_task_runner.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/synchronization/lock.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/test/scoped_task_environment.h"
-
+#include "base/threading/thread_restrictions.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -55,11 +59,25 @@ class PrioritizedTaskRunnerTest : public testing::Test {
     return out;
   }
 
+  void BlockTaskRunner(base::TaskRunner* task_runner) {
+    waitable_event_.Reset();
+
+    auto wait_function = [](base::WaitableEvent* waitable_event) {
+      base::ScopedAllowBaseSyncPrimitivesForTesting sync;
+      waitable_event->Wait();
+    };
+    task_runner->PostTask(FROM_HERE,
+                          base::BindOnce(wait_function, &waitable_event_));
+  }
+
+  void ReleaseTaskRunner() { waitable_event_.Signal(); }
+
  protected:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
 
   std::vector<std::string> callback_names_;
   base::Lock callback_names_lock_;
+  base::WaitableEvent waitable_event_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(PrioritizedTaskRunnerTest);
@@ -115,6 +133,7 @@ TEST_F(PrioritizedTaskRunnerTest, PostTaskAndReplyTestPriority) {
   auto prioritized_task_runner =
       base::MakeRefCounted<PrioritizedTaskRunner>(task_runner);
 
+  BlockTaskRunner(task_runner.get());
   prioritized_task_runner->PostTaskAndReply(
       FROM_HERE,
       base::BindOnce(&PrioritizedTaskRunnerTest::PushName,
@@ -138,6 +157,7 @@ TEST_F(PrioritizedTaskRunnerTest, PostTaskAndReplyTestPriority) {
       base::BindOnce(&PrioritizedTaskRunnerTest::PushName,
                      base::Unretained(this), "Reply7"),
       7);
+  ReleaseTaskRunner();
 
   // Run the TaskRunner and all of the tasks and replies should have run, in
   // priority order.
@@ -155,6 +175,7 @@ TEST_F(PrioritizedTaskRunnerTest, PriorityOverflow) {
 
   const uint32_t kMaxPriority = std::numeric_limits<uint32_t>::max();
 
+  BlockTaskRunner(task_runner.get());
   prioritized_task_runner->PostTaskAndReply(
       FROM_HERE,
       base::BindOnce(&PrioritizedTaskRunnerTest::PushName,
@@ -178,6 +199,7 @@ TEST_F(PrioritizedTaskRunnerTest, PriorityOverflow) {
       base::BindOnce(&PrioritizedTaskRunnerTest::PushName,
                      base::Unretained(this), "ReplyMaxPlus1"),
       kMaxPriority + 1);
+  ReleaseTaskRunner();
 
   // Run the TaskRunner and all of the tasks and replies should have run, in
   // priority order.
@@ -214,6 +236,7 @@ TEST_F(PrioritizedTaskRunnerTest, PostTaskAndReplyWithResultTestPriority) {
   auto prioritized_task_runner =
       base::MakeRefCounted<PrioritizedTaskRunner>(task_runner);
 
+  BlockTaskRunner(task_runner.get());
   prioritized_task_runner->PostTaskAndReplyWithResult(
       FROM_HERE,
       base::BindOnce(&PrioritizedTaskRunnerTest::PushNameWithResult,
@@ -237,6 +260,7 @@ TEST_F(PrioritizedTaskRunnerTest, PostTaskAndReplyWithResultTestPriority) {
       base::BindOnce(&PrioritizedTaskRunnerTest::PushName,
                      base::Unretained(this)),
       3);
+  ReleaseTaskRunner();
 
   // Run the TaskRunner and both the Task and Reply should run.
   scoped_task_environment_.RunUntilIdle();
@@ -245,5 +269,45 @@ TEST_F(PrioritizedTaskRunnerTest, PostTaskAndReplyWithResultTestPriority) {
             ReplyOrder());
 }
 
+TEST_F(PrioritizedTaskRunnerTest, OrderSamePriorityByPostOrder) {
+  auto task_runner =
+      base::CreateSequencedTaskRunnerWithTraits(base::TaskTraits());
+  auto prioritized_task_runner =
+      base::MakeRefCounted<PrioritizedTaskRunner>(task_runner);
+
+  std::vector<int> expected;
+
+  // Create 1000 tasks with random priorities between 1 and 3. Those that have
+  // the same priorities should run in posting order.
+  BlockTaskRunner(task_runner.get());
+  for (int i = 0; i < 1000; i++) {
+    int priority = base::RandInt(0, 2);
+    int id = (priority * 1000) + i;
+
+    expected.push_back(id);
+    prioritized_task_runner->PostTaskAndReply(
+        FROM_HERE,
+        base::BindOnce(&PrioritizedTaskRunnerTest::PushName,
+                       base::Unretained(this), base::IntToString(id)),
+        base::BindOnce(base::DoNothing::Once()), priority);
+  }
+  ReleaseTaskRunner();
+
+  // This is the order the tasks should run on the queue.
+  std::sort(expected.begin(), expected.end());
+
+  scoped_task_environment_.RunUntilIdle();
+
+  // This is the order that the tasks ran on the queue.
+  std::vector<int> results;
+  for (const std::string& result : callback_names_) {
+    int result_id;
+    EXPECT_TRUE(base::StringToInt(result, &result_id));
+    results.push_back(result_id);
+  }
+
+  EXPECT_EQ(expected, results);
+}
+
 }  // namespace
 }  // namespace net