-
-
Notifications
You must be signed in to change notification settings - Fork 931
Ensured explicit closing of async generators #3593
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
base: master
Are you sure you want to change the base?
Conversation
Note that this depends on another PR: #3592. I will mark as ready to review once that one has been merged. |
# Conflicts: # CHANGELOG.md
7ff0d5e
to
7b2b429
Compare
I checked locally with Python 3.10 that the supposedly missing coverage cannot be reproduced. I'm seeing 100% coverage, even without the |
Never mind, I was on the wrong branch. Fixing. |
@Kludex All good now. |
@@ -99,9 +102,8 @@ class UnattachedStream(AsyncByteStream, SyncByteStream): | |||
def __iter__(self) -> Iterator[bytes]: | |||
raise StreamClosed() | |||
|
|||
async def __aiter__(self) -> AsyncIterator[bytes]: | |||
def __aiter__(self) -> NoReturn: |
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.
Why is the async removed?
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.
It's not really necessary here. __aiter__()
can return any arbitrary object that supports the AsyncIterator
protocol, and the implementation here just raises NotImplementedError
.
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.
@tomchristie this is not an important part of this PR so if you object, I can just revert it.
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.
On another note, is either one of you at EuroPython?
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.
On another note, is either one of you at EuroPython?
I'm not. 😞
|
||
|
||
@asynccontextmanager | ||
async def safe_async_iterate( |
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.
Interesting. I'm curious about this vs. aclosing
.
https://docs.python.org/3/library/contextlib.html#contextlib.aclosing
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 originally went with aclosing()
but it doesn't work very well with arbitrary async iterables.
@@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). | |||
|
|||
## [UNRELEASED] | |||
|
|||
### Fixed | |||
|
|||
* Explicitly close all async generators to ensure predictable behavior |
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.
👍🏼 Neat.
Summary
This change ensures that async generators are always explicitly closed, even if the task running the generator is cancelled or another exception is raised. This prevents unpredictable behavior and also avoids the asyncgen finalizer warnings on Trio.
Checklist