Cross-platform thread primitives

test/Thread.cpp

 #include <gtest/gtest.h>
 #include <queue>
+#include <vector>
 
 #include "../src/Thread.h"
 
 namespace
 {
 #ifndef WIN32
   void Sleep(const int millis)
   {
       usleep(millis * 1000);
   }
 #endif
 
   class Mock : public AdblockPlus::Thread
   {
   public:
     int timesCalled;
+    AdblockPlus::Mutex mutex;
 
     Mock() : timesCalled(0)
     {
     }
 
     void Run()
     {
+      Sleep(5);
+      mutex.Lock();
       timesCalled++;

[Wladimir Palant, 2013/04/03 13:14:47]

This statement isn't thread-safe, you need a mutex to ensure that this variable
isn't incremented twice in parallel.

[Felix Dahlke, 2013/04/03 16:27:59]

I thought that's pretty unlikely here, but fair enough, fixed it.

+      mutex.Unlock();
     }
   };
 
   class LockingMock : public AdblockPlus::Thread
   {
   public:
     bool working;
 
-    LockingMock(AdblockPlus::Thread::Mutex& mutex, const int millisToSleep)
-      : mutex(mutex), millisToSleep(millisToSleep)
+    LockingMock(std::vector<std::string>& log, AdblockPlus::Mutex& logMutex)
+      : log(log), logMutex(logMutex)
     {
     }
 
     void Run()
     {
-      mutex.Lock();
-      working = true;
-      Sleep(millisToSleep);
-      working = false;
-      mutex.Unlock();
+      logMutex.Lock();
+      log.push_back("started");
+      Sleep(5);
+      log.push_back("ended");
+      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;
   };
 }
 
 TEST(ThreadTest, Run)
 {
   Mock mock;
   ASSERT_EQ(0, mock.timesCalled);
   mock.Start();
+  mock.mutex.Lock();
   ASSERT_EQ(0, mock.timesCalled);

[Wladimir Palant, 2013/04/03 13:14:47]

This is a race condition - by the time you check the call count might have been
incremented already.

[Felix Dahlke, 2013/04/03 16:27:59]

Yes indeed, made it a bit more robust. It's still a kind of silly test though,
any idea for a better one?

+  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(log, logMutex);
+  LockingMock mock2(log, logMutex);
   mock1.Start();
   mock2.Start();
-  Sleep(5);
-  ASSERT_TRUE(mock1.working);
-  ASSERT_FALSE(mock2.working);

[Wladimir Palant, 2013/04/03 13:14:47]

Please note that Sleep() (both the Windows and the POSIX variants) merely
guarantees that a thread will sleep *at least* a given number of milliseconds.
It does not guarantee that it will be exactly that number of milliseconds. In
other words, the assert here can fail if the main thread gets delayed (e.g.
because some other heavy action is going on). It's better to have the threads
push their states to some variable and later verify that the order of these
states is correct.

[Felix Dahlke, 2013/04/03 16:27:59]

Agreed, much more robust that way.

-  Sleep(10);
-  ASSERT_TRUE(mock2.working);
-  ASSERT_FALSE(mock1.working);
   mock1.Join();
   mock2.Join();
+  ASSERT_EQ("started", log[0]);
+  ASSERT_EQ("ended", log[1]);
+  ASSERT_EQ("started", log[2]);
+  ASSERT_EQ("ended", log[3]);
 }
 
 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(1, queue.size());

[Wladimir Palant, 2013/04/03 13:14:47]

Same flaw here as above.

[Felix Dahlke, 2013/04/03 16:27:59]

Fixed as well.

-  Sleep(15);
-  ASSERT_EQ(0, queue.size());
   enqueuer.Join();
   dequeuer.Join();
+  ASSERT_EQ(0, queue.size());
 }