apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Branko ─îibej <br...@xbc.nu>
Subject Win32 condition variable rewrite
Date Wed, 25 Jun 2003 22:02:04 GMT
I've been trying to use the APR condition variables on Win32, and the
dam' things kept deadlocking on me whatever I did. So, out of sheer
desperation, I took a look at the implementation... well, let's say that
I was awed by the number of bugs and race conditions I found in there. :-)

Anyway: I went and rewrote the whole thing; now I'd like some review and
feedback before I commit this. I'm 99% sure it's foolproof, but you
never know...

-- 
Brane ─îibej   <brane@xbc.nu>   http://www.xbc.nu/brane/

Index: include/arch/win32/apr_arch_thread_cond.h
===================================================================
RCS file: /home/cvs/apr/include/arch/win32/apr_arch_thread_cond.h,v
retrieving revision 1.1
diff -u -u -r1.1 apr_arch_thread_cond.h
--- include/arch/win32/apr_arch_thread_cond.h	6 Jan 2003 23:44:27 -0000	1.1
+++ include/arch/win32/apr_arch_thread_cond.h	25 Jun 2003 21:29:25 -0000
@@ -59,11 +59,10 @@
 
 struct apr_thread_cond_t {
     apr_pool_t *pool;
+    CRITICAL_SECTION lock;
     HANDLE event;
-    HANDLE mutex;
-    int signal_all;
+    int num_to_signal;
     int num_waiting;
-    int signalled;
 };
 
 #endif  /* THREAD_COND_H */
Index: locks/win32/thread_cond.c
===================================================================
RCS file: /home/cvs/apr/locks/win32/thread_cond.c,v
retrieving revision 1.12
diff -u -u -r1.12 thread_cond.c
--- locks/win32/thread_cond.c	27 Feb 2003 19:20:24 -0000	1.12
+++ locks/win32/thread_cond.c	25 Jun 2003 21:29:25 -0000
@@ -63,7 +63,7 @@
 static apr_status_t thread_cond_cleanup(void *data)
 {
     apr_thread_cond_t *cond = data;
-    CloseHandle(cond->mutex);
+    DeleteCriticalSection(&cond->lock);
     CloseHandle(cond->event);
     return APR_SUCCESS;
 }
@@ -73,133 +73,94 @@
 {
     *cond = apr_palloc(pool, sizeof(**cond));
     (*cond)->pool = pool;
+    InitializeCriticalSection(&(*cond)->lock);
     (*cond)->event = CreateEvent(NULL, TRUE, FALSE, NULL);
-    (*cond)->mutex = CreateMutex(NULL, FALSE, NULL);
-    (*cond)->signal_all = 0;
+    (*cond)->num_to_signal = 0;
     (*cond)->num_waiting = 0;
     return APR_SUCCESS;
 }
 
-APR_DECLARE(apr_status_t) apr_thread_cond_wait(apr_thread_cond_t *cond,
-                                               apr_thread_mutex_t *mutex)
+static apr_status_t cond_wait(apr_thread_cond_t *cond,
+                              apr_thread_mutex_t *mutex,
+                              DWORD timeout_ms)
 {
+    apr_status_t rv;
     DWORD res;
 
+    EnterCriticalSection(&cond->lock);
+    apr_thread_mutex_unlock(mutex);
+    ++cond->num_waiting;
     while (1) {
-        res = WaitForSingleObject(cond->mutex, INFINITE);
-        if (res != WAIT_OBJECT_0) {
-            return apr_get_os_error();
-        }
-        cond->num_waiting++;
-        ReleaseMutex(cond->mutex);
+        LeaveCriticalSection(&cond->lock);
+        res = WaitForSingleObject(cond->event, timeout_ms);
+        EnterCriticalSection(&cond->lock);
 
-        apr_thread_mutex_unlock(mutex);
-        res = WaitForSingleObject(cond->event, INFINITE);
-        cond->num_waiting--;
-        if (res != WAIT_OBJECT_0) {
-            apr_status_t rv = apr_get_os_error();
-            ReleaseMutex(cond->mutex);
-            return rv;
+        if (res == WAIT_FAILED) {
+            rv = apr_get_os_error();
+            break;
         }
-        if (cond->signal_all) {
-            if (cond->num_waiting == 0) {
-                ResetEvent(cond->event);
-            }
+
+        if (res == WAIT_TIMEOUT) {
+            rv = APR_TIMEUP;
             break;
         }
-        if (cond->signalled) {
-            cond->signalled = 0;
-            ResetEvent(cond->event);
+
+        if (cond->num_to_signal != 0) {
+            --cond->num_to_signal;
+            rv = APR_SUCCESS;
             break;
         }
-        ReleaseMutex(cond->mutex);
     }
+    --cond->num_waiting;
+    LeaveCriticalSection(&cond->lock);
     apr_thread_mutex_lock(mutex);
-    return APR_SUCCESS;
+    return rv;
+}
+
+APR_DECLARE(apr_status_t) apr_thread_cond_wait(apr_thread_cond_t *cond,
+                                               apr_thread_mutex_t *mutex)
+{
+    return cond_wait(cond, mutex, INFINITE);
 }
 
 APR_DECLARE(apr_status_t) apr_thread_cond_timedwait(apr_thread_cond_t *cond,
                                                     apr_thread_mutex_t *mutex,
                                                     apr_interval_time_t timeout)
 {
-    DWORD res;
-    DWORD timeout_ms = (DWORD) apr_time_as_msec(timeout);
+    return cond_wait(cond, mutex, (DWORD) apr_time_as_msec(timeout));
+}
 
-    while (1) {
-        res = WaitForSingleObject(cond->mutex, timeout_ms);
-        if (res != WAIT_OBJECT_0) {
-            if (res == WAIT_TIMEOUT) {
-                return APR_TIMEUP;
-            }
-            return apr_get_os_error();
-        }
-        cond->num_waiting++;
-        ReleaseMutex(cond->mutex);
+static apr_status_t cond_signal(apr_thread_cond_t *cond, int broadcast)
+{
+    apr_status_t rv = APR_SUCCESS;
 
-        apr_thread_mutex_unlock(mutex);
-        res = WaitForSingleObject(cond->event, timeout_ms);
-        cond->num_waiting--;
-        if (res != WAIT_OBJECT_0) {
-            apr_status_t rv = apr_get_os_error();
-            ReleaseMutex(cond->mutex);
-            apr_thread_mutex_lock(mutex);
-            if (res == WAIT_TIMEOUT) {
-                return APR_TIMEUP;
-            }
-            return apr_get_os_error();
-        }
-        if (cond->signal_all) {
-            if (cond->num_waiting == 0) {
-                ResetEvent(cond->event);
-            }
-            break;
-        }
-        if (cond->signalled) {
-            cond->signalled = 0;
-            ResetEvent(cond->event);
-            break;
+    EnterCriticalSection(&cond->lock);
+    if (cond->num_waiting != 0)
+    {
+        if (!PulseEvent(cond->event))
+        {
+            rv = apr_get_os_error();
+        }
+        else
+        {
+            if (broadcast)
+                cond->num_to_signal += cond->num_waiting;
+            else
+                cond->num_to_signal += 1;
         }
-        ReleaseMutex(cond->mutex);
     }
-    apr_thread_mutex_lock(mutex);
-    return APR_SUCCESS;
+    LeaveCriticalSection(&cond->lock);
+    return rv;
 }
 
 APR_DECLARE(apr_status_t) apr_thread_cond_signal(apr_thread_cond_t *cond)
 {
-    apr_status_t rv = APR_SUCCESS;
-    DWORD res;
-
-    res = WaitForSingleObject(cond->mutex, INFINITE);
-    if (res != WAIT_OBJECT_0) {
-        return apr_get_os_error();
-    }
-    cond->signalled = 1;
-    res = SetEvent(cond->event);
-    if (res == 0) {
-        rv = apr_get_os_error();
-    }
-    ReleaseMutex(cond->mutex);
-    return rv;
+    return cond_signal(cond, 0);
 }
 
 APR_DECLARE(apr_status_t) apr_thread_cond_broadcast(apr_thread_cond_t *cond)
 {
-    apr_status_t rv = APR_SUCCESS;
-    DWORD res;
-
-    res = WaitForSingleObject(cond->mutex, INFINITE);
-    if (res != WAIT_OBJECT_0) {
-        return apr_get_os_error();
-    }
-    cond->signalled = 1;
-    cond->signal_all = 1;
-    res = SetEvent(cond->event);
-    if (res == 0) {
-        rv = apr_get_os_error();
-    }
-    ReleaseMutex(cond->mutex);
-    return rv;
+    return cond_signal(cond, 1);
 }
 
 APR_DECLARE(apr_status_t) apr_thread_cond_destroy(apr_thread_cond_t *cond)




Mime
View raw message