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
> 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:
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
> 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:
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.
> 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:
> 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.
<ulrich.eckha...@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.
> 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?
> 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.ke...@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.
> On Wed, Oct 10, 2012 at 12:21 PM, Ian Kelly <ian.g.ke...@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:
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)
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.
