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

anyio integration #1157

Merged
merged 71 commits into from Jun 18, 2021
Merged

anyio integration #1157

merged 71 commits into from Jun 18, 2021

Conversation

uSpike
Copy link
Member

@uSpike uSpike commented Mar 26, 2021

Fixes: #811

This adds support for asyncio and trio by leveraging anyio and a little more structured concurrency in spots.

Things to do yet:

  • 100% tests passing (there's one failing test at the moment)
  • TestClient rewritten for anyio, probably with portals.
  • anyio 3.0 release

This adds a hard dependency for starlette, which didn't exist before, and was a selling point. In my opinion, the benefit is worth it. It would be great to use starlette with either uvicorn or hypercorn using Trio.

This also removes the need for aiofiles which is gained for free with anyio.

Some drawbacks are that the other integrations such as graphql are only currently compatible with asyncio. But, this does not make those integrations incompatible. It does give users the ability to use trio without them, and maybe they can be ported in the future as well.

I created a branch for framework benchmarks here: https://github.com/uSpike/FrameworkBenchmarks/tree/starlette-anyio

See results: https://www.techempower.com/benchmarks/#section=test&shareid=c92a1338-0058-486c-9e47-c92ec4710b6a&hw=ph&test=query&a=2

There is a performance penalty to using anyio. The weighted composite score is 173 vs 165, so a 4.5% drop in performance.

@uSpike uSpike mentioned this pull request Mar 26, 2021
README.md Outdated Show resolved Hide resolved
@uSpike
Copy link
Member Author

uSpike commented Mar 27, 2021

There's a weird bit of code in starlette/requests.py:

            async with anyio.move_on_after(
                0.001
            ):  # XXX: too small of a deadline and this blocks
                message = await self._receive()

The existing code used asyncio.wait_for(..., 0.00000001) or some really small number. I found that with anyio.move_on_after I needed a larger number for the deadline or else it would block. I'm not sure of a better way to handle that.

EDIT: refactored to an event in 4dd8c5d

@agronholm
Copy link
Contributor

Is there/does there need to be an anyio equivalent to asyncio.iscoroutinefunction()?

Not really, no. Trio does not do any tricks in that regard. As I understand, starting with Python 3.10 even asyncio doesn't need that.

@uSpike
Copy link
Member Author

uSpike commented Mar 27, 2021

03e312e added a chunk of work on testclient. On my machine, the tests finish but hang with a number of threads blocking. Not sure what's up there...

EDIT: not a problem anymore

@agronholm
Copy link
Contributor

My recommendation is to base this work on AnyIO 3.0, of which the first RC release is imminent. Since this is a new feature, I don't think backwards compatibility is necessary.

@agronholm
Copy link
Contributor

On that note, the 3.0.0rc1 pre-release is out now.

Copy link
Contributor

@agronholm agronholm left a comment

Choose a reason for hiding this comment

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

These should get you going on AnyIO 3. I had at least one question among the suggestions too.

setup.py Outdated Show resolved Hide resolved
starlette/concurrency.py Outdated Show resolved Hide resolved
starlette/concurrency.py Outdated Show resolved Hide resolved
starlette/middleware/base.py Outdated Show resolved Hide resolved
starlette/middleware/wsgi.py Outdated Show resolved Hide resolved
starlette/requests.py Outdated Show resolved Hide resolved
starlette/testclient.py Outdated Show resolved Hide resolved
starlette/testclient.py Outdated Show resolved Hide resolved
starlette/testclient.py Outdated Show resolved Hide resolved
starlette/testclient.py Outdated Show resolved Hide resolved
@uSpike
Copy link
Member Author

uSpike commented Mar 27, 2021

I've pushed changes to use anyio 3.0.0rc1, thanks @agronholm !

There's some failing tests left:

  • test_state_data_across_multiple_middlewares
  • test_wsgi_get
  • test_wsgi_post
  • test_wsgi_exc_info

Those tests seem to not be getting data in response.text from Testclient.get()

There's another test I've had to skip because it blocks forever: test_graphql_async_old_style_executor, need to investigate that one a bit.

@agronholm
Copy link
Contributor

Those test failures don't look too bad. Let me know if you need more help. You can catch me on Gitter.

@uSpike
Copy link
Member Author

uSpike commented Mar 27, 2021

starlette/graphql.py:GraphQLApp.__init__ states:

        else:
            # Old style. Use 'executor'.
            # We should remove this in the next median/major version bump.
            self.executor = executor
            self.executor_class = None
            self.is_async = isinstance(executor, AsyncioExecutor)

The test for this is blocking forever. I'd rather not spend too much effort debugging that, since it's slated to be removed. I will remove it assuming that this PR would bring about a median/major version bump.

@JayH5 JayH5 mentioned this pull request Jun 18, 2021
5 tasks
Copy link
Member

@JayH5 JayH5 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 it's time 🚀

@JayH5 JayH5 merged commit 42592d6 into encode:master Jun 18, 2021
JayH5 added a commit that referenced this pull request Jun 18, 2021
@uSpike
Copy link
Member Author

uSpike commented Jun 18, 2021

Cheers @JayH5 and thank you everyone for your help/work on this. I think this is an important step in bringing structured concurrency to the mainstream and I'm grateful for the opportunity to contribute. ❤️

@uSpike uSpike deleted the anyio branch June 18, 2021 15:32
JayH5 added a commit that referenced this pull request Jun 18, 2021
@agronholm
Copy link
Contributor

Just one question: how soon can we expect to see this in a release?

@graingert
Copy link
Member

@agronholm I think it's all but released now: #1202

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.

Trio support.
7 participants