something else i just noticed - i don't believe the behavior of the win32
conditions will be the same as the unix/pthread conditions.
specifically, with the win32 conditions, if a condition is signalled and
there are no waiting threads, the next thread that waits on the condition
will have that signal effectively applied to it. in other words, if thread
1 signals a condition, then thread 2 waits on that condition, thread 2 will
not block. however, i believe this is not the case with pthreads. with
pthreads, thread 2 would block in this situation, i think. again, not an
expert in these matters :)
if i'm correct here, i believe the problem could be solved by only calling
SetEvent in signal/broadcast if cond->num_waiting > 0.
i've attached a patch.
-kevin.
>
> hi. i just happened to be looking through the win32 implementation of
> apr_thread_cond_wait, and it seems to me there could be some
> issues. now,
> i'm not really an expert in this area, so i could be totally
> off, but i
> figured it might be worthwhile to share my thoughts. at the
> very least, i
> might learn something :)
>
> anyway, below is the implementation with my comments
> sprinkled throughout:
>
>
> APR_DECLARE(apr_status_t)
> apr_thread_cond_wait(apr_thread_cond_t *cond,
>
> apr_thread_mutex_t *mutex)
> {
> DWORD rv;
>
> while (1) {
> WaitForSingleObject(cond->mutex, INFINITE);
> cond->num_waiting++;
> ReleaseMutex(cond->mutex);
>
> apr_thread_mutex_unlock(mutex);
> rv = WaitForSingleObject(cond->event, INFINITE);
> /* shouldn't cond->mutex be acquired here with:
> * WaitForSingleObject(cond->mutex, INFINITE);
> * before decrementing cond->num_waiting and
> * examining it later (when cond->signal_all is 1)??
> */
> cond->num_waiting--;
> if (rv == WAIT_FAILED) {
> /* if cond->mutex is acquired above, it should
> * be released here.
> */
> return apr_get_os_error();
> }
> if (cond->signal_all) {
> if (cond->num_waiting == 0) {
> /* shouldn't cond->signal_all be reset here with:
> * cond->signal_all = 0;
> * ?? otherwise, a call to
> apr_thread_cond_broadcast()
> * will result in subsequent calls to
> apr_thread_cond_signal()
> * appearing to be calls to
> apr_thread_cond_broadcast(). i
> * suppose it's an unlikely scenario where for a given
> condition,
> * you'd use broadcast sometimes and signal
> other times.
> not
> * totally unthinkable though.
> */
> 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;
> }
>
Index: thread_cond.c
===================================================================
RCS file: /home/cvspublic/apr/locks/win32/thread_cond.c,v
retrieving revision 1.5
diff -u -w -r1.5 thread_cond.c
--- thread_cond.c 2001/10/12 01:05:03 1.5
+++ thread_cond.c 2001/12/21 04:39:40
@@ -92,12 +92,32 @@
apr_thread_mutex_unlock(mutex);
rv = WaitForSingleObject(cond->event, INFINITE);
+ /* shouldn't cond->mutex be acquired here with:
+ * WaitForSingleObject(cond->mutex, INFINITE);
+ * before decrementing cond->num_waiting and
+ * examining it later (when cond->signal_all is 1)??
+ */
+ WaitForSingleObject(cond->mutex, INFINITE);
cond->num_waiting--;
if (rv == WAIT_FAILED) {
+ /* if cond->mutex is acquired above, it should
+ * be released here.
+ */
+ ReleaseMutex(cond->mutex);
return apr_get_os_error();
}
if (cond->signal_all) {
if (cond->num_waiting == 0) {
+ /* shouldn't cond->signal_all be reset here with:
+ * cond->signal_all = 0;
+ * ?? otherwise, a call to apr_thread_cond_broadcast()
+ * will result in subsequent calls to apr_thread_cond_signal()
+ * appearing to be calls to apr_thread_cond_broadcast(). i
+ * suppose it's an unlikely scenario where for a given
condition,
+ * you'd use broadcast sometimes and signal other times. not
+ * totally unthinkable though.
+ */
+ cond->signal_all = 0;
ResetEvent(cond->event);
}
break;
@@ -125,7 +145,9 @@
WaitForSingleObject(cond->mutex, INFINITE);
cond->signalled = 1;
+ if (cond->num_waiting > 0) {
rv = SetEvent(cond->event);
+ }
ReleaseMutex(cond->mutex);
if (rv == 0) {
return apr_get_os_error();
@@ -140,7 +162,9 @@
WaitForSingleObject(cond->mutex, INFINITE);
cond->signalled = 1;
cond->signal_all = 1;
+ if (cond->num_waiting > 0) {
rv = SetEvent(cond->event);
+ }
ReleaseMutex(cond->mutex);
if (rv == 0) {
return apr_get_os_error();
