GeezerVison is showing github.com — Should `asyncio.{Lock,Semaphore}.locked()` return True when there are waiters? · Issue #97028 · python/cpython
Should asyncio.{Lock,Semaphore}.locked() return True when there are waiters? #97028DescriptionCurrently 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. Activitykumaraditya303 commented on Sep 24, 2022 njsmith commented on Sep 24, 2022 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 commented on Sep 25, 2022
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 commented on Sep 25, 2022 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 commented on Sep 25, 2022 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 commented on Sep 25, 2022
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 commented on Sep 25, 2022 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 commented on Sep 25, 2022 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 commented on Sep 30, 2022 gvanrossum commented on Sep 30, 2022 Yes, it is, thanks for the reminder.
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|