an error in python lib?

[Wenhua Zhao]

Hi list,

I just noticed that in /usr/lib/python2.7/threading.py

class _Condition(_Verbose):
...
def _is_owned(self):
# Return True if lock is owned by current_thread.
# This method is called only if __lock doesn't have
_is_owned().
if self.__lock.acquire(0):
self.__lock.release()
return False
else:
return True

The return values seem to be wrong. They should be swapped:

def _is_owned(self):
if self.__lock.acquire(0):
self.__lock.release()
return True
else:
return False

Or I understood it wrong here?

Thanks,
Wenhua

[MRAB]

The .acquire method will return True if the attempt to acquire has been
successful. This can occur only if it is not currently owned.

In pseudocode:

if the attempt to acquire it succeeds:
no-one owed it before, but now I own it
release it
now no-one owns it
return False
else:
someone already owns it
return True

[Ulrich Eckhardt]

Am 10.10.2012 02:32, schrieb Wenhua Zhao:
> I just noticed that in /usr/lib/python2.7/threading.py
>
> class _Condition(_Verbose):
> ...
> def _is_owned(self):
> # Return True if lock is owned by current_thread.
> # This method is called only if __lock doesn't have

> # _is_owned().

> if self.__lock.acquire(0):
> self.__lock.release()
> return False
> else:
> return True
>
> The return values seem to be wrong. They should be swapped:
>
> def _is_owned(self):
> if self.__lock.acquire(0):
> self.__lock.release()
> return True
> else:
> return False
>
> Or I understood it wrong here?

I think you are correct, but there is one thing that I would audit
first: The whole code there seems to use integers in places where a
boolean would be appropriate, like e.g. the 'blocking' parameter to
acquire(). I wouldn't be surprised to find the interpretation of "0
means no error" in some places there, so that a False translates to 0
and then to "OK, I have the lock".

Also, assuming an underlying implementation where a nonblocking
acquire() could still newly acquire an uncontended lock, that
implementation would release the acquired lock and still return "yes I'm
holding the lock", which would be dead wrong. It must verify if the lock
count is at least 2 after acquiring the lock.

Uli

[Ulrich Eckhardt]

Am 10.10.2012 03:16, schrieb MRAB:
> On 2012-10-10 01:32, Wenhua Zhao wrote:
>> Hi list,
>>
>> I just noticed that in /usr/lib/python2.7/threading.py
>>
>> class _Condition(_Verbose):
>> ...
>> def _is_owned(self):
>> # Return True if lock is owned by current_thread.
>> # This method is called only if __lock doesn't have

>> # _is_owned().

>> if self.__lock.acquire(0):
>> self.__lock.release()
>> return False
>> else:
>> return True
>>
>> The return values seem to be wrong. They should be swapped:
>>
>> def _is_owned(self):
>> if self.__lock.acquire(0):
>> self.__lock.release()
>> return True
>> else:
>> return False
>>
>> Or I understood it wrong here?
>>
> The .acquire method will return True if the attempt to acquire has been
> successful. This can occur only if it is not currently owned.

The comment clearly states "owned by current thread", not "owned by any
thread". The latter would also be useless, as that can change
concurrently at any time when owned by a different thread, so making
decisions on this state is futile. Also, acquire() can also return true
when locking recursively, at least that's how I read the sources.

I think that this is really a bug, but it doesn't surface often because
the built-in lock has its own _is_owned() function which is used instead
of this flawed logic.

Uli

[Ian Kelly]

On Wed, Oct 10, 2012 at 6:18 AM, Ulrich Eckhardt
<ulrich....@dominolaser.com> wrote:
>> The .acquire method will return True if the attempt to acquire has been
>> successful. This can occur only if it is not currently owned.
>
>
> The comment clearly states "owned by current thread", not "owned by any
> thread". The latter would also be useless, as that can change concurrently
> at any time when owned by a different thread, so making decisions on this
> state is futile.

If you're correct, then the bug runs deeper than simply swapping the
return values. If the first case returned True instead of False, then
it would be returning True when the lock is not owned by any thread.

> Also, acquire() can also return true when locking
> recursively, at least that's how I read the sources.

Python 2.7.1 (r271:86832, Nov 27 2010, 18:30:46) [MSC v.1500 32 bit (Intel)] on
win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from threading import Lock
>>> lock = Lock()
>>> lock.acquire(0)
True
>>> lock.acquire(0)
False

> I think that this is really a bug, but it doesn't surface often because the
> built-in lock has its own _is_owned() function which is used instead of this
> flawed logic.

Can you demonstrate an API bug that is caused by this?

[Wenhua Zhao]

> On Wed, Oct 10, 2012 at 6:18 AM, Ulrich Eckhardt

>> The comment clearly states "owned by current thread", not "owned by any
>> thread". The latter would also be useless, as that can change concurrently
>> at any time when owned by a different thread, so making decisions on this
>> state is futile.

Agree.

On Wed, Oct 10, 2012 at 12:21 PM, Ian Kelly <ian.g...@gmail.com> wrote:
> Can you demonstrate an API bug that is caused by this?

A simple demo of this error is:

-----------8<------------------------8<----------------------
import time
from threading import Condition, Lock, Thread

cv = Condition(Lock())

def do_acquire():
cv.acquire()
print "cv acquired in thread"
time.sleep(5)
cv.release()
print "cv released in thread"

thread = Thread(target=do_acquire)
thread.start()

for i in range(10):
print "in main cv._is_owned: ", cv._is_owned()
time.sleep(1)
-----------8<------------------------8<----------------------

The condition variable is only acquired in the thread, so in main it should
always print false. However, my output gives:

$ python test.py
cv acquired in threadin main cv._is_owned:
True
in main cv._is_owned: True
in main cv._is_owned: True
in main cv._is_owned: True
in main cv._is_owned: True
cv released in thread
in main cv._is_owned: False
in main cv._is_owned: False
in main cv._is_owned: False
in main cv._is_owned: False
in main cv._is_owned: False

However, as long as you follow the generic pattern, i.e. always use
acquire() before wait(), this error is not triggered.

Thanks all.
Wenhua

[Ulrich Eckhardt]

Am 12.10.2012 00:06, schrieb Wenhua Zhao:
> On Wed, Oct 10, 2012 at 12:21 PM, Ian Kelly <ian.g...@gmail.com> wrote:
>> Can you demonstrate an API bug that is caused by this?
>
> A simple demo of this error is:

[...]

> print "in main cv._is_owned: ", cv._is_owned()

That is kind of cheating, because as far as I can tell that function is
private and not even documented in any way. You can use wait() though,
which should always raise a RuntimeError:

----------->8------------------------>8----------------------

import time
from threading import Condition, Lock, Thread

cv = Condition(Lock())

def do_acquire():
cv.acquire()
print "cv acquired in thread"
time.sleep(5)
cv.release()
print "cv released in thread"

thread = Thread(target=do_acquire)
thread.start()

for i in range(10):

try:
cv.wait()
print "in main cv.wait() succeeded"
except RuntimeError:
print "in main cv.wait() raised RuntimeError"
time.sleep(1)

----------->8------------------------>8----------------------

This gives me the following output:

in main cv.wait() raised RuntimeErrorcv acquired in thread

Exception in thread Thread-1:
Traceback (most recent call last):
File "C:\Python27\lib\threading.py", line 551, in __bootstrap_inner
self.run()
File "C:\Python27\lib\threading.py", line 504, in run
self.__target(*self.__args, **self.__kwargs)
File "ttest.py", line 10, in do_acquire
cv.release()
error: release unlocked lock

Following that, the program hangs. It seems the wait() released the lock
that it didn't own, causing the error in do_acquire(). It then hung in
wait(), although it should have raised a RuntimeError instead.


Uli