Re: Boost.Threads Win32 notify

From: bill_kempf (williamkempf_at_[hidden])
Date: 2002-01-01 10:24:57

--- In boost_at_y..., Rob Lievaart <Rob.Lievaart_at_n...> wrote:
> Hi,
> 
> I was trying out the threads library when I ran into
> the following problem.  When using a bounded buffer
> (the one from the example) the system locked up if
> I had more then one reader and I started the readers
> before the writers. With only one reader, or if I 
> started the writer thread first, it all seemed to 
> work fine.  Also changing the notify_one to notify_all
> made it work again.
> 
> When I looked into the implementation of the condition 
> variable (Which is non-trivial, to say the least) I found
> that in both notify_one and notify_all not all control paths
> release the m_mutex member, it is allways locked, but
> in one specific case is never unlocked.  
> 
> This also happes for the m_gate member, but is unlocked in
> other functions (were it is never locked).  I could not find
> such a case for the m_mutex member.  The code in question
> is:
> 
> void condition::notify_one()
> {
>     unsigned signals = 0;
> 
>     int res = 0;
>     res = WaitForSingleObject(reinterpret_cast<HANDLE>(m_mutex), 
INFINITE);
>     assert(res == WAIT_OBJECT_0);
> 
>     if (m_waiting != 0) // the m_gate is already closed
>     {
>         if (m_blocked == 0)
>         {
>             res = ReleaseMutex(reinterpret_cast<HANDLE>(m_mutex));
>             assert(res);
>             return;
>         }
> 
>         ++m_waiting;
>         --m_blocked;
> 	// NEVER UNLOCKED
>     }
>     else
>     {
>  	...snip...
>     }
> }
> 
> If m_waiting != 0 and m_blocked != 0 then m_mutex is never unlocked.
> if add the release at the location of the comment everything seems
> to work.  But, unfortunately, that proves nothing in multitreaded
> programs.  Locking m_mutex and not unlocking it seems wrong to
> me, but given the implementation I could be missing something.
> 
> So is this a bug in the code, or a bug in my head?

It's a bug in the code.  A fix has been checked into CVS.

Bill Kempf