Re: Win32 condition variable rewrite
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 <[EMAIL PROTECTED]> http://www.xbc.nu/brane/
Re: Win32 condition variable rewrite
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 <[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 - 1.1 +++ include/arch/win32/apr_arch_thread_cond.h 25 Jun 2003 23:25:04 - @@ -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 - 1.12 +++ locks/win32/thread_cond.c 25 Jun 2003 23:25:04 - @@ -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); - -a
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 <[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 - 1.1 +++ include/arch/win32/apr_arch_thread_cond.h 25 Jun 2003 21:29:25 - @@ -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 - 1.12 +++ locks/win32/thread_cond.c 25 Jun 2003 21:29:25 - @@ -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_c