Re: [patch] Deadlock in apr_thread_cond_wait on WIN32 platform [BugID 27654, 34336]

[Henry Jen]

Oops, keep reminding myself to attach the patch, but in the end ...

Here it is.


On Thu, 2005-06-02 at 23:05 -0700, Henry Jen wrote:
> Hi,
> 
> We encountered the issue as reported in Bug
> 27654(http://issues.apache.org/bugzilla/show_bug.cgi?id=27654), too bad
> I did not check the issue database earlier until I figured it out after
> 3 days.
> 
> The patch in the issues database should be working, is there a reason
> for not committing it?
> 
> Anyway, I have created another patch(see attachment) to address the same
> issue and which would also fix bug 34336 as it totally removing the
> cond->mutex. I believe using the mutex passed in to ensure mutual access
> should be sufficient.
> 
> Unless the different threads can use different mutex for the same cond,
> but I don't see that as a valid usage and nor can I think of a use case
> for that.
> 
> Please review it, comments are welcome. I would like to see the patch be
> committed to head and also back port to 0.9.x release. The patch had
> been tested for both apr-0.9.6 and SVN trunk with 'patch -Np0' in the
> apr folder.
>  
> Cheers,
-- 
Henry Jen aka slowhog
http://blogs.sun.com/slowhog

--- include/arch/win32/apr_arch_thread_cond.orig_096.h	2005-02-04 12:36:31.000000000 -0800
+++ include/arch/win32/apr_arch_thread_cond.h	2005-06-02 18:00:01.250000000 -0700
@@ -22,7 +22,6 @@
 struct apr_thread_cond_t {
     apr_pool_t *pool;
     HANDLE event;
-    HANDLE mutex;
     int signal_all;
     int num_waiting;
     int signalled;
--- locks/win32/thread_cond.orig_096.c	2005-06-02 22:27:51.265625000 -0700
+++ locks/win32/thread_cond.c	2005-06-02 22:34:06.078125000 -0700
@@ -25,7 +25,6 @@
 static apr_status_t thread_cond_cleanup(void *data)
 {
     apr_thread_cond_t *cond = data;
-    CloseHandle(cond->mutex);
     CloseHandle(cond->event);
     return APR_SUCCESS;
 }
@@ -36,7 +35,6 @@
     *cond = apr_palloc(pool, sizeof(**cond));
     (*cond)->pool = pool;
     (*cond)->event = CreateEvent(NULL, TRUE, FALSE, NULL);
-    (*cond)->mutex = CreateMutex(NULL, FALSE, NULL);
     (*cond)->signal_all = 0;
     (*cond)->num_waiting = 0;
     return APR_SUCCESS;
@@ -48,20 +46,14 @@
     DWORD res;
 
     while (1) {
-        res = WaitForSingleObject(cond->mutex, INFINITE);
-        if (res != WAIT_OBJECT_0) {
-            return apr_get_os_error();
-        }
         cond->num_waiting++;
-        ReleaseMutex(cond->mutex);
 
         apr_thread_mutex_unlock(mutex);
         res = WaitForSingleObject(cond->event, INFINITE);
+        apr_thread_mutex_lock(mutex);
         cond->num_waiting--;
         if (res != WAIT_OBJECT_0) {
-            apr_status_t rv = apr_get_os_error();
-            ReleaseMutex(cond->mutex);
-            return rv;
+            return apr_get_os_error();
         }
         if (cond->signal_all) {
             if (cond->num_waiting == 0) {
@@ -74,9 +66,7 @@
             ResetEvent(cond->event);
             break;
         }
-        ReleaseMutex(cond->mutex);
     }
-    apr_thread_mutex_lock(mutex);
     return APR_SUCCESS;
 }
 
@@ -88,23 +78,14 @@
     DWORD timeout_ms = (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);
 
         apr_thread_mutex_unlock(mutex);
         res = WaitForSingleObject(cond->event, timeout_ms);
+        apr_thread_mutex_lock(mutex);
         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;
             }
@@ -121,9 +102,7 @@
             ResetEvent(cond->event);
             break;
         }
-        ReleaseMutex(cond->mutex);
     }
-    apr_thread_mutex_lock(mutex);
     return APR_SUCCESS;
 }
 
@@ -132,16 +111,11 @@
     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;
 }
 
@@ -150,17 +124,12 @@
     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;
 }
 

signature.asc
Description: This is a digitally signed message part