Cross-platform thread primitives

test/Thread.cpp

Index: test/Thread.cpp
===================================================================
--- a/test/Thread.cpp
+++ b/test/Thread.cpp
@@ -1,5 +1,6 @@
 #include <gtest/gtest.h>
 #include <queue>
+#include <vector>
 
 #include "../src/Thread.h"
 
@@ -16,6 +17,7 @@
   {
   public:
     int timesCalled;
+    AdblockPlus::Mutex mutex;
 
     Mock() : timesCalled(0)
     {
@@ -23,7 +25,10 @@
 
     void Run()
     {
+      Sleep(5);

[Wladimir Palant, 2013/04/04 07:09:00]

Why is this necessary? Seems random.

[Felix Dahlke, 2013/04/04 07:27:40]

See below.

+      mutex.Lock();
       timesCalled++;
+      mutex.Unlock();
     }
   };
 
@@ -32,72 +37,70 @@
   public:
     bool working;
 
-    LockingMock(AdblockPlus::Thread::Mutex& mutex, const int millisToSleep)
-      : mutex(mutex), millisToSleep(millisToSleep)
+    LockingMock(const std::string& name, std::vector<std::string>& log,
+                AdblockPlus::Mutex& logMutex)
+      : name(name), log(log), logMutex(logMutex)
     {
     }
 
     void Run()
     {
-      mutex.Lock();
-      working = true;
-      Sleep(millisToSleep);
-      working = false;
-      mutex.Unlock();
+      logMutex.Lock();
+      log.push_back(name);
+      logMutex.Unlock();
     }
 
   private:
-    AdblockPlus::Thread::Mutex& mutex;
-    int millisToSleep;
+    const std::string name;
+    std::vector<std::string>& log;
+    AdblockPlus::Mutex& logMutex;
   };
 
   class Enqueuer : public AdblockPlus::Thread
   {
   public:
-    Enqueuer(std::queue<int>& queue, AdblockPlus::Thread::Mutex& mutex,
-             AdblockPlus::Thread::Condition& notEmpty)
-      : queue(queue), mutex(mutex), notEmpty(notEmpty)
+    Enqueuer(std::queue<int>& queue, AdblockPlus::Mutex& queueMutex,
+             AdblockPlus::ConditionVariable& notEmpty)
+      : queue(queue), queueMutex(queueMutex), notEmpty(notEmpty)
     {
     }
 
     void Run()
     {
-      Sleep(5);
-      mutex.Lock();
+      queueMutex.Lock();
       queue.push(1);
-      Sleep(10);
       notEmpty.Signal();
-      mutex.Unlock();
+      queueMutex.Unlock();
     }
 
   private:
     std::queue<int>& queue;
-    AdblockPlus::Thread::Mutex& mutex;
-    AdblockPlus::Thread::Condition& notEmpty;
+    AdblockPlus::Mutex& queueMutex;
+    AdblockPlus::ConditionVariable& notEmpty;
   };
 
   class Dequeuer : public AdblockPlus::Thread
   {
   public:
-    Dequeuer(std::queue<int>& queue, AdblockPlus::Thread::Mutex& mutex,
-             AdblockPlus::Thread::Condition& notEmpty)
-      : queue(queue), mutex(mutex), notEmpty(notEmpty)
+    Dequeuer(std::queue<int>& queue, AdblockPlus::Mutex& queueMutex,
+             AdblockPlus::ConditionVariable& notEmpty)
+      : queue(queue), queueMutex(queueMutex), notEmpty(notEmpty)
     {
     }
 
     void Run()
     {
-      mutex.Lock();
+      queueMutex.Lock();
       if (!queue.size())
-        notEmpty.Wait(mutex);
+        notEmpty.Wait(queueMutex);
       queue.pop();
-      mutex.Unlock();
+      queueMutex.Unlock();
     }
 
   private:
     std::queue<int>& queue;
-    AdblockPlus::Thread::Mutex& mutex;
-    AdblockPlus::Thread::Condition& notEmpty;
+    AdblockPlus::Mutex& queueMutex;
+    AdblockPlus::ConditionVariable& notEmpty;
   };
 }
 
@@ -106,41 +109,39 @@
   Mock mock;
   ASSERT_EQ(0, mock.timesCalled);
   mock.Start();
+  mock.mutex.Lock();

[Wladimir Palant, 2013/04/04 07:09:00]

This should be done before mock.Start(), otherwise the thread might lock the
mutex before you do.

[Felix Dahlke, 2013/04/04 07:27:40]

That's what the 5ms sleep above is for. It's not brilliant, but that way it will
at least test that there's actually a separate thread of execution.

[Wladimir Palant, 2013/04/04 09:26:53]

Oh well :-(

   ASSERT_EQ(0, mock.timesCalled);
+  mock.mutex.Unlock();
   mock.Join();
   ASSERT_EQ(1, mock.timesCalled);
 }
 
 TEST(ThreadTest, Mutex)
 {
-  AdblockPlus::Thread::Mutex mutex;
-  LockingMock mock1(mutex, 10);
-  LockingMock mock2(mutex, 20);
+  std::vector<std::string> log;
+  AdblockPlus::Mutex logMutex;
+  LockingMock mock1("mock1", log, logMutex);
+  LockingMock mock2("mock2", log, logMutex);
   mock1.Start();
+  Sleep(5);
   mock2.Start();

[Wladimir Palant, 2013/04/04 07:09:00]

This test doesn't make sense - mock1 will already be finished by the time mock2
starts, locking or not. Should LockingMock have Sleep(10) before pushing its
name to the log?

Actually, there are ordering assumptions here regardless - these will be correct
most of the time but there is no guarantee. How about starting both mock1 and
mock2 immediately? They could log "name started" when they start, sleep a bit
and log "name done" before releasing the lock. There will be two valid results:
"mock1 started", "mock1 done", "mock2 started", "mock2 done" and "mock2
started", "mock2 done", "mock1 started", "mock1 done".

[Felix Dahlke, 2013/04/04 07:27:40]

Yeah, that's a much better test. Did that.

-  Sleep(5);
-  ASSERT_TRUE(mock1.working);
-  ASSERT_FALSE(mock2.working);
-  Sleep(10);
-  ASSERT_TRUE(mock2.working);
-  ASSERT_FALSE(mock1.working);
   mock1.Join();
   mock2.Join();
+  ASSERT_EQ("mock1", log[0]);
+  ASSERT_EQ("mock2", log[1]);
 }
 
 TEST(ThreadTest, ConditionVariable)
 {
   std::queue<int> queue;
-  AdblockPlus::Thread::Mutex mutex;
-  AdblockPlus::Thread::Condition notEmpty;
-  Enqueuer enqueuer(queue, mutex, notEmpty);
-  Dequeuer dequeuer(queue, mutex, notEmpty);
+  AdblockPlus::Mutex queueMutex;
+  AdblockPlus::ConditionVariable notEmpty;
+  Dequeuer dequeuer(queue, queueMutex, notEmpty);
+  Enqueuer enqueuer(queue, queueMutex, notEmpty);
+  dequeuer.Start();
+  Sleep(5);
   enqueuer.Start();
-  dequeuer.Start();
-  Sleep(10);
-  ASSERT_EQ(1u, queue.size());
-  Sleep(15);
-  ASSERT_EQ(0u, queue.size());
   enqueuer.Join();
   dequeuer.Join();
+  ASSERT_EQ(0, queue.size());

[Wladimir Palant, 2013/04/04 09:26:53]

This fails to compile for me once again, 0u please.

 }