Cross-platform thread primitives

src/Thread.cpp

+#include "Thread.h"
+
+using namespace AdblockPlus;
+
+namespace
+{
+  void CallRun(Thread* thread)
+  {
+    thread->Run();
+  }
+}
+
+Thread::Mutex::Mutex()
+{
+#ifdef WIN32
+  InitializeCriticalSection(&nativeMutex);
+#else
+  pthread_mutex_init(&nativeMutex, 0);
+#endif
+}
+
+Thread::Mutex::~Mutex()
+{
+  Unlock();

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

That's a race condition - you unlock the mutex so some thread waiting on it can
grab it before you delete it. Just delete it, do not unlock before you do.

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

Both DeleteCriticalSection and pthread_mutex_destroy lead to undefined behaviour
when called on a locked mutex, so... 

But you're right, this may cause trouble. Then again, it'd be quite the bug if
one thread would try to lock the mutex while it's being destructed.

[Wladimir Palant, 2013/04/03 19:30:16]

To me it sounds like AutoLock is a better solution to ensure that a mutex is
definitely unlocked when it is being destroyed...

[Felix Dahlke, 2013/04/04 02:52:58]

Yeah, you're probably right. I was planning to wrap pthreads/Win32 threads
thinly after all, so it's better not to have any magic here. Removed the
Unlock() call.

+#ifdef WIN32
+  DeleteCriticalSection(&nativeMutex);
+#else
+  pthread_mutex_destroy(&nativeMutex);
+#endif
+}
+
+void Thread::Mutex::Lock()
+{
+#ifdef WIN32
+  EnterCriticalSection(&nativeMutex);
+#else
+  pthread_mutex_lock(&nativeMutex);
+#endif
+}
+
+void Thread::Mutex::Unlock()
+{
+#ifdef WIN32
+  LeaveCriticalSection(&nativeMutex);
+#else
+  pthread_mutex_unlock(&nativeMutex);
+#endif
+}

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

I would be in favor of an AutoLock helper class that will unlock the mutex
automatically when the scope is exited:

    {
      AutoLock al(mutex);
      // Do something here
    }

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

Agreed, that's neat. I'll add one later with a new review.

+
+Thread::Condition::Condition()
+{
+#ifdef WIN32
+  InitializeConditionVariable(&nativeCondition);
+#else
+  pthread_cond_init(&nativeCondition, 0);
+#endif
+}
+
+Thread::Condition::~Condition()
+{
+#ifndef WIN32
+  Signal();

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

This is a race condition again. Why would we want to signal before destroying
anyway? This will only introduce an inconsistency compared to Windows.

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

As above, to avoid undefined behaviour.

[Wladimir Palant, 2013/04/03 19:30:16]

I think that not destroying conditions that are still in use is a better way to
avoid undefined behavior - otherwise we are looking for trouble no matter what
we do.

[Felix Dahlke, 2013/04/04 02:52:58]

Yeah, same as above, removed the Signal() call.

+  pthread_cond_destroy(&nativeCondition);
+#endif
+}
+
+void Thread::Condition::Wait(Thread::Mutex& mutex)
+{
+#ifdef WIN32
+  SleepConditionVariableCS(&nativeCondition, &mutex.nativeMutex, INFINITE);
+#else
+  pthread_cond_wait(&nativeCondition, &mutex.nativeMutex);
+#endif
+}
+
+void Thread::Condition::Signal()
+{
+#ifdef WIN32
+  WakeConditionVariable(&nativeCondition);
+#else
+  pthread_cond_signal(&nativeCondition);
+#endif
+}
+
+Thread::~Thread()
+{
+}
+
+void Thread::Start()
+{
+#ifdef WIN32
+  CreateThread(0, 0, &CallRun, 0, this, &thread);

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

Wrong order of parameters? |this| should passed as fourth parameter.

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

Yep, I somehow mixed that up and didn't manage to pull properly before compiling
my changes on Windows, so it seemed fine... Oleksandr fixed this.

+#else
+  pthread_create(&thread, 0, (void *(*)(void*)) &CallRun, this);
+#endif
+}
+
+void Thread::Join()
+{
+#ifdef WIN32
+  WaitForSingleObject(thread, INFINITE);
+#else
+  pthread_join(thread, 0);
+#endif
+}