bug in PyThread_release_lock()

[shihao]

BPO	433625
Nosy	@gvanrossum, @tim-one, @navytux

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/tim-one'
closed_at = <Date 2019-09-11.11:34:31.933>
created_at = <Date 2001-06-16.02:17:17.000>
labels = ['interpreter-core', 'invalid']
title = 'bug in PyThread_release_lock()'
updated_at = <Date 2019-09-11.11:34:31.933>
user = 'https://bugs.python.org/shihao'

bugs.python.org fields:

activity = <Date 2019-09-11.11:34:31.933>
actor = 'navytux'
assignee = 'tim.peters'
closed = True
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2001-06-16.02:17:17.000>
creator = 'shihao'
dependencies = []
files = []
hgrepos = []
issue_num = 433625
keywords = []
message_count = 13.0
messages = ['5065', '5066', '5067', '5068', '5069', '5070', '5071', '5072', '5073', '5074', '5075', '5076', '351829']
nosy_count = 4.0
nosy_names = ['gvanrossum', 'tim.peters', 'shihao', 'navytux']
pr_nums = []
priority = 'normal'
resolution = 'not a bug'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue433625'
versions = []

[shihao]

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 */
}

[tim-one]

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.

[gvanrossum]

Logged In: YES
user_id=6380

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

[tim-one]

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.

[tim-one]

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.

[shihao]

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.

[tim-one]

Logged In: YES
user_id=31435

It appears you're concerned that the signal will "get lost"
in this scenario. I agree that it may, but it doesn't
matter: thread 2's "while (thelock->locked)" test fails
because thread 1 already set thelock->locked to 0, so
thread 2 doesn't execute its pthread_cond_wait, so it
doesn't matter that nobody is *waiting* to see thread 1's
pthread_cond_signal. IOW, the condition thread 1 will try
to signal has *already* been detected by thread 2, and the
signal is useless (because redundant) information. Indeed,
it's a beauty of the condition protocol that signals can be
sloppy.

The linuxthread man page is worded strangely here, but note
that it does not say the mutex *must* be locked. They
can't, either, because POSIX doesn't require it; while
POSIX stds aren't available freely online, some derived
specs are, and are much clearer about this; e.g., see the
Single Unix Specification, here:

<http://www.opengroup.org/onlinepubs/7908799/xsh/pthread_con
d_signal.html>

I'll tell you why I don't *want* to change this: in
Python's use of the global interpreter lock, it's almost
always the case that someone is waiting on the lock. By
releasing the mutex before signaling, this gives a waiter a
chance to run immediately upon calling
pthread_cond_signal. Else, because pthread_cond_wait
(which the waiters are executing) has to lock the mutex, if
the signaler is holding the mutex during the signal,
pthread_cond_signal can't finish the job -- it was to go
back to the signaler and let it unlock the mutex first
before a waiter can proceed. This makes the region of
exclusion longer than it needs to be.

So if there's not an actual race problem here (& I still
don't see one), I don't want to change this.