Re: svn commit: rev 7004 - branches/svnserve-thread-pools/subversion/svnserve

On Mon, Sep 08, 2003 at 06:24:03PM +0200, Branko ??ibej wrote:
> Sort of. What we need is a reliable Win32 implementation of condition
> variables in APR -- the current one is full of holes and races -- which
> is far from trivial to do. Without that, svnserve would be broken on
> Windows if we merged this to the trunk.
>

Attached is a COMPLETELY UNTESTED patch against APR's HEAD. Care to take
a glance?

The error handling is (fundamentally) broken, because if it has an error
obtaining the condition variable mutex, it cannot update the number of
waiters in the condition variable to reflect the fact that *this* waiter
is no longer waiting. The timeout handling is also broken, because it
should be sensitive to accumulated timeslices, and it isn't. In these
respects, though, it is no worse than the existing implementation.

It may of course be broken in other ways, as well. It might not even
compile!

--ben

Log Message:

Cleanup condition variable implementation; avoid several potential race
conditions (hopefully without creating new ones!). Most importantly:
deliver exactly the right number of signals to only those threads who
were actually waiting at the time of the signal/broadcast.

* include/arch/win32/apr_arch_thread_cond.h
  Remove "signal_all" member, and replace "signalled" variable with
  "num_undelivered".

* locks/win32/thread_cond.c
  Overhaul.

Index: include/arch/win32/apr_arch_thread_cond.h
===================================================================
RCS file: /home/cvspublic/apr/include/arch/win32/apr_arch_thread_cond.h,v
retrieving revision 1.1
diff -r1.1 apr_arch_thread_cond.h
64d63
< int signal_all;
66c65
< int signalled;

---
>     int num_undelivered;
Index: locks/win32/thread_cond.c
===================================================================
RCS file: /home/cvspublic/apr/locks/win32/thread_cond.c,v
retrieving revision 1.12
diff -r1.12 thread_cond.c
78c78
<     (*cond)->signal_all = 0;
---
>     (*cond)->num_undelivered = 0;
87a88
>     /* wait for undelivered signals to be delivered */
93c94,95
<         cond->num_waiting++;
---
>         if (cond->num_undelivered == 0)
>             break;
94a97,99
>     }
>     cond->num_waiters++;
>     ReleaseMutex(cond->mutex);
96c101,103
<         apr_thread_mutex_unlock(mutex);
---
>     /* wait for a signal */
>     apr_thread_mutex_unlock(mutex);
>     while (1) {
98d104
<         cond->num_waiting--;
101c107,109
<             ReleaseMutex(cond->mutex);
---
>             apr_thread_mutex_lock(mutex);
>             /* XXX: disaster!  we don't hold the cond->mutex! */
>             cond->num_waiters--;
104,108c112,118
<         if (cond->signal_all) {
<             if (cond->num_waiting == 0) {
<                 ResetEvent(cond->event);
<             }
<             break;
---
>         res = WaitForSingleObject(cond->mutex, INFINITE);
>         if (res != WAIT_OBJECT_0) {
>             apr_status_t rv = apr_get_os_error();
>             apr_thread_mutex_lock(mutex);
>             /* XXX: disaster!  we don't hold the cond->mutex! */
>             cond->num_waiters--;
>             return rv;
110,113c120,132
<         if (cond->signalled) {
<             cond->signalled = 0;
<             ResetEvent(cond->event);
<             break;
---
>         switch (cond->num_undelivered) {
>             case 0:
>                 ReleaseMutex(cond->mutex);
>                 continue;
>             case 1:
>                 ResetEvent(cond->event);
>                 /* fall through */
>             default: 
>                 cond->num_undelivered--;
>                 cond->num_waiters--;
>                 ReleaseMutex(cond->mutex);
>                 apr_thread_mutex_lock(mutex);
>                 return APR_SUCCESS;
115d133
<         ReleaseMutex(cond->mutex);
117,118d134
<     apr_thread_mutex_lock(mutex);
<     return APR_SUCCESS;
120a137,140
> /* XXX: the timeout is not implemented correctly at all, since there is a
>    potential to wait an unbounded amount of time on a frequently signalled
>    condition variable */
> 
125d144
<     DWORD res;
126a146
>     DWORD res;
127a148
>     /* wait for undelivered signals to be delivered */
131c152
<             if (res == WAIT_TIMEOUT) {
---
>             if (res == WAIT_TIMEOUT)
133d153
<             }
136c156,157
<         cond->num_waiting++;
---
>         if (cond->num_undelivered == 0)
>             break;
137a159,161
>     }
>     cond->num_waiters++;
>     ReleaseMutex(cond->mutex);
139c163,165
<         apr_thread_mutex_unlock(mutex);
---
>     /* wait for a signal */
>     apr_thread_mutex_unlock(mutex);
>     while (1) {
141d166
<         cond->num_waiting--;
144d168
<             ReleaseMutex(cond->mutex);
146c170,172
<             if (res == WAIT_TIMEOUT) {
---
>             /* XXX: disaster!  we don't hold the cond->mutex! */
>             cond->num_waiters--;
>             if (res == WAIT_TIMEOUT)
148,149c174
<             }
<             return apr_get_os_error();
---
>             return rv;
151,155c176,184
<         if (cond->signal_all) {
<             if (cond->num_waiting == 0) {
<                 ResetEvent(cond->event);
<             }
<             break;
---
>         res = WaitForSingleObject(cond->mutex, timeout_ms);
>         if (res != WAIT_OBJECT_0) {
>             apr_status_t rv = apr_get_os_error();
>             apr_thread_mutex_lock(mutex);
>             /* XXX: disaster!  we don't hold the cond->mutex! */
>             cond->num_waiters--;
>             if (res == WAIT_TIMEOUT)
>                 return APR_TIMEUP;
>             return rv;
157,160c186,198
<         if (cond->signalled) {
<             cond->signalled = 0;
<             ResetEvent(cond->event);
<             break;
---
>         switch (cond->num_undelivered) {
>             case 0:
>                 ReleaseMutex(cond->mutex);
>                 continue;
>             case 1:
>                 ResetEvent(cond->event);
>                 /* fall through */
>             default: 
>                 cond->num_undelivered--;
>                 cond->num_waiters--;
>                 ReleaseMutex(cond->mutex);
>                 apr_thread_mutex_lock(mutex);
>                 return APR_SUCCESS;
162d199
<         ReleaseMutex(cond->mutex);
164,165d200
<     apr_thread_mutex_lock(mutex);
<     return APR_SUCCESS;
167a203
> 
177,180c213,217
<     cond->signalled = 1;
<     res = SetEvent(cond->event);
<     if (res == 0) {
<         rv = apr_get_os_error();
---
>     if (cond->num_waiters) {
>         cond->num_undelivered++;
>         if (cond->num_undelivered > cond->num_waiters)
>             cond->num_undelivered = cond->num_waiters;
>         SetEvent(cond->event);
182a220
> 
195,199c233,235
<     cond->signalled = 1;
<     cond->signal_all = 1;
<     res = SetEvent(cond->event);
<     if (res == 0) {
<         rv = apr_get_os_error();
---
>     if (cond->num_waiters) {
>         cond->num_undelivered = cond->num_waiters;
>         SetEvent(cond->event);
201a238
> 
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org