Merge pull request #143 from kspangsege/ks-windows-pthreads-errors

Errors in Windows port of POSIX Threads

src/tightdb/thread.cpp

@@ -8,9 +8,13 @@
 #endif
 
 #ifdef _POSIX_THREAD_PROCESS_SHARED
-#  define TIGHTDB_HAVE_PTHREAD_PROCESS_SHARED
-#  if _POSIX_THREADS >= 200809L
-#    define TIGHTDB_HAVE_ROBUST_PTHREAD_MUTEX
+#  if _POSIX_THREAD_PROCESS_SHARED != -1 // can apparently also be -1
+#    define TIGHTDB_HAVE_PTHREAD_PROCESS_SHARED
+#    if !defined _WIN32 // robust not supported by our windows pthreads port
+#      if _POSIX_THREADS >= 200809L
+#        define TIGHTDB_HAVE_ROBUST_PTHREAD_MUTEX
+#      endif
+#    endif
 #  endif
 #endif
 

src/win32/pthread/pthread.h

@@ -466,7 +466,7 @@ extern "C"
 #define _POSIX_THREAD_PRIORITY_SCHEDULING -1
 
 #undef _POSIX_THREAD_PROCESS_SHARED
-#define _POSIX_THREAD_PROCESS_SHARED -1
+#define _POSIX_THREAD_PROCESS_SHARED 1
 
 
 /*

src/win32/pthread/pthread_mutex_init.c

@@ -61,23 +61,12 @@ pthread_mutex_init (pthread_mutex_t * mutex, const pthread_mutexattr_t * attr)
            * Creating mutex that can be shared between
            * processes.
            */
-#if _POSIX_THREAD_PROCESS_SHARED >= 0
-
-          /*
-           * Not implemented yet.
-           */
-
-#error ERROR [__FILE__, line __LINE__]: Process shared mutexes are not supported yet.
-
-#else
           mutex->is_shared = 1;
 
           // Create unique and random mutex name. UuidCreate() needs linking with Rpcrt4.lib, so we use CoCreateGuid() 
           // instead. That way end-user won't need to mess with Visual Studio project settings
           CoCreateGuid(&guid);
-          sprintf(mutex->shared_name, "Global\\%08X%04hX%04hX%02X%02X%02X%02X%02X%02X%02X%02X", guid.Data1, guid.Data2, guid.Data3, guid.Data4[0], guid.Data4[1], guid.Data4[2], guid.Data4[3], guid.Data4[4], guid.Data4[5], guid.Data4[6], guid.Data4[7]);
-          mutex->shared_name[33] = '\0';
-
+          sprintf_s(mutex->shared_name, sizeof(mutex->shared_name), "Global\\%08X%04X%04X%02X%02X%02X%02X%02X", guid.Data1, guid.Data2, guid.Data3, guid.Data4[0], guid.Data4[1], guid.Data4[2], guid.Data4[3], guid.Data4[4]);
           h = CreateMutexA(NULL, 0, mutex->shared_name);
           if(h == NULL)
               return EAGAIN;
@@ -87,8 +76,6 @@ pthread_mutex_init (pthread_mutex_t * mutex, const pthread_mutexattr_t * attr)
           mutex->cached_windows_pid = GetCurrentProcessId();
 
           return 0;
-
-#endif /* _POSIX_THREAD_PROCESS_SHARED */
         }
     }
 
@@ -144,7 +131,6 @@ pthread_mutex_init (pthread_mutex_t * mutex, const pthread_mutexattr_t * attr)
         }
     }
 
-  mutex->original = mx.original;
-
+  memcpy(mutex, &mx, sizeof(mx));
   return (result);
 }

src/win32/pthread/pthread_spin_init.c

@@ -69,7 +69,7 @@ pthread_spin_init (pthread_spinlock_t * lock, int pshared)
 	   * Not implemented yet.
 	   */
 
-#error ERROR [__FILE__, line __LINE__]: Process shared spin locks are not supported yet.
+//#error ERROR [__FILE__, line __LINE__]: Process shared spin locks are not supported yet.
 
 #else
 

test/test_thread.cpp

@@ -1,3 +1,4 @@
+#include <cstring>
 #include <algorithm>
 #include <queue>
 
@@ -20,18 +21,31 @@ void increment(int* i)
     ++*i;
 }
 
-
 struct Shared {
     Mutex m_mutex;
     int m_value;
 
-    void increment_1000_times()
+    // 10000 takes less than 0.1 sec
+    void increment_10000_times()
     {
-        for (int i=0; i<1000; ++i) {
+        for (int i=0; i<10000; ++i) {
             Mutex::Lock lock(m_mutex);
             ++m_value;
         }
     }
+
+    void increment_10000_times2()
+    {
+        for (int i=0; i<10000; ++i) {
+            Mutex::Lock lock(m_mutex);
+            // Create a time window where thread interference can take place. Problem with ++m_value is that it
+            // could assemble into 'inc [addr]' which has very tiny gap
+            double f = m_value;
+            f += 1.;
+            m_value = int(f);
+        }
+    }
+
 };
 
 
@@ -161,18 +175,6 @@ TEST(Thread_Start)
 }
 
 
-// FIXME: Our POSIX Threads port for Windows seems to have a number
-// of bugs that prevent the rest of the unit-tests from working.
-// One bug appears to be that pthread_mutex_lock() incorrectly
-// believes that a regular mutex is a process-shared mutex.
-// It also looks like there is an error when calling
-// pthread_mutex_destroy() on a process-shared mutex.
-// And finally, it appears that it incorrectly claims support for
-// robust mutexes.
-// Lasse, could you take a look at it?
-
-#ifndef _WIN32
-
 TEST(Thread_MutexLock)
 {
     Mutex mutex;
@@ -203,10 +205,23 @@ TEST(Thread_CriticalSection)
     shared.m_value = 0;
     Thread threads[10];
     for (int i = 0; i < 10; ++i)
-        threads[i].start(util::bind(&Shared::increment_1000_times, &shared));
+        threads[i].start(util::bind(&Shared::increment_10000_times, &shared));
+    for (int i = 0; i < 10; ++i)
+        threads[i].join();
+    CHECK_EQUAL(100000, shared.m_value);
+}
+
+
+TEST(Thread_CriticalSection2)
+{
+    Shared shared;
+    shared.m_value = 0;
+    Thread threads[10];
+    for (int i = 0; i < 10; ++i)
+        threads[i].start(util::bind(&Shared::increment_10000_times2, &shared));
     for (int i = 0; i < 10; ++i)
         threads[i].join();
-    CHECK_EQUAL(10000, shared.m_value);
+    CHECK_EQUAL(100000, shared.m_value);
 }
 
 
@@ -356,11 +371,11 @@ TEST(Thread_DeathDuringRecovery)
 TEST(Thread_CondVar)
 {
     QueueMonitor queue;
-    const int num_producers = 2; // 32;
-    const int num_consumers = 2; // 32;
+    const int num_producers = 32;
+    const int num_consumers = 32;
     Thread producers[num_producers], consumers[num_consumers];
     int consumed_counts[num_consumers][num_producers];
-    fill(&consumed_counts[0][0], &consumed_counts[num_consumers][num_producers], 0);
+    memset(consumed_counts, 0, sizeof consumed_counts);
 
     for (int i = 0; i < num_producers; ++i)
         producers[i].start(util::bind(&producer_thread, &queue, i));
@@ -379,5 +394,3 @@ TEST(Thread_CondVar)
         CHECK_EQUAL(1000, n);
     }
 }
-
-#endif // _WIN32