[Python-bugs-list] [ python-Bugs-433625 ] bug in PyThread_release_lock()

[noreply@sourceforge.net]

Bugs item #433625, was updated on 2001-06-15 19:17
You can respond by visiting: 
http://sourceforge.net/tracker/?func=detail&atid=105470&aid=433625&group_id=5470

Category: Threads
Group: Platform-specific
>Status: Open
Resolution: Invalid
Priority: 5
Submitted By: Shih-Hao Liu (shihao)
Assigned to: Tim Peters (tim_one)
Summary: bug in PyThread_release_lock()

Initial Comment:
Mutex should be hold when calling
pthread_cond_signal().  This function should look like:


PyThread_release_lock(PyThread_type_lock lock)
{
        pthread_lock *thelock = (pthread_lock *)lock;
        int status, error = 0;

        dprintf(("PyThread_release_lock(%p) called\n",
lock));

        status = pthread_mutex_lock( &thelock->mut );
        CHECK_STATUS("pthread_mutex_lock[3]");

        thelock->locked = 0;
        /* ***** call pthread_cond_signal before unlock
mutex */
        status = pthread_cond_signal(
&thelock->lock_released );
        CHECK_STATUS("pthread_cond_signal");

        status = pthread_mutex_unlock( &thelock->mut );
        CHECK_STATUS("pthread_mutex_unlock[3]");

        /* wake up someone (anyone, if any) waiting on
the lock */
}


----------------------------------------------------------------------

>Comment By: Shih-Hao Liu (shihao)
Date: 2001-06-17 23:01

Message:
Logged In: YES 
user_id=246388

I closed it because I thought the thelock->locked variable 
will ensure that the PyThread_release_lock will help to 
protect the condition variable and I was wrong.  The 
linuxthread man page on pthread_cond_signal:

A  condition  variable  must  always  be associated with a
mutex, to avoid the race condition where a thread prepares
to wait on a condition variable and another thread signals
the condition just before the first thread actually  waits
on it.

which means you can't call pthread_cond_signal & 
pthread_cond_wait on the same condition variable at the 
same time.  And using a mutex to protect them is a good 
idea.  Here is how thing might go wrong with current 
implementation:

    thread 1                            thread 2        
                                                        
                            |int PyThread_acquire_lock _
                            |/** assume lock was acquired
                            |  by thread 1, hence locked=0 
                            |  & success would be 0  **/
                            |{                          
                            |  ...                      
                            |  status = pthread_mutex_lo
                            |  CHECK_STATUS("pthread_mut
                            |  success = thelock->locked
                            |  if (success) thelock->loc
                            |  status = pthread_mutex_un
                            |  /** thread 2 suspended **/
void PyThread_release_lock _|                           
{                           |                           
  ...                       |                           
  status = pthread_mutex_loc|                           
  CHECK_STATUS("pthread_mute|                           
                            |                           
  thelock->locked = 0;      |                           
                            |                           
  status = pthread_mutex_unl|                           
/** thread 1 suspend **/    |                           
                            |  CHECK_STATUS("pthread_mut
                            |                           
                            |  if ( !success && waitflag
                            |    /* continue trying unti
                            |                           
                            |    /* mut must be locked b
                            |     * protocol */         
                            |    status = pthread_mutex_
                            |    CHECK_STATUS("pthread_m
                            |    while ( thelock->locked
                            |      status = pthread_cond
                            |/** thread 2 suspended while
                            |    updating shared data **
  CHECK_STATUS("pthread_mute|                           
                            |                           
  /* wake up someone (anyone|                           
  status = pthread_cond_sign|                           
/** thread 1 update shared  |                           
  data and corrupt it. **/  |                           

Not sure what the effect would be.  It's wouldn't be nice 
anyway.





----------------------------------------------------------------------

Comment By: Tim Peters (tim_one)
Date: 2001-06-17 16:02

Message:
Logged In: YES 
user_id=31435

Closing this again, as it appears the original submitter 
deleted it.  shihao, if you want to pursure this, open it 
again.


----------------------------------------------------------------------

Comment By: Tim Peters (tim_one)
Date: 2001-06-17 15:01

Message:
Logged In: YES 
user_id=31435

Ack, did I delete this?!  I sure didn't intend to -- didn't 
even intend to close it.  Reopened pending more info.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2001-06-17 14:51

Message:
Logged In: YES 
user_id=6380

Set status to closed -- no need to delete it.

----------------------------------------------------------------------

Comment By: Tim Peters (tim_one)
Date: 2001-06-17 14:16

Message:
Logged In: YES 
user_id=31435

Why?  It's allowed to signal the condition whether or not 
the mutex is held.  Since changing this can have visible 
effects on thread scheduling, I'm reluctant to change it 
without a good reason.


----------------------------------------------------------------------

You can respond by visiting: 
http://sourceforge.net/tracker/?func=detail&atid=105470&aid=433625&group_id=5470