Re: locks/win32/thread_cond.c

[rb...@rkbloom.net]

I wrote this code initially a long time ago, and since then some others
primarily Will Rowe have tried to improve it.  However, for the most part
the code you are talking about is my original code.

On Tue, 1 Jun 2004, malc wrote:

> Hello,
>
> I have a couple of question regarding win32 condition variables
> implementation.
>
> a. If i am reading the code correctly once apr_thread_cond_broadcast was
>    used cond->signal_all gets set to 1, and even if i later decide to use
>    signal i will still get broadcast semantics (since nobody resets
>    signal_all to zero)

Most likely an oversight.

> b. In apr_thread_cond[_timed]_wait cond->mutex and mutex are reset
>    multiple times, why?

I don't know.  I'll need to dig into this.

>
> c. Return codes of nearly all win32 API calls are not being checked,
>    lazyness?

yes, I was being lazy, I didn't really expect the code to live for very
long.   :-)

Ryan

Re: locks/win32/thread_cond.c

[rb...@rkbloom.net]

On Tue, 1 Jun 2004, [UTF-8] Branko Čibej wrote:

> Klaus Keppler wrote:
>
> > Hello Ryan,
> >
> >>> b. In apr_thread_cond[_timed]_wait cond->mutex and mutex are reset
> >>>   multiple times, why?
> >>
> >
> >> I don't know.  I'll need to dig into this.
> >
> >
> > Recently I wrote a multithreaded application and tested it with
> > heavy load under both linux and windows.
> > The proposed patch (bugzilla #27654) takes care of releasing
> > the mutex (given by parameter) only once; without the "unlock_once"
> > workaround the apr_thread_cond_(timed)wait function ran into a
> > deadlock situation after some loops.
>
> We shouldn't be writing patches for the Win32 thread_cond code. We
> should start from scratch, correctly. The current code has so many race
> conditions it's not even funny. My apologies to the authors, but it
> really is naive.

Yes, it is naive.  I didn't want to write the code at all, but we needed
an implementation.  What I wrote worked well enough to pass the tests, but
I was always hoping somebody would re-write it to use the correct Windows
APIs.  :-)

Ryan

Re: locks/win32/thread_cond.c

[Branko Čibej <br...@xbc.nu>]

Klaus Keppler wrote:

> Hello Ryan,
>
>>> b. In apr_thread_cond[_timed]_wait cond->mutex and mutex are reset
>>>   multiple times, why?
>>
>
>> I don't know.  I'll need to dig into this.
>
>
> Recently I wrote a multithreaded application and tested it with
> heavy load under both linux and windows.
> The proposed patch (bugzilla #27654) takes care of releasing
> the mutex (given by parameter) only once; without the "unlock_once"
> workaround the apr_thread_cond_(timed)wait function ran into a
> deadlock situation after some loops.

We shouldn't be writing patches for the Win32 thread_cond code. We
should start from scratch, correctly. The current code has so many race
conditions it's not even funny. My apologies to the authors, but it
really is naive.

I've been sort of working on this...procrastinating really...for a while
now. I'll definitely have a go at dusting off and committing what I have
done.

-- 
Brane Čibej   <br...@xbc.nu>   http://www.xbc.nu/brane/

Re: locks/win32/thread_cond.c

[Klaus Keppler <kl...@informatik.stud.uni-erlangen.de>]

Hello Ryan,

>>b. In apr_thread_cond[_timed]_wait cond->mutex and mutex are reset
>>   multiple times, why?

> I don't know.  I'll need to dig into this.

Recently I wrote a multithreaded application and tested it with
heavy load under both linux and windows.
The proposed patch (bugzilla #27654) takes care of releasing
the mutex (given by parameter) only once; without the "unlock_once"
workaround the apr_thread_cond_(timed)wait function ran into a
deadlock situation after some loops.
The MSDN docs say that you have to unlock a mutex as often as
it was locked; so I think that unlocking a mutex more often
than it was locked will result in a negative "lock count".

Having a second look at the code, cond->mutex is only released
a second time at the end of the while(1) loop, just before acquiring the
mutex againt (in WaitForSingleObject at loop start).
What's the reason for this? Ryan, do you remember?

Here's my actual (patched) version of apr_thread_cond_wait
(as a base for discussion, maybe?)

Bye,

    Klaus


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);

     int unlock_once = 1;

     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);

/* start patch */
         if (unlock_once) {
             unlock_once = 0;
             apr_thread_mutex_unlock(mutex);
         }
/* end patch */
         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); /* <- what's this for? */
     }
     apr_thread_mutex_lock(mutex);
     return APR_SUCCESS;
}