Buggy conditions

[Peter Slacik <Peter.Slacik@tatramed.sk>]

Hi there,

I've got problems with code which uses pthread_cond_timedwait() +
pthread_cond_broadcast(). Sometimes threads deadlocks locking all the same mutex
(bound to the condition variable).

It seems that if thread wakes up due to the timeout, it omits to check whether the
condition variable was broadcasted concurrently. The waiter returns, but the
signaling thread waits on cv->waitersDone event to be set. If the last thread,
which decrements cv->waiters, timed out in _pthread_sem_timedwait(),
((result=errno)==0) is false and cv->waitersDone is never set. The waiter then
attempts to lock external mutex, locked by the broadcasting thread.

Additionally, it turned out that cv's data is not protected enough, e.g.
cv->waiters may be decremented by thread waked up after timeout and incremented by
thread wishing to wait at the same time. Therefore cv->waitersLock is really
requested there.

Just working around... I'm sorry to post untested code, I have no opportunity to
compile my attempt today to check it's correctness.

One question more: What happens, if thread waits on semaphore object, wakes up
after timeout and then another (broadcasting) thread posts the semaphore? Probably
another thread will spuriously wake up next time, nothing more. Can someone tell
his/her opinion to this?

--
Peter Slacik


===================================================================
RCS file: pthread/condvar.c,v
retrieving revision 1.4
diff -c -r1.4 condvar.c
*** condvar.c	1999/05/31 14:00:11	1.4
--- condvar.c	1999/06/23 16:34:50
***************
*** 459,472 ****
  
    if (cv != (pthread_cond_t) _PTHREAD_OBJECT_AUTO_INIT)
      {
        if (cv->waiters > 0)
  	{
  	  return EBUSY;
  	}
  
        (void) sem_destroy (&(cv->sema));
-       (void) pthread_mutex_destroy (&(cv->waitersLock));
        (void) CloseHandle (cv->waitersDone);
  
        free(cv);
      }
--- 459,477 ----
  
    if (cv != (pthread_cond_t) _PTHREAD_OBJECT_AUTO_INIT)
      {
+       if (pthread_mutex_lock(&(cv->waitersLock)) != 0)
+ 	return EINVAL;
+ 
        if (cv->waiters > 0)
  	{
+ 	  (void) pthread_mutex_unlock(&(cv->waitersLock));
  	  return EBUSY;
  	}
  
        (void) sem_destroy (&(cv->sema));
        (void) CloseHandle (cv->waitersDone);
+       (void) pthread_mutex_unlock(&(cv->waitersLock));
+       (void) pthread_mutex_destroy (&(cv->waitersLock));
  
        free(cv);
      }
***************
*** 510,519 ****
    cv = *cond;
  
    /*
!    * OK to increment  cond->waiters because the caller locked 'mutex'
     */
    cv->waiters++;
  
    /*
     * We keep the lock held just long enough to increment the count of
     * waiters by one (above).
--- 515,532 ----
    cv = *cond;
  
    /*
!    * It's not OK to increment cond->waiters while the caller locked 'mutex',
!    * there may be another threads just waking up (with 'mutex' unlocked)
!    * and cv->... data is not protected.
     */
+   if (pthread_mutex_lock(&(cv->waitersLock)) != 0)
+     return EINVAL;
+ 
    cv->waiters++;
  
+   if (pthread_mutex_unlock(&(cv->waitersLock)) != 0)
+     return EINVAL;
+ 
    /*
     * We keep the lock held just long enough to increment the count of
     * waiters by one (above).
***************
*** 526,532 ****
        /*
         * Wait to be awakened by
         *              pthread_cond_signal, or
!        *              pthread_cond_broadcast
         *
         * Note: 
         *      _pthread_sem_timedwait is a cancellation point,
--- 539,546 ----
        /*
         * Wait to be awakened by
         *              pthread_cond_signal, or
!        *              pthread_cond_broadcast, or
!        *              timeout
         *
         * Note: 
         *      _pthread_sem_timedwait is a cancellation point,
***************
*** 548,555 ****
    if ((internal_result = pthread_mutex_lock (&(cv->waitersLock))) == 0)
      {
        /*
!        * By making the waiter responsible for decrementing
!        * its count we don't have to worry about having an internal
         * mutex.
         */
        cv->waiters--;
--- 562,569 ----
    if ((internal_result = pthread_mutex_lock (&(cv->waitersLock))) == 0)
      {
        /*
!        * The waiter is responsible for decrementing
!        * its count, protected by an internal
         * mutex.
         */
        cv->waiters--;
***************
*** 559,565 ****
        internal_result = pthread_mutex_unlock (&(cv->waitersLock));
      }
  
!   if (result == 0 && internal_result == 0)
      {
        if (lastWaiter)
          {
--- 573,579 ----
        internal_result = pthread_mutex_unlock (&(cv->waitersLock));
      }
  
!   if ((result == 0 || result == ETIMEDOUT) && internal_result == 0)
      {
        if (lastWaiter)
          {
***************
*** 816,822 ****
        * ------------------------------------------------------
        */
  {
!   int result = 0;
    pthread_cond_t cv;
  
    if (cond == NULL || *cond == NULL)
--- 830,837 ----
        * ------------------------------------------------------
        */
  {
!   int result = 0, internal_result = 0;
!   int wereWaiters = FALSE;
    pthread_cond_t cv;
  
    if (cond == NULL || *cond == NULL)
***************
*** 834,849 ****
        return 0;
      }
  
    cv->wasBroadcast = TRUE;
  
    /*
!    * Wake up all waiters
     */
    result = (ReleaseSemaphore( cv->sema, cv->waiters, NULL )
  	    ? 0
  	    : EINVAL);
  
!   if (cv->waiters > 0 && result == 0)
      {
        /*
         * Wait for all the awakened threads to acquire their part of
--- 849,872 ----
        return 0;
      }
  
+   if ((internal_result = pthread_mutex_lock(&(cv->waitersLock))) == EINVAL)
+     {
+       return EINVAL;
+     }
+ 
    cv->wasBroadcast = TRUE;
+   wereWaiters = (cv->waiters > 0);
  
    /*
!    * Wake up all waiters. Does it make sense to check the real number of waiters?
     */
    result = (ReleaseSemaphore( cv->sema, cv->waiters, NULL )
  	    ? 0
  	    : EINVAL);
  
!   internal_result = pthread_mutex_unlock(&(cv->waitersLock));
! 
!   if (wereWaiters && result == 0)
      {
        /*
         * Wait for all the awakened threads to acquire their part of
***************
*** 862,868 ****
      }
  
    return (result);
- 
  }
  
  
--- 885,890 ----