Clarifications of ASYNC119 scope and details

[RomanValov]

The issue is considered to be a follow up of discussion at #238.

0. The rules page states ( https://flake8-async.readthedocs.io/en/stable/rules.html#async119 ):

   yield in context manager in async generator is unsafe, the cleanup may be delayed until await is no longer allowed

   But the rule is triggered for sync context managers too. But sync CMs can't have any awaits inside. So the description is confusing.

   FTR, error message displayed by flake8 is:

   Yield in contextmanager in async generator might not trigger cleanup. Use @asynccontextmanager or refactor.

1. As per comments in original pull request, there are basically two distinct issues / leves of concern mixed into the issue:

   a. Generator cleanup may be performed by garbage collector and may not be performed promptly.
   b. Generator cleanup may be performed in context where coroutine runner is not available and awaits are doomed to be skipped/raise errors.

   IMO these two cases should be split into distinct error because case a is less severe than case b. It may be desired to "ignore" them independently. Moreover as demonstrated by snippet in my comment, case a applies to purely sync code with sync generators/sync context managers. So maybe such rule should be pushed towards more generic linters, i.e. flake8 on its own.

2. Back to statement that generator cleanup may run in context where coroutine runner is not available. As per my understanding it should not happen with asyncio due to shutdown asyncgens machinery. Well, at least if asyncio.run is used. It would be nice to have a demonstrating snippet and clarify conditions under which it could happen (i.e. use trio, use pypy, etc.). So we have reproducible example everyone could check and developers may decide if they should bother for their case. Snippets posted by Zac and me on original pull request discussion both demonstrate case a, but not case b. In case issue only applies to non-asyncio libraries, the rule could be adjusted like it was done with ASYNC102.

3. Last but not least. In my code base I have snippets like:

   from contextlib import suppress
   
   async def iterate():
     with suppress(SomeError):
       yield
       yield

   I think the snippet is perfectly safe (even omitting my other concerns), because implementation of suppress.__exit__ only inspects exception in case it was thrown and do nothing otherwise. However it seems there is no way to instruct flake8-async about it other then adding "noqa" directives. There is --transform-async-generator-decorators but it's too broad for my case because it marks overall generator as safe, but not exact context manager used inside generator.

[RomanValov]

CC @alicederyn

[alicederyn]

Worth noting that delaying cleanup to the garbage collector can be a problem even for synchronous code, as you may be relying on running on a single thread for the duration of the generator. I agree that that's not the purview of this linter though. Excluding synchronous context managers that cannot await makes sense to me.

[jakkdl]

You can get a delayed await despite only using sync CM in an asyncgen with the use of except/finally:

import contextlib, itertools, trio

@contextlib.contextmanager
def hold_some_resource(n: int):
    print(f"acquire {n=}")
    try:
        yield
    finally:
        print(f"release {n=}")

async def loop_with_resource(n: int):
    try:
        with hold_some_resource(n):
            yield
            yield
    finally:
        # there could be an await here
        print("end agen", n)

@trio.run
async def main():
    for n in range(3):
        async for _ in loop_with_resource(n):
            if n == 0:
                break

It's possible we might want special logic, or a separate rule, for those to reduce false alarms.

I don't have a repro with the runner being stopped, that would indeed be great.

[alicederyn]

That await isn't associated with the context manager though, and it would be flagged up separately because we check for await in finally blocks. There's no way to "hide" an await within a synchronous context manager.

[jakkdl]

That await isn't associated with the context manager though, and it would be flagged up separately because we check for await in finally blocks. There's no way to "hide" an await within a synchronous context manager.

if it's shielded it (correctly) won't get flagged by ASYNC102

[alicederyn]

Is this some unique feature of trio? I can't find any reference to it in the documentation, and in regular async python there is no wait to yield to the event loop without an await or an async for. Could you link to details? If this is a feature of trio, then there should be some way to "opt in" to trio-specific linter errors -- this should not be flagged up in regular asyncio. I tried searching their documentation for "shield" but there was no suggestion that it could cause regular code to lose control to the event loop outside of recognized async keywords, and I'm not 100% sure how that would work mechanically within cpython.

[jakkdl]

yeah sorry, I'll clarify a bit more:

ASYNC102 (await-in-finally-or-canceled) no longer triggers for asyncio since #393, since the edge cancellation semantics means that await is sometimes fine and necessary in except/finally.
For trio, with its level cancellation semantics (i.e. once canceled every subsequent await will raise Cancelled) it's never fine to await in finally/except Cancelled unless you shield it from outside cancellations:

try:
    ...
finally:
    with trio.CancelScope(shield=True):
        await my_cleanup_function()  # perfectly fine, does not trigger ASYNC102

So we cannot rely on warnings for await in finally/except for warning about this entirely separate problem where the async gen can be stuck inside the try and not enter the except/finally until much later - where the run loop theoretically may no longer be running.

But ASYNC119 would be somewhat misleading here, triggering on the CM and not on the await in the finally, so we should probably add some more logic to ASYNC119 for this case.

[jakkdl]

Okay, after some digging I realized that the trio docs have all the answers I was looking for: https://trio.readthedocs.io/en/stable/reference-core.html#notes-on-async-generators

2. Back to statement that generator cleanup may run in context where coroutine runner is not available. As per my understanding it should not happen with asyncio due to shutdown asyncgens machinery. Well, at least if asyncio.run is used. [...]

I have not looked closely at asyncio, but it appears to be the same with trio nowadays. It has checks to make sure that the runner will clean up all async gens before fully exiting:

• If an async generator is still alive at the end of the whole call to trio.run(), then it will be cleaned up after all tasks have exited and before trio.run() returns. Since the “system nursery” has already been closed at this point, Trio isn’t able to support any new calls to trio.lowlevel.spawn_system_task().

And lists several other potential problems with having non-prompt cleanup, and I suspect that asyncio shares several of those:

• The cleanup of the generator occurs in a cancelled context, i.e., all blocking calls executed during cleanup will raise Cancelled. This is to compensate for the fact that any timeouts surrounding the original use of the generator have been long since forgotten.
• The cleanup runs without access to any context variables that may have been present when the generator was originally being used.
• If the generator raises an exception during cleanup, then it’s printed to the trio.async_generator_errors logger and otherwise ignored.

But those are also pretty much the same ones as you can get with a fully sync thing. I still think there's a pretty good case for keeping the rule around simply because structured concurrency tries very hard to maintain expected behavior everywhere - and most users would expect their async gens to be promptly cleaned up within the context where they're used.
But if there was a linter out there that had a good rule covering both sync+async cases I wouldn't mind dropping async119.

The documentation definitely warrants a rewrite though ("yield in context manager in async generator is unsafe, the cleanup may be delayed until await is no longer allowed" is misleading at best).

And I no longer think we need a logic rewrite of async119, even non-finally+await cases with non-prompt gc is worth warning about imo.