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 8 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.
Nice comment! It doesn't immediately alleviate my fear that the future now at the head is already cancelled, and nothing will be woken up. The explanation is, of course, that all cancelled tasks in the waiters list are already woken up and are just waiting to run. Maybe you could add a hint about that?
(And maybe reflow the text, pretty please? In comments, you have to do the rendering in your head. :-)
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'd remove that -- I don't think there's such a thing as
fut.true()
and the semantics ofset_result()
ensure that if the future was cancelled it will raise an exception.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.
(Ditto if it already had another result, or an exception.)
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.
Right, I added that as an explanation to what the state ought to be, to mirror the other test we make, but it is really redundant (and incorrect).
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.