Re: [VOTE] APR 1.2.0

[Henry Jen]

E Holyat wrote:

Here is a patch for win32 that has been tested extensively for a few
 months now.  I submitted it to bugzilla

The previous patch addressed only the unlock being called more than
once.

This attachment avoids race conditions that the previous patch
doesn't. This patch also fixes the multiple calls to unlock. This
patch also consolidates the the duplicate efforts in
apr_thread_cond_wait and apr_thread_cond_timedwait

Seems like there are couple patches around, here is the one I submitted
on Jun 02, this patch is used by jxta-c project.

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.

--- 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;
 }