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

Removed check for coroutine in asyncio backend #762

Closed
wants to merge 3 commits into from

Conversation

davidbrochart
Copy link
Contributor

Changes

See #759 (reply in thread).
In the asyncio backend, there is no need to check if the object is a coroutine before launching it as a task:

  • asyncio already does it,
  • it prevents launching a class instance that looks like a coroutine, for instance:
import typing

import anyio

class Foo(typing.Coroutine):
    def __await__(self):
        return self.run().__await__()

    def send(self, value=None):
        return self.__await__().send(value)

    def throw(self, *_unused):
        raise RuntimeError("Shouldn't happen")

    async def run(self):
        print("run")

async def main():
    async with anyio.create_task_group() as tg:
        tg.start_soon(Foo)

anyio.run(main, backend="trio")  # works fine
anyio.run(main, backend="asyncio")  # doesn't work

This already works with the Trio backend, but currently not with the asyncio backend.

Checklist

If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):

  • You've added tests (in tests/) added which would fail without your patch
  • You've updated the documentation (in docs/, in case of behavior changes or new
    features)
  • You've added a new changelog entry (in docs/versionhistory.rst).

If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.

Updating the changelog

If there are no entries after the last release, use **UNRELEASED** as the version.
If, say, your patch fixes issue #123, the entry should look like this:

* Fix big bad boo-boo in task groups (#123 <https://github.com/agronholm/anyio/issues/123>_; PR by @yourgithubaccount)

If there's no issue linked, just link to your pull request instead by updating the
changelog after you've created the PR.

@davidbrochart
Copy link
Contributor Author

@agronholm What do you think about this?

@agronholm
Copy link
Owner

Frankly I considered the inability to run generator-based coroutine functions a feature rather than a limitation. The main problem with this, however, is that I don't know what ends up a the coro of a task, and we can't accept just anything there. Maybe it works, maybe it won't, but you haven't added any tests to that end so it's hard to say. The problem with the coro of a task is outlined in a review comment of my own PR (#675).

Furthermore, if we go through with this and flat out disable such checks, we may get inconsistent or misleading error messages across backends when a user tries to pass an incompatible object as the callable to anyio.run().

@smurfix
Copy link
Collaborator

smurfix commented Jul 19, 2024

Well. We could fix this by replacing anyio's iscoroutine test with isinstance(…, typing.Coroutine).

@davidbrochart
Copy link
Contributor Author

Thanks @agronholm, I didn't see #675.
In particular, the following argument makes sense: "the problem is that anything that AnyIO allows past this check needs to support inspect.getcoroutinestate (because of _task_started)". But then, since the code at the top comment works with the Trio backend, I'm wondering if it should?

@agronholm
Copy link
Owner

Thanks @agronholm, I didn't see #675. In particular, the following argument makes sense: "the problem is that anything that AnyIO allows past this check needs to support inspect.getcoroutinestate (because of _task_started)". But then, since the code at the top comment works with the Trio backend, I'm wondering if it should?

The question is, where do we draw the line, and how? I don't know just what isinstance(coro, Coroutine) will pass through. Does it guarantee that the object works with inspect.getcoroutinestate()? I have a faint recollection that I've experimented with alternate checks in the past, but without much success.

@davidbrochart
Copy link
Contributor Author

davidbrochart commented Jul 20, 2024

Does it guarantee that the object works with inspect.getcoroutinestate()?

No it doesn't. But then how can Trio accept such coroutine-like objects? It must have some other way of handling their state.
Anyway, this seems to be a limitation of the asyncio backend and indeed such objects shouldn't be accepted.

@graingert
Copy link
Collaborator

No it doesn't. But then how can Trio accept such coroutine-like objects? It must have some other way of handling their state.

trio works by checking frame = getattr(coro, "cr_frame", None) and adding a wrapper coroutine if frame is missing

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.

4 participants