Should asyncio.{Lock,Semaphore}.locked() return True when there are waiters?

[gvanrossum]

Currently when an asyncio Lock or Semaphore has been recently vacated but still has waiters queued up, the locked() method may return False, making prospective acquirers believe that if they call acquire() they will immediately proceed, without blocking.

In a threading system with pre-emptive scheduling (like Python's threading module, which uses OS threads) this makes sense, because locked() never makes any guarantee: code like

if not lock.locked():
    lock.acquire()

may block in the acquire() call because another thread may be executing similar code and there's a race.

But in asyncio there's no race -- there is no pre-emptive task switching and everything associated with a particular event loop runs in a single OS thread. So we could make it so that locked() checks if the queue is empty and returns True only if it is. But should we?

CC: @kumaraditya303 @cykerway @njs.

[kumaraditya303]

I assume you meant @njsmith not @njs :)

[njsmith]

What makes this a tough call? Like, this method doesn't seem that important in the grand scheme of things, but being more accurate is generally nicer than being less accurate, right?

[gvanrossum]

What makes this a tough call?

Just that in the past it's always worked like this.

I came up with a restaurant metaphor for semaphores (surely not unique) and it's the difference between "there is currently a table with no dining guests" and "we can seat you right away".

Also the code for acquire() calls locked() in a few places so we'd have to change those to self._value == 0 otherwise the algorithm would change. Perhaps worse, the Semaphore class could be subclassed and a subclass might expect the algorithm to call locked()? (This seems like a bad idea, but who knows (https://xkcd.com/1172/).

[cykerway]

That metaphor gets the point. We can be more accurate with three methods (names can vary): has_table(), has_waiter() and can_seat(). They are pretty straightforward to implement. The semantics of locked() was changed from can_seat() to has_table() in the recent commit to facilitate the fix. Users may want locked() to work like can_seat(), but I'm not sure. It's easy to update acquire() once the API is decided.

[gvanrossum]

But was it really changed? The old code also checked for value==0 in locked() and in release() it also bumped the value. So the case where there are waiters who have been buzzed but who haven't yet claimed their table would be the same, wouldn't it?

This issue is not about making the change, it's about deciding what locked() should return.

(While I have your attention, did you see #93222 (comment)?)

[cykerway]

But was it really changed? The old code also checked for value==0 in locked() and in release() it also bumped the value. So the case where there are waiters who have been buzzed but who haven't yet claimed their table would be the same, wouldn't it?

As someone pointed out, the old code might be buggy on that. It meant to be can_seat() but was actually has_table(). This was fixed in the recent commit by changing its semantics (to be consistent with Lock.locked() which doesn't care waiters). Yet we don't know if this is what we want because we are having this issue now.

This issue is not about making the change, it's about deciding what locked() should return.

I guess can_seat() is probably what they want?

(While I have your attention, did you see #93222 (comment)?)

Yeah, there I dropped a reply.

[gvanrossum]

Okay, um, let me summarize what I am seeing.

• In the distant past, locked() meant can_seat().
• The previous fix (the one before yours) accidentally made it mean has_table(), as was pointed out by that comment.
• The only thing that I can see was fixed in your commit was that you changed the docstring; locked() still returns has_table(), and the docstring now acknowledges that.

Have I got that right?

Since in the distant past it meant can_seat(), and that is clearly the more useful semantics, we should change it to that, right?

[cykerway]

Yeah you got everything, except for "the distant past" which I don't know.

The above PR changed the semantics of locked() from has_table() to can_seat(), while at the same time addressing the performance problem you mentioned in another issue. It's a fantasy without much proofread so take my words with a grain of salt.

[kumaraditya303]

@gvanrossum Is this fixed by #97549?

[gvanrossum]

Yes, it is, thanks for the reminder.