Skip to content
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

Fixed inability to start tasks from async_generator_asend objects on asyncio #675

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

agronholm
Copy link
Owner

No description provided.

Copy link
Collaborator

@gschaffner gschaffner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think that this PR is buggy as-is because async_generator_asend does not support inspect.getcoroutinestate. for example, here's a branch adding a very similar second test that crashes: test_start_soon_from_asend_cancelled in 19b4e58...5cc5aba.

the simplest way to fix this would probably be to put an async def wrapper around the async_generator_asend (something like https://github.com/agronholm/anyio/pull/567/files#diff-202c3fc4551e5a287df488a9bd96a25dcc1405ec135d5c7aceb6ef4c02755341, except with ensure_returns_coro using inspect.iscoroutine instead of isinstance(awaitable, abc.Coroutine) or asyncio.iscoroutine(awaitable) so that it always returns an async def coroutine (which will be inspectable). for almost all calls, ensure_returns_coro(func)(*args, **kwargs)/ensure_returns_coro(func, *args, **kwargs)/whatever would just return func(*args, **kwargs). an async def wrapper would only need to be added when the user's callable actually returned a non-coroutine1 Awaitable such as an async_generator_asend.)

i also thought a bit more about whether it's possible to fix the async_generator_asend case by special-casing it while leaving the other non-coroutine awaitables unsupported still. (IIRC in past discussions you wanted to avoid doing wrapping for non-coroutine awaitables.) on Python >= 3.12, i initially thought that it might be possible to add a async_generator_asend getcoroutinestate special case instead of doing wrapping, but after some more thought i don't think it would actually work. here was the idea though, in case you want to try to figure out a way to make it work:

click to expand

PEP 525 says that an instance of PyAsyncGenASend holds a reference to its parent agen. i poked around a little bit and i do not think that that private API is exposed to Python code, but gc.get_referents can be used instead (obviously this is not ideal). then on Python >= 3.12, perhaps the new inspect.getasyncgenstate could be used; something like:

from __future__ import annotations

import gc
import inspect
import sys
from collections.abc import AsyncGenerator, Coroutine
from inspect import CORO_CLOSED, CORO_CREATED
from typing import Any, Literal

import anyio
import anyio.lowlevel

if sys.version_info >= (3, 12):
    from inspect import AGEN_CLOSED, AGEN_CREATED, AGEN_RUNNING, AGEN_SUSPENDED


# AFAICT there isn't a type exposed anywhere for PyAsyncGenASend implementations (e.g.
# CPython's _PyAsyncGenASend_Type), so for `isinstance` we'll just get the Python
# implementation's PyAsyncGenASend implementation at runtime.
async def _agenfunc() -> AsyncGenerator[None, None]:
    yield


PyAsyncGenASend_impl_type = type(_agenfunc().asend(None))


def getcoroutinestate(
    coroutine: Coroutine[Any, Any, Any],
) -> Literal["CORO_CREATED", "CORO_RUNNING", "CORO_SUSPENDED", "CORO_CLOSED"]:
    """
    Like `inspect.getcoroutinestate`, but also works for more coroutine-likes, including
    `PyAsyncGenASend` on Python >= 3.12.
    """

    if sys.version_info >= (3, 12) and isinstance(coroutine, PyAsyncGenASend_impl_type):
        agens = [
            obj
            for obj in gc.get_referents(coroutine)
            if isinstance(obj, AsyncGenerator)
        ]
        if len(agens) < 1:
            raise TypeError(
                "PyAsyncGenASend impl. is not compliant with PEP 525: it does not "
                "hold a reference back to the async generator"
            )
        if len(agens) != 1:
            raise RuntimeError(
                "PyAsyncGenASend is holding references to multiple async generators"
            )
        agen = agens[0]
        agen_state = inspect.getasyncgenstate(agen)
        return {
            # If a PyAsyncGenASend exists and the generator is just created, the
            # PyAsyncGenASend is just created.
            AGEN_CREATED: CORO_CREATED,
            # If a PyAsyncGenASend exists and the generator is running, we don't know if
            # this PyAsyncGenASend is a completed coroutine for a previous step or a
            # running coroutine for the running step.
            AGEN_RUNNING: ...,  # ???
            # If a PyAsyncGenASend exists and the generator is suspended, we don't know
            # if this PyAsyncGenASend is a completed coroutine for a previous step or a
            # created coroutine for the next step.
            AGEN_SUSPENDED: ...,  # ???
            # If the PyAsyncGenASend exists and the generator is closed, the
            # PyAsyncGenASend is closed (probably?).
            AGEN_CLOSED: CORO_CLOSED,
        }[agen_state]
    else:
        return inspect.getcoroutinestate(coroutine)


async def main() -> None:
    async def agenfunc() -> AsyncGenerator[None, None]:
        await anyio.lowlevel.checkpoint()
        yield
        await anyio.lowlevel.checkpoint()

    agen = _agenfunc()
    asend0 = agen.asend(None)
    print(getcoroutinestate(asend0))  # should be CORO_CREATED
    await asend0
    asend1 = agen.asend(None)
    print(getcoroutinestate(asend0))  # should be CORO_CLOSED
    print(getcoroutinestate(asend1))  # should be CORO_CREATED


anyio.run(main)

due to the ambiguity i haven't been able to get that working though.

Footnotes

  1. when i write "non-coroutine" here, i am using the inspect definition: it's not a coroutine/coroutine-like because it doesn't have any of the cr_{await,frame,running,code,origin} attributes that inspect says coroutines have. so it can't be used with inspect.

    there are many other definitions of "coroutine" and "coroutine-like" in different parts of the language spec. & stdlib though...

    each of the following uses a different definition of "coroutine-like":

    • PEP 525 says that "PyAsyncGenASend [(i.e. async_generator_asend on CPython)] [is a] coroutine-like object (implementing __iter__, __next__, send() and throw() methods)".

    • inspect.getcoroutinestate says that it "will accept any coroutine-like object that has cr_running and cr_frame attributes".

    notably, the "coroutine-like" PyAsyncGenASend in PEP 525 does not have cr_running or cr_frame, so its not a "coroutine-like" according to inspect.getcoroutinestate! this is true both in theory (i.e. PyAsyncGenASend) and in practice (i.e. with CPython's async_generator_asend implementation of PyAsyncGenASend).

    each of the following uses a different definition of "coroutine":

    • inspect.iscoroutine and isinstance(obj, types.CoroutineType) use an async def definition of coroutine.

      by its definition of coroutine, coroutines have cr_*.

    • isinstance(obj, abc.Coroutine) has a wider definition (which is a bit mutable since folks can use .register to add things).

      notably, its definition of coroutine does not require any cr_*

    • asyncio.iscoroutine seems to be the most permissive of the bunch. it says lots of things are coroutines, such as every generator on Python < 3.12:

      def foo():
          yield
      
      asyncio.iscoroutine(foo())
      # True on Python < 3.12. (N.B.: it was not changed to False in 3.11 when
      # generator-based coroutines were removed; it was actually changed to False in
      # 3.12)
      

      its definition also does not require any cr_*.

    there is also sometimes ambiguity because "sometimes coroutine means coroutine object but sometimes coroutine means coroutine function", but that's entirely unrelated.

@@ -741,7 +739,7 @@ def task_done(_task: asyncio.Task) -> None:
parent_id = id(self.cancel_scope._host_task)

coro = func(*args, **kwargs)
if not iscoroutine(coro):
if not isinstance(coro, Coroutine):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the above comment is about async_generator_asend specifically, but in general it seems buggy to replace inspect.iscoroutine here with either (a) abc.Coroutine.__instancecheck__ or (b) asyncio.iscoroutine.

the problem is that anything that AnyIO allows past this check needs to support inspect.getcoroutinestate (because of _task_started), but:

  • (a) and (b) would allow through coroutines/coroutine-likes that don't support getcoroutinestate, such as types that implement the abc.Coroutine interface but are not inspectable (e.g. async_generator_asend) and types that a user or a third-party library calls abc.Coroutine.register on.

  • (b) would also allow through "coroutine-likes" that aren't even coroutine-like, such as every generator on Python < 3.12.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the in-depth analysis. In this light, wrapping the async_generator_asend object in a coroutine might be less of a hassle. I need to consider my options here.

@gschaffner
Copy link
Collaborator

async_generator_asend does not support inspect.getcoroutinestate

it looks like this was filed upstream as python/cpython#81952.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants