DM-48576: Suppress some client warnings for async generators #460
Conversation
Python 3.13 adds a new warning when the `aclose` method of an async generator is not called explicitly. Async generators should ideally be explicitly shut down with `aclose` when they are no longer in use. Fix typing in some cases where async generators were returned but typed as async iterators (which don't require `aclose`), and close more generators explicitly. Include some async iterators from third party libraries that are actually generators, which requires working around the typing and some future-proofing for API changes. Ignore the remaining warning that's internal to httpx-sse and can't be fixed at the client level.
| async with aclosing(progress): | ||
| async with asyncio.timeout(30): |
There was a problem hiding this comment.
Just curious if the aclosing has to be outside the timeout, or if it doesn't matter.
There was a problem hiding this comment.
It shouldn't matter. It will close the generator in the finally block when exited by the exception raised by asyncio.timeout either way.
| # therefore is supposed to be explicitly closed with alcose() | ||
| # after break, unlike a true iterator. Handle this case to suppress | ||
| # warnings in Python 3.13. | ||
| with suppress(AttributeError): |
There was a problem hiding this comment.
Why do we have to handle AttributeErrors here (and elsewhere), instead of using aclosing?
There was a problem hiding this comment.
This is because the upstream typing says that they're returning an async iterator, but it's actually an async generator. Async iterators don't have aclose methods, and they could replace the generator with an iterator at any time and not break the interface guarantee. So we have to allow for the possibility that aclose doesn't exist, because it really is an iterator.
I've now refactored this into a helper function (which we could lift into Safir if we wanted to pursue this elsewhere) and added a bunch more documentation to make this clearer.
There was a problem hiding this comment.
Ohhhhhhh that's to suppress the case where aclose isn't there. I mistakenly thought it was to catch an AttributeError from the contents of aclose. This makes total sense.
There was a problem hiding this comment.
Oh, yeah, the suppress might be a bit too broadly scoped. I didn't think about that. I suppose I should actually use getattr just to be safer?
Move the code in the Nublado client to close upstream library async iterators that are actually generators into an `_aclosing_iter` helper class with the same interface as `contextlib.aclosing`. Use that class documentation to more thoroughly document why we're taking this approach.
Python 3.13 adds a new warning when the
aclosemethod of an async generator is not called explicitly. Async generators should ideally be explicitly shut down withaclosewhen they are no longer in use. Fix typing in some cases where async generators were returned but typed as async iterators (which don't requireaclose), and close more generators explicitly. Include some async iterators from third party libraries that are actually generators, which requires working around the typing and some future-proofing for API changes.Ignore the remaining warning that's internal to httpx-sse and can't be fixed at the client level.