Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.python > #31061 > unrolled thread

Re: an error in python lib?

Back to article view | Back to comp.lang.python

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: an error in python lib? MRAB <python@mrabarnett.plus.com> - 2012-10-10 02:16 +0100
    Re: an error in python lib? Ulrich Eckhardt <ulrich.eckhardt@dominolaser.com> - 2012-10-10 14:18 +0200
      Re: an error in python lib? Ian Kelly <ian.g.kelly@gmail.com> - 2012-10-10 13:21 -0600
      Re: an error in python lib? Wenhua Zhao <whzhao@gmail.com> - 2012-10-11 15:06 -0700
        Re: an error in python lib? Ulrich Eckhardt <ulrich.eckhardt@dominolaser.com> - 2012-10-12 09:15 +0200

#31061 — Re: an error in python lib?

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.

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

[toc] | [next] | [standalone]

#31080

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

[toc] | [prev] | [next] | [standalone]

#31092

On Wed, Oct 10, 2012 at 6:18 AM, Ulrich Eckhardt
<ulrich.eckhardt@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?

[toc] | [prev] | [next] | [standalone]

#31135

> 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.kelly@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

[toc] | [prev] | [next] | [standalone]

#31151

Am 12.10.2012 00:06, schrieb Wenhua Zhao:
> On Wed, Oct 10, 2012 at 12:21 PM, Ian Kelly <ian.g.kelly@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

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.python

csiph-web