Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-111693: Propagate correct asyncio.CancelledError instance out of asyncio.Condition.wait() #111694
GH-111693: Propagate correct asyncio.CancelledError instance out of asyncio.Condition.wait() #111694
Changes from 5 commits
c3aff47
111e74a
6d41771
4e16fa7
ad56f29
30e0ec4
6d59820
c060f40
62bd6cd
01046c5
8802dfa
f7c103c
1572b2b
dce420e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not very comfortable with widening this exception net for the purpose of sneaking in support for your library. If we want that to be supported we should make it an explicit feature, everywhere, rather than just tweaking an
except
clause here and there until your tests pass. Without any comments explaining the intended guarantee, what's to stop the next clever maintainer from narrowing the exception being caught toCancelledError
again?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. I quite understand that you are reluctant to touch this just to humour an eccentric experimental library which is trying to push the envelope of the original design. And I'm happy to have this rejected as long as we have had the chance to discuss it. I'll add a comment here in the mean time, just for safety.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC you're planning to make your library to use a subclass of
CancelledError
, so you don't need this to be more general anyway. So I'd like to seeCancelledError
here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Of course, IMO, it would be even greater if in some future version, we would have something like:
but that is future music :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment makes things more mysterious to me, not less. If we're precise, we get to this exception handles whenever
await fut
raised something -- that can currently only be a cancellation, unless something reached into private data, extracted the future, and calledset_exception()
on it. (Apparently this is what you're planning to do in your own library?)If I had to explain in plain English what was going on here, I'd have to say something like "The acquiring task was cancelled after the lock was released, but before it resumed execution. (This is one of these things that's easy to forget is possible when casually reading async code.) We're going to re-raise the cancellation here, but we should first wake up the next task waiting for the lock, if there are any." I guess you summarized the condition as "after release() was called", which I think is too concise.
Thinking all this over, I am also thinking there's an incredibly subtle invariant going on here regarding
self._waiters
. Skimming the code ofrelease()
and_wake_up_first()
one starts wondering, "what if two or more waiters are cancelled, the first waiter wakes up (being cancelled), and its_wake_up_first()
sees that the second waiter is also cancelled (and hence done), so it doesn't wake up the third waiter." My guess is that in this case the second waiter will also wake up and call_wake_up_first()
, at which point the (originally) third waiter will be woken up. But maybe this deserves a reassuring comment somewhere?Maybe there's a way of thinking about this that makes it easier to prove to oneself that the code here is correct, without such a "global" analysis? Maybe the invariant is something like "every cancelled future in the list corresponds to a task that will eventually be woken up, at which point it will remove itself from the list and call
_wake_up_first()
. Although that immediately makes me wonder if it is possible for the second waiter to be woken up first. Fortunately in that case things also work out, once the first waiter is also woken up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your thoughts. Yes, the comment was too concise and your version is better, but thinking more about it, it is not really true either. This here is not a case of "cancelled after lock was released". this is because release() does not do any modification of the _waiters queue itself. All it does is
self._locked=False
I go over the self._waiters in a comment below. Here is how I follow the logic:
_wake_up_first()
always wakes up the same Task, at the head of the queue.self._locked()
is False, it is always safe to call_wake_up_first()
, even multiple times.Looking at the case you mention:
self._waiters
queue.self._waiters
. Decides it doesn't want the lock, sees thatself._locked
is False (no one else has it), and so calls `self._wake_up_first(). This is a no-op, since the future has already been cancelled.self._waiters
. Decides it doesn't want the lock, seelf thatself._locked
is False, callsself._wake_up_first
.Not sure how to formulate that into an invariant, but something like this:
In other words, if the lock is not claimed, then the head of the queue, if any, should no longer be blocked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I look at this more carefully, I think I like the original version using nested
try
blocks better. Reading the new code I have to correlate the two separate removals to prove to myself that as soon as we've awaitedfut
(whether it got a return value or was cancelled) it is removed from the list of waiters. And that's why it was written like that originally.(Note that
try
blocks cause no overhead unless an exception is actually raised -- the compiler even duplicates thefinally
block to ensure this.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I guess it makes it simpler to reason about, good to know that nested blocks have no overhead anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing as before -- I actually like the nested
try
/finally
phrasing better. Also, again, sneaky about catching all errors.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, especially if it costs nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please convert the docstrings on the tests to comments -- test docstrings tend to be printed by the test runner, messing up the neat output. Note how no other tests here have docstrings.