Condition._is_owned is wrong

[anntzer]

BPO	20247
Nosy	@tim-one, @mdickinson, @berkerpeksag, @anntzer
Superseder	bpo-25516: threading.Condition._is_owned() is wrong when using threading.Lock

^{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 = None
closed_at = <Date 2016-04-29.12:24:45.095>
created_at = <Date 2014-01-14.00:55:21.466>
labels = ['library']
title = 'Condition._is_owned is wrong'
updated_at = <Date 2016-04-29.12:24:45.093>
user = 'https://github.com/anntzer'

bugs.python.org fields:

activity = <Date 2016-04-29.12:24:45.093>
actor = 'berker.peksag'
assignee = 'none'
closed = True
closed_date = <Date 2016-04-29.12:24:45.095>
closer = 'berker.peksag'
components = ['Library (Lib)']
creation = <Date 2014-01-14.00:55:21.466>
creator = 'Antony.Lee'
dependencies = []
files = []
hgrepos = []
issue_num = 20247
keywords = []
message_count = 4.0
messages = ['208063', '208131', '210582', '264493']
nosy_count = 5.0
nosy_names = ['tim.peters', 'mark.dickinson', 'neologix', 'berker.peksag', 'Antony.Lee']
pr_nums = []
priority = 'normal'
resolution = 'duplicate'
stage = 'resolved'
status = 'closed'
superseder = '25516'
type = None
url = 'https://bugs.python.org/issue20247'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']

[anntzer]

I believe that the implementation of Condition._is_owned is wrong, as mentioned here: https://mail.python.org/pipermail/python-list/2012-October/632682.html. Specifically, the two return values (True and False) should be inverted.

I guess this slipped through as this private function would only be called if one passed to a Condition object a (R)Lock-like object that doesn't define _is_owned. (I noticed this when I tried to pass a custom-written reader-writer lock to a Condition object.) Technically, the docs says that "If the lock argument is given and not None, it must be a Lock or RLock object", but in practice anything that defines acquire(), release() (and possibly _is_owned()) should work.

[tim-one]

They certainly should _not_ be swapped, as explained clearly in the message following the one you referenced. For the first half:

        if self._lock.acquire(0):

succeeds if and only if the lock is not held by _any_ thread at the time. In that case, the lock is _acquired_ as a side-effect of performing the test, and the code goes on to restore the lock again to its initially unacquired state, and (correctly) return False:

            self._lock.release()
            return False

The second half of the code is certainly wrong:

    else:
        return True

The docs state:

    # Return True if lock is owned by current_thread.

but the code actually returns True if the lock is currently owned by _any_ thread. All that is also explained in the thread you pointed at.

However, I don't see any possibility of fixing that. There is simply no way to know, for an arbitrary "lock-like" object, which thread owns it.

Indeed, on Windows it's not even possible for a threading.Lock. For example (here under Python3, but I'm sure it's the same under Python2):

>>> import threading as th
>>> lock = th.Lock()
>>> c = th.Condition(lock)
>>> def hmm():
...   print("is_owned:", c._is_owned())
...
>>> t = th.Thread(target=hmm)
>>> t.start()
is_owned: False
>>> lock.acquire()
True
>>> t = th.Thread(target=hmm)
>>> t.start()
is_owned: True

Note that the last line showed True despite that the lock is _not_ owned by the thread doing the print - it's owned by the main program thread. That's because threading.Lock._is_owned doesn't exist under Windows, so it's falling into the second case of the Condition._is_owned() implementation at issue here.

Also note that if the True and False cases were swapped, _none_ of the output would make any sense at all ;-)

[anntzer]

For the second half, the same behavior applies under Linux -- basically, a simple Lock doesn't have a notion of owning thread and can the be unlocked by any thread.

However, the first half is not correct if the lock used is a *RLock-like* object (that is, it has a notion of ownership, and once acquired it can be reacquired and released only by the owning thread). If that is the case, then _lock.acquire(0) will succeed if the current thread already owns the lock (or if no one owns it) and fail if the lock is owned by another thread.

So perhaps it may not be possible to do really better, but the docs could (should?) mention that custom implementations of locks passed to Condition objects should implement their own version of _is_owned.

[berkerpeksag]

bpo-25516 is a duplicate, but I'm going to close this one since bpo-25516 has a patch.