TaskScheduler [2/9] Scheduler Lock

base/task_scheduler/scheduler_lock_unittest.cc

Index: base/task_scheduler/scheduler_lock_unittest.cc
diff --git a/base/task_scheduler/scheduler_lock_unittest.cc b/base/task_scheduler/scheduler_lock_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..48b8b087a33e52fa76e8743ef3afd2e916628218
--- /dev/null
+++ b/base/task_scheduler/scheduler_lock_unittest.cc
@@ -0,0 +1,299 @@
+// 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_lock.h"
+
+#include <stdlib.h>
+
+#include "base/compiler_specific.h"
+#include "base/macros.h"
+#include "base/rand_util.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/threading/platform_thread.h"
+#include "base/threading/simple_thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+namespace internal {
+namespace {
+
+// Death tests misbehave on Android.
+#if DCHECK_IS_ON() && defined(GTEST_HAS_DEATH_TEST) && !defined(OS_ANDROID)
+#define EXPECT_DCHECK_DEATH(statement, regex) EXPECT_DEATH(statement, regex)
+#else
+#define EXPECT_DCHECK_DEATH(statement, regex)

[gab, 2016/03/14 18:26:04]

Shouldn't this be 
#define EXPECT_DCHECK_DEATH(statement, regex) statement
?

To at least run the statement in non-dcheck scenarios? Otherwise any statement
that comes after the death test and depends on its state would fail?

Also, if we do this then we can put only the specific statement that is expected
to generate the DCHECK in the EXPECT_DCHECK_DEATH statement and improve
readability + making sure we test what we expect?

[fdoray, 2016/03/14 18:55:52]

Note that on Posix, the |statement| of EXPECT_DCHECK_DEATH  runs in a new
process. That's why we sometimes have multiple statements inside the macro.

Consider this:

1. lock.Acquire();
2. EXPECT_DCHECK_DEATH({
3.   predecessor.Acquire();
4. }, "");

On Windows, lines 1 and 3 run on the same thread -> line 3 DCHECKs.
On Posix, lines 1 and 3 run on different threads -> no DCHECK.

We must write the test like this to ensure that |predecessor.Acquire()| DCHECKs
on every platform:

EXPECT_DCHECK_DEATH({
  lock.Acquire();
  predecessor.Acquire();
}, "");

[robliao, 2016/03/14 19:09:52]

The way we've structured the tests, we expect death as the last statement. Given
that the last statement will fail, there shouldn't be statements after that
point because now you're in a strange state. 

Other tests simply don't run these tests when DCHECK_IS_ON(), and we've followed
the spirit of that with EXPECT_DCHECK_DEATH.

+1 to fdoray's comment. The first CL's actually had the statement that caused
problems alone in EXPECT_DEATH, but that caused problems with POSIX since POSIX
runs a fork from that point. Windows re-executes the entire test.

[gab, 2016/03/14 19:51:13]

Ok, re-lgtm.

PS: Would be nice to confirm that try bots indeed cover DCHECKs. Maybe
intentionally break something in this CL in a patch set to verify the tests
react as intended?

[robliao, 2016/03/14 19:55:41]

From the tests so far, the mobile ones at least do :-). The android ones were
very flaky, so I had to disable them.

For the other bots, we can see dcheck_always_on=1 (win_chromium_x64_rel_ng), so
we should have good confidence that the DCHECK tests are being run.

https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...

[gab, 2016/03/14 20:02:52]

I seem to recall a surprising episode on chromium-dev where people thought
DCHECKs were on but they weren't -- not sure if it was fixed. Feels to me it's
worth an extra hours to do a try with an unintentional bug in this CL just to
confirm for realz :-).

[robliao, 2016/03/14 20:25:07]

Sounds good. Feel free to monitor
https://codereview.chromium.org/1802003002/

For the fireworks :-).

[robliao, 2016/03/14 20:37:13]

And the fireworks have started (tests going red), so we should be good to go
here.

+#endif
+
+// Adapted from base::Lock's BasicLockTestThread to make sure
+// Acquire()/Release() don't crash.
+class BasicLockTestThread : public SimpleThread {
+ public:
+  explicit BasicLockTestThread(SchedulerLock* lock)
+      : SimpleThread("BasicLockTestThread"),
+        lock_(lock),
+        acquired_(0) {}
+
+  int acquired() const { return acquired_; }
+
+ private:
+  void Run() override {
+    for (int i = 0; i < 10; i++) {
+      lock_->Acquire();
+      acquired_++;
+      lock_->Release();
+    }
+    for (int i = 0; i < 10; i++) {
+      lock_->Acquire();
+      acquired_++;
+      PlatformThread::Sleep(TimeDelta::FromMilliseconds(base::RandInt(0, 19)));
+      lock_->Release();
+    }
+  }
+
+  SchedulerLock* const lock_;
+  int acquired_;
+
+  DISALLOW_COPY_AND_ASSIGN(BasicLockTestThread);
+};
+
+class BasicLockAcquireAndWaitThread : public SimpleThread {
+ public:
+  explicit BasicLockAcquireAndWaitThread(SchedulerLock* lock)
+      : SimpleThread("BasicLockAcquireAndWaitThread"),
+        lock_(lock),
+        lock_acquire_event_(false, false),
+        main_thread_continue_event_(false, false) {}
+
+  void WaitForLockAcquisition() {
+    lock_acquire_event_.Wait();
+  }
+
+  void ContinueMain() {
+    main_thread_continue_event_.Signal();
+  }
+
+ private:
+  void Run() override {
+    lock_->Acquire();
+    lock_acquire_event_.Signal();
+    main_thread_continue_event_.Wait();
+    lock_->Release();
+  }
+
+  SchedulerLock* const lock_;
+  WaitableEvent lock_acquire_event_;
+  WaitableEvent main_thread_continue_event_;
+
+  DISALLOW_COPY_AND_ASSIGN(BasicLockAcquireAndWaitThread);
+};
+
+TEST(TaskSchedulerLock, Basic) {
+  SchedulerLock lock;
+  BasicLockTestThread thread(&lock);
+
+  thread.Start();
+
+  int acquired = 0;
+  for (int i = 0; i < 5; i++) {
+    lock.Acquire();
+    acquired++;
+    lock.Release();
+  }
+  for (int i = 0; i < 10; i++) {
+    lock.Acquire();
+    acquired++;
+    PlatformThread::Sleep(TimeDelta::FromMilliseconds(base::RandInt(0, 19)));
+    lock.Release();
+  }
+  for (int i = 0; i < 5; i++) {
+    lock.Acquire();
+    acquired++;
+    PlatformThread::Sleep(TimeDelta::FromMilliseconds(base::RandInt(0, 19)));
+    lock.Release();
+  }
+
+  thread.Join();
+
+  EXPECT_EQ(acquired, 20);
+  EXPECT_EQ(thread.acquired(), 20);
+}
+
+TEST(TaskSchedulerLock, AcquirePredecessor) {
+  SchedulerLock predecessor;
+  SchedulerLock lock(&predecessor);
+  predecessor.Acquire();
+  lock.Acquire();
+  lock.Release();
+  predecessor.Release();
+}
+
+TEST(TaskSchedulerLock, AcquirePredecessorWrongOrder) {
+  SchedulerLock predecessor;
+  SchedulerLock lock(&predecessor);
+  EXPECT_DCHECK_DEATH({
+    lock.Acquire();
+    predecessor.Acquire();
+  }, "");
+}
+
+TEST(TaskSchedulerLock, AcquireNonPredecessor) {
+  SchedulerLock lock1;
+  SchedulerLock lock2;
+  EXPECT_DCHECK_DEATH({
+    lock1.Acquire();
+    lock2.Acquire();
+  }, "");
+}
+
+TEST(TaskSchedulerLock, AcquireMultipleLocksInOrder) {
+  SchedulerLock lock1;
+  SchedulerLock lock2(&lock1);
+  SchedulerLock lock3(&lock2);
+  lock1.Acquire();
+  lock2.Acquire();
+  lock3.Acquire();
+  lock3.Release();
+  lock2.Release();
+  lock1.Release();
+}
+
+TEST(TaskSchedulerLock, AcquireMultipleLocksInTheMiddleOfAChain) {
+  SchedulerLock lock1;
+  SchedulerLock lock2(&lock1);
+  SchedulerLock lock3(&lock2);
+  lock2.Acquire();
+  lock3.Acquire();
+  lock3.Release();
+  lock2.Release();
+}
+
+TEST(TaskSchedulerLock, AcquireMultipleLocksNoTransitivity) {
+  SchedulerLock lock1;
+  SchedulerLock lock2(&lock1);
+  SchedulerLock lock3(&lock2);
+  EXPECT_DCHECK_DEATH({
+    lock1.Acquire();
+    lock3.Acquire();
+  }, "");
+}
+
+TEST(TaskSchedulerLock, AcquireLocksDifferentThreadsSafely) {
+  SchedulerLock lock1;
+  SchedulerLock lock2;
+  BasicLockAcquireAndWaitThread thread(&lock1);
+  thread.Start();
+
+  lock2.Acquire();
+  thread.WaitForLockAcquisition();
+  thread.ContinueMain();
+  thread.Join();
+  lock2.Release();
+}
+
+TEST(TaskSchedulerLock,
+     AcquireLocksWithPredecessorDifferentThreadsSafelyPredecessorFirst) {
+  // A lock and its predecessor may be safely acquired on different threads.
+  // This Thread                Other Thread
+  // predecessor.Acquire()
+  //                            lock.Acquire()
+  // predecessor.Release()
+  //                            lock.Release()
+  SchedulerLock predecessor;
+  SchedulerLock lock(&predecessor);
+  predecessor.Acquire();
+  BasicLockAcquireAndWaitThread thread(&lock);
+  thread.Start();
+  thread.WaitForLockAcquisition();
+  predecessor.Release();
+  thread.ContinueMain();
+  thread.Join();
+}
+
+TEST(TaskSchedulerLock,
+     AcquireLocksWithPredecessorDifferentThreadsSafelyPredecessorLast) {
+  // A lock and its predecessor may be safely acquired on different threads.
+  // This Thread                Other Thread
+  // lock.Acquire()
+  //                            predecessor.Acquire()
+  // lock.Release()
+  //                            predecessor.Release()
+  SchedulerLock predecessor;
+  SchedulerLock lock(&predecessor);
+  lock.Acquire();
+  BasicLockAcquireAndWaitThread thread(&predecessor);
+  thread.Start();
+  thread.WaitForLockAcquisition();
+  lock.Release();
+  thread.ContinueMain();
+  thread.Join();
+}
+
+TEST(TaskSchedulerLock,
+     AcquireLocksWithPredecessorDifferentThreadsSafelyNoInterference) {
+  // Acquisition of an unrelated lock on another thread should not affect a
+  // legal lock acquisition with a predecessor on this thread.
+  // This Thread                Other Thread
+  // predecessor.Acquire()
+  //                            unrelated.Acquire()
+  // lock.Acquire()
+  //                            unrelated.Release()
+  // lock.Release()
+  // predecessor.Release();
+  SchedulerLock predecessor;
+  SchedulerLock lock(&predecessor);
+  predecessor.Acquire();
+  SchedulerLock unrelated;
+  BasicLockAcquireAndWaitThread thread(&unrelated);
+  thread.Start();
+  thread.WaitForLockAcquisition();
+  lock.Acquire();
+  thread.ContinueMain();
+  thread.Join();
+  lock.Release();
+  predecessor.Release();
+}
+
+TEST(TaskSchedulerLock, SelfReferentialLock) {
+  struct SelfReferentialLock {
+    SelfReferentialLock() : lock(&lock) {}
+
+    SchedulerLock lock;
+  };
+
+  EXPECT_DCHECK_DEATH({ SelfReferentialLock lock; }, "");
+}
+
+TEST(TaskSchedulerLock, PredecessorCycle) {
+  struct LockCycle {
+    LockCycle() : lock1(&lock2), lock2(&lock1) {}
+
+    SchedulerLock lock1;
+    SchedulerLock lock2;
+  };
+
+  EXPECT_DCHECK_DEATH({ LockCycle cycle; }, "");
+}
+
+TEST(TaskSchedulerLock, PredecessorLongerCycle) {
+  struct LockCycle {
+    LockCycle()
+        : lock1(&lock5),
+          lock2(&lock1),
+          lock3(&lock2),
+          lock4(&lock3),
+          lock5(&lock4) {}
+
+    SchedulerLock lock1;
+    SchedulerLock lock2;
+    SchedulerLock lock3;
+    SchedulerLock lock4;
+    SchedulerLock lock5;
+  };
+
+  EXPECT_DCHECK_DEATH({ LockCycle cycle; }, "");
+}
+
+}  // namespace
+}  // namespace internal
+}  // namespace base