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 <[EMAIL PROTECTED]> 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)
