Add full SetThreadPriority support to Windows.

base/threading/platform_thread_unittest.cc

Index: base/threading/platform_thread_unittest.cc
diff --git a/base/threading/platform_thread_unittest.cc b/base/threading/platform_thread_unittest.cc
index 21260e5ec36bd27dcdc9bf1500167e32fbab2524..42e49fb5a1236e12aa21115ac02f855914b56717 100644
--- a/base/threading/platform_thread_unittest.cc
+++ b/base/threading/platform_thread_unittest.cc
@@ -3,10 +3,15 @@
 // found in the LICENSE file.
 
 #include "base/compiler_specific.h"
+#include "base/macros.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
-
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if defined(OS_WIN)
+#include <windows.h>
+#endif
+
 namespace base {
 
 // Trivial tests that thread runs and doesn't crash on create and join ---------
@@ -53,10 +58,22 @@ TEST(PlatformThreadTest, TrivialTimesTen) {
 
 class FunctionTestThread : public TrivialThread {
  public:
-  FunctionTestThread() : thread_id_(0) {}
+  FunctionTestThread()
+      : thread_id_(kInvalidThreadId),
+        thread_started_(true, false),
+        terminate_thread_(true, false) {}
+  ~FunctionTestThread() override {
+    EXPECT_TRUE(terminate_thread_.IsSignaled())
+        << "Need to mark thread for termination and join the underlying thread "
+        << "before destroying a FunctionTestThread as it owns the "
+        << "WaitableEvent blocking the underlying thread's main.";
+  }
 
+  // Grabs |thread_id_|, signals |thread_started_|, and then waits for
+  // |terminate_thread_| to be signaled before exiting.
   void ThreadMain() override {
     thread_id_ = PlatformThread::CurrentId();
+    EXPECT_NE(thread_id_, kInvalidThreadId);
     PlatformThread::YieldCurrentThread();
     PlatformThread::Sleep(TimeDelta::FromMilliseconds(50));
 
@@ -64,13 +81,30 @@ class FunctionTestThread : public TrivialThread {
     EXPECT_EQ(thread_id_, PlatformThread::CurrentId());
 
     TrivialThread::ThreadMain();
+
+    thread_started_.Signal();

[rvargas (doing something else), 2015/03/31 19:27:10]

Shouldn't this be set between 76 and 77? (do we still need yield and sleep?)

[gab, 2015/03/31 20:18:20]

I guess the yield and sleep are no longer needed. The only value was to make
sure that we don't somehow change thread IDs after we do that, but
PlatformThreadTest.Function already verifies that for the main thread.

Removed.

Otherwise, it does belong on this line as we need to call
TrivialThread::ThreadMain() first to set did_run().

[rvargas (doing something else), 2015/03/31 22:43:45]

Thanks. But then I believe the name is misleading because this is not about the
thread having started but about the thread having finished whatever it was
supposed to do (set did_run_?). We should name things accordingly.

And it's a nice improvement btw.

[gab, 2015/04/01 01:14:39]

I see, agreed, actually it's the dependency on TrivialThread that doesn't make
much sense here, removed and replaced by a more meaningful IsRunning() method.

I now actually think that the tests based on TrivialThread actually test a
strict subset of what FunctionTestThread-based tests do, shall we get rid of
them?

[rvargas (doing something else), 2015/04/01 02:04:55]

Given that one set has explicit synchronization and the other doesn't, it seems
better to keep them.

+
+    terminate_thread_.Wait();
+  }
+
+  PlatformThreadId thread_id() const {
+    EXPECT_TRUE(thread_started_.IsSignaled()) << "Thread ID still unknown";
+    return thread_id_;
   }
 
-  PlatformThreadId thread_id() const { return thread_id_; }
+  // Blocks until this thread is started.
+  void WaitForThreadStart() { thread_started_.Wait(); }
+
+  // Mark this thread for termination (callers must then join this thread to be
+  // guaranteed of termination).
+  void MarkForTermination() { terminate_thread_.Signal(); }
 
  private:
   PlatformThreadId thread_id_;
 
+  mutable WaitableEvent thread_started_;
+  WaitableEvent terminate_thread_;
+
   DISALLOW_COPY_AND_ASSIGN(FunctionTestThread);
 };
 
@@ -82,10 +116,13 @@ TEST(PlatformThreadTest, Function) {
 
   ASSERT_FALSE(thread.did_run());
   ASSERT_TRUE(PlatformThread::Create(0, &thread, &handle));
-  PlatformThread::Join(handle);
+  thread.WaitForThreadStart();
   ASSERT_TRUE(thread.did_run());
   EXPECT_NE(thread.thread_id(), main_thread_id);
 
+  thread.MarkForTermination();
+  PlatformThread::Join(handle);
+
   // Make sure that the thread ID is the same across calls.
   EXPECT_EQ(main_thread_id, PlatformThread::CurrentId());
 }
@@ -98,10 +135,12 @@ TEST(PlatformThreadTest, FunctionTimesTen) {
 
   for (size_t n = 0; n < arraysize(thread); n++)
     ASSERT_FALSE(thread[n].did_run());
+
   for (size_t n = 0; n < arraysize(thread); n++)
     ASSERT_TRUE(PlatformThread::Create(0, &thread[n], &handle[n]));
   for (size_t n = 0; n < arraysize(thread); n++)
-    PlatformThread::Join(handle[n]);
+    thread[n].WaitForThreadStart();
+
   for (size_t n = 0; n < arraysize(thread); n++) {
     ASSERT_TRUE(thread[n].did_run());
     EXPECT_NE(thread[n].thread_id(), main_thread_id);
@@ -112,8 +151,116 @@ TEST(PlatformThreadTest, FunctionTimesTen) {
     }
   }
 
+  for (size_t n = 0; n < arraysize(thread); n++)
+    thread[n].MarkForTermination();
+  for (size_t n = 0; n < arraysize(thread); n++)
+    PlatformThread::Join(handle[n]);
+
   // Make sure that the thread ID is the same across calls.
   EXPECT_EQ(main_thread_id, PlatformThread::CurrentId());
 }
 
+namespace {
+
+const ThreadPriority kThreadPriorityTestValues[] = {
+// Disable non-normal priority toggling on POSIX as it appears to be broken
+// (http://crbug.com/468793). This is prefered to disabling the tests altogether
+// on POSIX as it at least provides coverage for running this code under
+// "normal" priority.
+#if !defined(OS_POSIX)
+    kThreadPriority_RealtimeAudio,
+    kThreadPriority_Display,
+    kThreadPriority_Background,
+#endif  // !defined(OS_POSIX)
+    // Keep normal last to test unbackgrounding.
+    kThreadPriority_Normal
+};
+
+}  // namespace
+
+// Test changing another thread's priority.
+// NOTE: This test is partially disabled on POSIX, see note above and
+// http://crbug.com/468793.
+TEST(PlatformThreadTest, ThreadPriorityOtherThread) {
+  PlatformThreadHandle current_handle(PlatformThread::CurrentHandle());
+
+  // Confirm that the current thread's priority is as expected.
+  EXPECT_EQ(kThreadPriority_Normal,
+            PlatformThread::GetThreadPriority(current_handle));
+
+  // Create a test thread.
+  FunctionTestThread thread;
+  PlatformThreadHandle handle;
+  ASSERT_TRUE(PlatformThread::Create(0, &thread, &handle));
+  thread.WaitForThreadStart();
+  EXPECT_NE(thread.thread_id(), kInvalidThreadId);
+  EXPECT_NE(thread.thread_id(), PlatformThread::CurrentId());
+
+  // New threads should get normal priority by default.
+  EXPECT_EQ(kThreadPriority_Normal, PlatformThread::GetThreadPriority(handle));
+
+  // Toggle each supported priority on the test thread and confirm it only
+  // affects it (and not the current thread).
+  for (size_t i = 0; i < arraysize(kThreadPriorityTestValues); ++i) {
+    SCOPED_TRACE(i);
+
+    // Alter and verify the test thread's priority.
+    PlatformThread::SetThreadPriority(handle, kThreadPriorityTestValues[i]);
+    EXPECT_EQ(kThreadPriorityTestValues[i],
+              PlatformThread::GetThreadPriority(handle));
+
+    // Make sure the current thread was otherwise unaffected.
+    EXPECT_EQ(kThreadPriority_Normal,
+              PlatformThread::GetThreadPriority(current_handle));
+  }
+
+  thread.MarkForTermination();
+  PlatformThread::Join(handle);
+}
+
+// Test changing the current thread's priority (which has different semantics on
+// some platforms).
+// NOTE: This test is partially disabled on POSIX, see note above and
+// http://crbug.com/468793.
+TEST(PlatformThreadTest, ThreadPriorityCurrentThread) {
+  PlatformThreadHandle current_handle(PlatformThread::CurrentHandle());
+
+  // Confirm that the current thread's priority is as expected.
+  EXPECT_EQ(kThreadPriority_Normal,
+            PlatformThread::GetThreadPriority(current_handle));
+
+  // Create a test thread for verification purposes only.
+  FunctionTestThread thread;
+  PlatformThreadHandle handle;
+  ASSERT_TRUE(PlatformThread::Create(0, &thread, &handle));
+  thread.WaitForThreadStart();
+  EXPECT_NE(thread.thread_id(), kInvalidThreadId);
+  EXPECT_NE(thread.thread_id(), PlatformThread::CurrentId());
+
+  // Confirm that the new thread's priority is as expected.
+  EXPECT_EQ(kThreadPriority_Normal, PlatformThread::GetThreadPriority(handle));
+
+  // Toggle each supported priority on the current thread and confirm it only
+  // affects it (and not the test thread).
+  for (size_t i = 0; i < arraysize(kThreadPriorityTestValues); ++i) {
+    SCOPED_TRACE(i);
+
+    // Alter and verify the current thread's priority.
+    PlatformThread::SetThreadPriority(current_handle,
+                                      kThreadPriorityTestValues[i]);
+    EXPECT_EQ(kThreadPriorityTestValues[i],
+              PlatformThread::GetThreadPriority(current_handle));
+
+    // Make sure the test thread was otherwise unaffected.
+    EXPECT_EQ(kThreadPriority_Normal,
+              PlatformThread::GetThreadPriority(handle));
+  }
+
+  // Restore current thread priority for follow-up tests.
+  PlatformThread::SetThreadPriority(current_handle, kThreadPriority_Normal);
+
+  thread.MarkForTermination();
+  PlatformThread::Join(handle);
+}
+
 }  // namespace base