You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Branko Čibej <br...@xbc.nu> on 2003/06/26 00:02:04 UTC
Win32 condition variable rewrite
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 <br...@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)
Re: Win32 condition variable rewrite
Posted by Branko Čibej <br...@xbc.nu>.
Branko Čibej wrote:
>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...
>
>
Well, I was a bit hasty posting that patch -- there is indeed a much
simpler was to implement condition variables on Windows. Here it is.
--
Brane Čibej <br...@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 23:25:04 -0000
@@ -59,11 +59,8 @@
struct apr_thread_cond_t {
apr_pool_t *pool;
- HANDLE event;
- HANDLE mutex;
- int signal_all;
- int num_waiting;
- int signalled;
+ HANDLE broadcast_event;
+ HANDLE signal_event;
};
#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 23:25:04 -0000
@@ -63,8 +63,8 @@
static apr_status_t thread_cond_cleanup(void *data)
{
apr_thread_cond_t *cond = data;
- CloseHandle(cond->mutex);
- CloseHandle(cond->event);
+ CloseHandle(cond->broadcast_event);
+ CloseHandle(cond->signal_event);
return APR_SUCCESS;
}
@@ -73,133 +73,78 @@
{
*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;
+ (*cond)->broadcast_event = CreateEvent(NULL, TRUE, FALSE, NULL);
+ (*cond)->signal_event = CreateEvent(NULL, FALSE, FALSE, NULL);
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)
{
- DWORD res;
+ apr_status_t rv;
+ HANDLE waitlist[2];
+
+ waitlist[0] = cond->broadcast_event;
+ waitlist[1] = cond->signal_event;
+ apr_thread_mutex_unlock(mutex);
+
+ switch (WaitForMultipleObjects(2, waitlist, FALSE, timeout_ms))
+ {
+ case WAIT_FAILED:
+ rv = apr_get_os_error();
+ break;
- 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);
- cond->num_waiting--;
- if (res != WAIT_OBJECT_0) {
- apr_status_t rv = apr_get_os_error();
- ReleaseMutex(cond->mutex);
- return rv;
- }
- if (cond->signal_all) {
- if (cond->num_waiting == 0) {
- ResetEvent(cond->event);
- }
- break;
- }
- if (cond->signalled) {
- cond->signalled = 0;
- ResetEvent(cond->event);
- break;
- }
- ReleaseMutex(cond->mutex);
+ case WAIT_TIMEOUT:
+ rv = APR_TIMEUP;
+ break;
+
+ default:
+ rv = APR_SUCCESS;
+ break;
}
+
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);
-
- 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);
- 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;
- }
- ReleaseMutex(cond->mutex);
- }
- apr_thread_mutex_lock(mutex);
- return APR_SUCCESS;
+ return cond_wait(cond, mutex, (DWORD) apr_time_as_msec(timeout));
}
-APR_DECLARE(apr_status_t) apr_thread_cond_signal(apr_thread_cond_t *cond)
+
+static apr_status_t cond_signal(apr_thread_cond_t *cond, int broadcast)
{
- apr_status_t rv = APR_SUCCESS;
- DWORD res;
+ BOOL result;
- res = WaitForSingleObject(cond->mutex, INFINITE);
- if (res != WAIT_OBJECT_0) {
+ if (broadcast)
+ result = PulseEvent(cond->broadcast_event);
+ else
+ result = PulseEvent(cond->signal_event);
+
+ if (result)
+ return APR_SUCCESS;
+ else
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;
}
-APR_DECLARE(apr_status_t) apr_thread_cond_broadcast(apr_thread_cond_t *cond)
+APR_DECLARE(apr_status_t) apr_thread_cond_signal(apr_thread_cond_t *cond)
{
- apr_status_t rv = APR_SUCCESS;
- DWORD res;
+ return cond_signal(cond, 0);
+}
- 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;
+APR_DECLARE(apr_status_t) apr_thread_cond_broadcast(apr_thread_cond_t *cond)
+{
+ return cond_signal(cond, 1);
}
APR_DECLARE(apr_status_t) apr_thread_cond_destroy(apr_thread_cond_t *cond)
Re: Win32 condition variable rewrite
Posted by Branko Čibej <br...@xbc.nu>.
Hm, I found one problem with both my reimplementations, and this problem
existed in the original implementation, too: there's a race at the point
where the associated mutex is released and before the thread starts
waiting for the trigger event. IIRC, POSIX promises this will be atomic.
I'm not sure how to fix this, short of reverting to some sort of
manual-reset model for the event.
Have to think about this a bit.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/