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

Trio support. #169

Closed
tomchristie opened this issue Jul 30, 2018 · 25 comments
Closed

Trio support. #169

tomchristie opened this issue Jul 30, 2018 · 25 comments

Comments

@tomchristie
Copy link
Member

Revisiting some thoughts on this following on from #118

I'm not against having trio-support as one of the built-in options. Presumably that'd mean using --loop trio, and having that switch out:

  • The loop implementation.
  • The server class implementation.
  • Using a new trio h11 based protocol implementation for the default --http auto.

The asyncio/trio/curio split is problematic, but it's an ecosystem issue, rather than an ASGI issue (ASGI is just an async/await interface, and is agnostic what event loop implementation is used to call into that interface.)

The problem is that whatever event loop implementation you're running in must also match any async primitives used in the application codebase.

If you're just using async/await and send/receive, then it doesn't matter, but as soon as you start using things like asyncio.sleep()/trio.sleep(), or creating concurrent tasks, or accessing network or file I/O directly, then you're locked into whichever system you're in.

I'd say that Trio is indisputably more refined and more properly constrained than asyncio, but the ecosystem split is problematic.

@miracle2k
Copy link

miracle2k commented Jul 31, 2018

For the version I did in #118, rather than implementing a new protocol, I instead built a bridge between the asyncio protocol interface and trio.

Specifically, the reason trio (and curio) do not have a protocol class, as I understand, is that the asyncio protocol implementation is not fully async/await (https://vorpus.org/blog/some-thoughts-on-asynchronous-api-design-in-a-post-asyncawait-world/#review-and-summing-up-what-is-async-await-native-anyway). That is, both data_received and transport.send are sync-functions, not async, and as such do not communicate backpressure. Rather, in asyncio, you need to use pause_reading and pause_writing to handle backpressure.

So, no "asyncio protocols" in trio. But in asyncio proper, a server code that is inside asyncio is just reading from the socket and calling the Protocol.data_received class, and taking the data from transport.send and writing it to the socket. So my solution was to reproduce this server logic in trio, such that the existing protocol classes in uvicorn can be used.

This means that we lose the purported advantage of trio's "async/await all the way down" in some way, but I don't think this matters here - it might be a better developer experience in general, but it's not necessary to implement a correctly functioning server.

I packaged up this bridge code in https://github.com/miracle2k/trio-protocol.

So my long-winded point: What are your thoughts on this approach? Are you ok with an optional dependency on trio-protocol (maybe installable via uvicorn[trio])? Should we copy the code of trio-protocol to uvicorn? (it's not that much line-wise, though has enough complexity that it could benefit from bundling improvements and fixes.)

Finally, the trio code could forgo the existing code for h11 and httptools, but re-implement this stuff on top of trio without using the protocol interface, but... that seems like a lot of work with little benefit.

@tomchristie
Copy link
Member Author

This means that we lose the purported advantage of trio's "async/await all the way down" in some way, but I don't think this matters here - it might be a better developer experience in general, but it's not necessary to implement a correctly functioning server.

I don't think it matters at all to the end user in this case. We're correctly handling back-pressure, we're correctly handling fully-drained transports on graceful shutdown, we've done the extra legwork that a trio implementation might have helped with anyway, so as far as the developer is concerned there's just an async/await interface for them to develop against, and the specifics of the server implementation don't much matter, beyond which of asyncio/trio they ought to be using.

Are you ok with an optional dependency on trio-protocol

I'm okay with it, yup - I can't see any better option. One way or another we want to have an implementation that we share, and trio's streams are different enough in flow from asyncio's protocols that I can't see a better approach.

Since #118 we've now got some timeout behavior in there. I'd far prefer to be able to be using trio's more structured timeout "with" blocks. I can see how I'd do something like that for the request/response timeout easily enough, since it wraps the ASGI call, but I can't see how to make that work with the keep alive timeouts, which span between requests. I think the loop.call_later behavior might mean that #118 wouldn't now be working against master, and that it'd need adding to trio-protocol or something?

@tomchristie
Copy link
Member Author

I can see how we could restructure things slightly to avoid using loop.call_later if we think that'd be sensible.

@miracle2k
Copy link

My plan is to work on this sometime in the next week or so; I think we can make call_later work just fine.

@miracle2k miracle2k mentioned this issue Aug 1, 2018
@0alpha
Copy link

0alpha commented Apr 15, 2020

What's the current state of #169/#170?

@florimondmanca
Copy link
Member

florimondmanca commented Nov 8, 2020

I'm very keen on moving forward with this, here's my initial analysis looking at the current state of the code. :-)

Approach

We won't be able to take strictly the same approach as in HTTPX, i.e. just have an AsyncBackend interface and implement it for asyncio, trio, curio…

The reason is that server code uses more low-level APIs, in this case we use asyncio transports and protocols, and trio or curio simply don't have equivalents for that. So we'll need to duplicate some logic, and just have different implementations for each library.

That said, for some components (esp those at the ASGI layer rather than the networking layer), it wil make sense to be implemented using an AsyncBackend (e.g. Lifespan only uses events and queues, which can be abstracted away).

So we'll need both:

  1. A server implementation for each library
  2. An AsyncBackend interface for ASGI-level components (or other components that aren't strictly related to server networking).

Plan

First, we'll need to isolate any asyncio-specific code:

  • Move Server and ServerState from uvicorn/main.py to something like uvicorn/_impl/asyncio/server.py. Keep an alias in main.py for backwards compatibility.
  • Move uvicorn/protocols/to uvicorn/_impl/asyncio/protocols/. Keep shims in uvicorn/protocols/ for backwards compatibility.
  • Move uvicorn/loops/ to uvicorn/_impl/asyncio/loops/. Keep shims in uvicorn/loops/ for bacwkards compatibility. (It doesn't make sense to keep this at the top-level layer, since neither trio nor curio have a concept of "loop" whose implementation could be swapped out. So it really is asyncio-specific.)
  • Rework uvicorn/lifespan/on.py so it doesn't refer to asyncio directly. This means introducing uvicorn/_backends/{base,asyncio}.py, where base.py defines an AsyncBackend with .create_event() and .create_queue() methods, and asyncio.py implements that interface for asyncio. (We can revisit later whether a queue is really the ideal internal API to use, rather than something like trio's channel API, but one step at a time.)
  • Rework uvicorn/middleware/wsgi.py so it uses the AsyncBackend interface rather than calling into asyncio directly.
  • (uvicorn/workers.py contains the Gunicorn worker implementation, and contains a loop.run_until_complete() call. For now we can keep it like that. Later one we can rework it to allow using a different server class (eg when we have a TrioServer).)
  • Tests: if all is well, no tests should have to be changed, since those are reaching to public modules which this refactoring ought to keep available and unchanged.

These are the places where I'm seeing we do import asyncio right now.

Once those are isolated into uvicorn/_impl/asyncio, and ASGI-level components call into AsyncBackend, then we can start adding trio support by adding uvicorn/_impl/trio, and adding a TrioBackend(AsyncBackend) implementation.

@florimondmanca
Copy link
Member

florimondmanca commented Nov 22, 2020

I've done more investigation, and from what I found out it seems there may not be a safe, simple way to transition incrementally from the current state to an "async agnostic" state, at least without partial rewrites of lots of stuff.

For example, the "protocols" code (h11 and httptools for HTTP/1.1, wsproto for WebSocket) is very much entangled with asyncio (mostly because of the use of the transport/protocol API, rather than the streams API).

Additive rewrite?

So I'm exploring the space of what's possible on a branch: https://github.com/encode/uvicorn/tree/fm/async-agnostic

My approach there is to rewrite parts of Uvicorn (basically everything that touches networking, plus the lifespan code) independently of the existing code paths.

The rewritten code uses an AsyncBackend API (approach very similar to what we've done in HTTPCore).

The new code path can be taken by passing an --async-library ... flag. This selects the async backend on startup (and we can auto-detect it when inside the server code for our own convenience).

Current status

I'm quite happy to say that I've got a lot working on that branch yet:

  • HTTP/1.1 server implementation (no WebSocket yet) for h11 + httptools
  • asyncio + trio + curio 🎉

File structure (approximately):

_async_agnostic/
  # Async backends, similar to approach in HTTPCore. Provides an `AsyncSocket` abstraction, among other things
  backends/

  # HTTP/1.1 implementation
  http11/
    parsers/  # Sans-I/O parsers
      base.py  # Interface (inspired by `h11.Connection`)
      h11.py
      httptools.py
    handler.py  # Handler interface implementation.

  # Server implementation.
  # Relies on a "handler" interface: `async handler(sock, state, config) -> None`
  # This is similar to trio's and curio's server APIs, which expect
  # a `async handler(stream) -> None` function to be passed.
  # This allows preparing support for HTTP/2 (basically: add `http2/`, and a handler).
  # The server is responsible for "things around actually serving connections", such as
  # running the lifespan task, or listening for shutdown signals.
  # (This means handlers are testable in isolation.)
  server.py

The _async_agnostic directory lives within uvicorn/. When --async-library is given, the main run() function targets the new async-agnostic server's .run(), rather than the existing asyncio-only server. (That one is currently in _impl/asyncio.py, we may want to revert that and move it to _server.py, or similar.)

I also have a tests/async_agnostic directory, with relevant tests copied over and adapted using the new internal async-agnostic APIs. I'm happy to say that all tests pass. ✅

Is this madness?

Well, probably. :-)

Obviously we can't ever downright replace what's in Uvicorn right now with what I'm working on in that branch.

Also I'm aware the code I'm working on mixes two problems: async-agnosticity, and preparing for HTTP/2 support. It's very possible that preparing HTTP/2 support could be done separately. (And an internal "handler API" could be a sensible way to go. It should make it much easier to add HTTP/2 support incrementally, by simply adding an HTTP/2 handler, implemented with h2 for example.)

But as for the async-agnostic side of things, there may be a way forward in the form of a "new implementation" that exists alongside the existing one, and that users are able to opt into (using --async-library (asyncio|trio|curio)).

If we think this is an interesting path forward, we could go for something like this:

  1. Add new implementation, as a "opt-in beta". This can be released in a minor release, since it's purely additive.
  2. Let new implementation live on for a while. Add more to it (eg WebSocket support, which I think is the only thing missing in my branch).
  3. Once the new implementation seems ready enough, exit the "live beta" phase. Deprecate the older implementation, eg by marking the absence of --async-library deprecated.
  4. Make the switch by making the new implementation opt-out.

There are pain points, though:

  • We'll need to maintain two implementations. They look different, which may make back-porting bug fixes from one to the other non-trivial (not just copy pasting).
  • Introduces a bit of a burden on users — at least, we need to make sure the communication is efficient.

But…

  • The "live beta" phase is safe for users: no risk of breaking changes, since it's purely additive, and always opt-in.

Happy to read thoughts about this! I keep working on my branch since I think it's a valuable bunch of work anyway (I've already cumulatively spent a few days straight on it!), at least for figuring out what can be done and how.

(Also interesting to think about this wrt HTTPCore: I think we have a "someday" ambition to build a server-side minimal HTTP server implementation, compatible with sync/async, with support for various async libraries, various parsers, etc…)

@euri10
Copy link
Member

euri10 commented Nov 23, 2020

this is brillant and very elegantly written.
the only thing that worries me is the multiprocessing / reloader integration

@florimondmanca
Copy link
Member

florimondmanca commented Nov 23, 2020

the only thing that worries me is the multiprocessing / reloader integration

Actually it works very well. Well, at least, the tests pass! https://github.com/encode/uvicorn/blob/fm/async-agnostic/tests/async_agnostic/test_main.py I did have to figure out some gnarly issues (esp binding to a raw socket.socket from trio), but overall it's all just working great. The server shuts down when expected, starts up fine, even when using multiple workers… In fact I didn't have to change the reloader or multiprocessing code at all, since it's only relying on a run() function that's supposed to clean up everything after itself.

@euri10
Copy link
Member

euri10 commented Nov 23, 2020

well I looked very rapidly but :

❯ git checkout fm/async-agnostic
Switched to branch 'fm/async-agnostic'
Your branch is up to date with 'upstream/fm/async-agnostic'.
❯ uvicorn app:app --async-library asyncio --reload
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     Started reloader process [28731] using watchgod
Traceback (most recent call last):
  File "/home/lotso/PycharmProjects/uvicorn/venv/bin/uvicorn", line 33, in <module>
    sys.exit(load_entry_point('uvicorn', 'console_scripts', 'uvicorn')())
  File "/home/lotso/PycharmProjects/uvicorn/venv/lib/python3.8/site-packages/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/home/lotso/PycharmProjects/uvicorn/venv/lib/python3.8/site-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/home/lotso/PycharmProjects/uvicorn/venv/lib/python3.8/site-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/lotso/PycharmProjects/uvicorn/venv/lib/python3.8/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/home/lotso/PycharmProjects/uvicorn/uvicorn/main.py", line 369, in main
    run(**kwargs)
  File "/home/lotso/PycharmProjects/uvicorn/uvicorn/main.py", line 391, in run
    supervisor.run()
  File "/home/lotso/PycharmProjects/uvicorn/uvicorn/supervisors/basereload.py", line 34, in run
    self.startup()
  File "/home/lotso/PycharmProjects/uvicorn/uvicorn/supervisors/basereload.py", line 55, in startup
    self.process.start()
  File "/home/lotso/.asdf/installs/python/3.8.6/lib/python3.8/multiprocessing/process.py", line 121, in start
    self._popen = self._Popen(self)
  File "/home/lotso/.asdf/installs/python/3.8.6/lib/python3.8/multiprocessing/context.py", line 284, in _Popen
    return Popen(process_obj)
  File "/home/lotso/.asdf/installs/python/3.8.6/lib/python3.8/multiprocessing/popen_spawn_posix.py", line 32, in __init__
    super().__init__(process_obj)
  File "/home/lotso/.asdf/installs/python/3.8.6/lib/python3.8/multiprocessing/popen_fork.py", line 19, in __init__
    self._launch(process_obj)
  File "/home/lotso/.asdf/installs/python/3.8.6/lib/python3.8/multiprocessing/popen_spawn_posix.py", line 47, in _launch
    reduction.dump(process_obj, fp)
  File "/home/lotso/.asdf/installs/python/3.8.6/lib/python3.8/multiprocessing/reduction.py", line 60, in dump
    ForkingPickler(file, protocol).dump(obj)
TypeError: cannot pickle '_thread.lock' object

which incidentally is funny because I've got kind of the same pickle issue but on windows only in my attempt to deal with multiprocessing better in #853

@florimondmanca
Copy link
Member

florimondmanca commented Nov 23, 2020

windows

Okay, I revert: I meant to say "It works very well on my machine" (macOS). 😄

@euri10
Copy link
Member

euri10 commented Nov 23, 2020

windows

Okay, I revert: I meant to say "It works very well on my machine" (macOS).

this is linux where you see this stacktrace, the windows issue I'm mentioning is another story from the PR I linked above.

anyway, we can talk about it in another thread but the whole Multiprocess class is currently unable to handle properly the various spawned servers mostly because it uses a threading.Event lock which will block any gentle shutdown when trying to join process.

You can then add to that the fact that signals in the multiprocess case should be dealt with differently (ie ignore SIGINT first mostly then start the processes then registering signals) than in a the single runner case where you just have to put a signal on the loop, and that multiprocessing context (spawn vs fork) is different from os to os and you see the issue is complex.

I'm mentioning the 2 points above because it has deep impacts on the way the run method of the Server (the one from Process(target=run) ) should handle shutdown, its logic to handle signals, which should be different in multiprocess / single runner cases.

the approach taken in #853 works fine on linux, hangs on windows because it can't pickle stuff (I think the ssl context but not sure) and I didnt test on macos.

@tomchristie
Copy link
Member Author

One of the issues here is that adding trio support gracefully would involve some changes that negatively impact the micro-benchmarking performance of uvicorn.

The server handling isn't super reusable across differing frameworks, but that's partly because it's geared towards "performance". It's a bit of a nonsense, but at the same time it's important that we're adequately demonstrating that ASGI servers/frameworks are equally as fast as tightly-coupled alternatives.

It might be that if we want to be able to preserve that, then we just need to suck up that there will be duplication, and have a fast path for asyncio-specifically, and a neat path for wider ecosystem support.

@kontsaki
Copy link

hello, is AnyIO something that could help pushing this forward?

@florimondmanca
Copy link
Member

@kontsaki We know about AnyIO yup, thanks for the suggestion. From my POV at this point this is more of a change management issue (how to get this in incrementally, in a way that's low risk for existing users, and easy enough to review by maintainers and contributors, ...), rather than a technical one. We've already achieved this async agnosticism in HTTPX, we know it's possible and that there are various technical approaches, but we "just" have to do put in the work. :) It's a tad harder than in HTTPX because of how Uvicorn already supports a wide amount of users; we're not starting from a clean slate.

@uSpike
Copy link
Member

uSpike commented Apr 24, 2021

The asyncio/trio/curio split is problematic, but it's an ecosystem issue, rather than an ASGI issue (ASGI is just an async/await interface, and is agnostic what event loop implementation is used to call into that interface.)

Is supporting structured concurrency (Trio) an ASGI issue? For instance, if I want to run a background task during the lifespan of my application, there's no way to do that (AFAICT) conforming to structured concurrency principles. A simple example using starlette:

from starlette import Starlette

async def lifespan(app):
    async with trio.open_nursery() as nursery:
        nursery.start_soon(some_background_task)
        yield
        
app = Starlette(lifespan=lifespan)

Why this is bad is well explained here: https://anyio.readthedocs.io/en/stable/cancellation.html#avoiding-cancel-scope-stack-corruption. TL;DR:

The problem with this code is that it violates structural concurrency: what happens if the spawned task raises an exception? The host task would be cancelled as a result, but the host task might be long gone by the time that happens. Even if it weren’t, any enclosing try...except in the generator would not be triggered. In other words: it’s a Really Bad Idea to do this!

@njsmith
Copy link

njsmith commented Apr 26, 2021

Many web apps do probably want a way to start background work that outlives a single request handler. If that's what your design needs, then structured concurrency doesn't object, it just wants you to be explicit about lifetimes :-)

Quart is an ASGI framework, and it handles this using standard ASGI features. Specifically, it uses a "lifespan" handler to start up a nursery at startup that stays open as long as the app is running, and make it available to user code as an attribute on their app object:

https://gitlab.com/pgjones/quart-trio/-/blob/bd451f23672704836e57e8e899d280fe879380e2/src/quart_trio/asgi.py#L98

@tomchristie
Copy link
Member Author

tomchristie commented Apr 26, 2021

Is supporting structured concurrency (Trio) an ASGI issue?

No.

A simple example using starlette

Ah, that yield caveat is interesting, thanks. Hadn't seen that pointed out before. (*)

Starlette could perfectly well tweak things around there to ensure there's a nursery available for the duration of the app lifespan, in the same way that Quart already does, yup.

(*) I needed to read up on this a bit more, and found the Trio docs helpful so I'm going to reference them here for other potential readers: https://trio.readthedocs.io/en/stable/reference-core.html#cancel-scopes-and-nurseries

@uSpike
Copy link
Member

uSpike commented Apr 26, 2021

Thanks @tomchristie and @njsmith . I'm able to use anyio with starlette/fastapi with the following modification for background tasks:

class TaskGroupFastAPI(FastAPI):
    async def __call__(self, scope, receive, send):
        async with anyio.create_task_group() as task_group:
            self.task_group = task_group
            await super().__call__(scope, receive, send)

app = TaskGroupFastAPI()

async def manager_task(shutdown_event):
    async with manager:
        await shutdown_event.wait()

async def lifespan(app):
    shutdown_event = anyio.create_event()
    await app.task_group.spawn(manager_task, shutdown_event)
    try:
        yield
    finally:
        await shutdown_event.set()

# XXX: Better support lifespan context
# https://github.com/tiangolo/fastapi/issues/2943
app.router.lifespan_context = lifespan

Seems like a reasonable way forward.

@Kludex Kludex added this to the Version 1.0 milestone Jun 26, 2021
@graingert
Copy link
Member

graingert commented Aug 15, 2021

Many web apps do probably want a way to start background work that outlives a single request handler. If that's what your design needs, then structured concurrency doesn't object, it just wants you to be explicit about lifetimes :-)

Quart is an ASGI framework, and it handles this using standard ASGI features. Specifically, it uses a "lifespan" handler to start up a nursery at startup that stays open as long as the app is running, and make it available to user code as an attribute on their app object:

gitlab.com/pgjones/quart-trio/-/blob/bd451f23672704836e57e8e899d280fe879380e2/src/quart_trio/asgi.py#L98

Starlette currently does the right thing with yield and lifespan to support task groups, however quart and uvicorn do not. I'm currently looking at scrapping lifespan all together in favour of a much simpler "yield safe" api: #1152

@graingert
Copy link
Member

Regarding trio support in uvicorn, currently anyio is installed in CI so I think the first step is to swap pytest-asyncio for anyio and start moving more and more stuff from the tests out to use anyio async functions. @Kludex do you want me to start a PR on this?

@florimondmanca
Copy link
Member

to use anyio async functions.

@graingert Let's remember from past discussions that we want to keep a fast-track code path for asyncio, as Uvicorn is also meant to be a flagship for a fast and lightweight async server. This means keeping the existing asyncio-optimized code, and so I don't think we should make anyio required like we did in Starlette (where I think we could have done without anyio magic too for the sake of future maintenance, but I didn't participate in that decision).

I had also laid out ways we could move forward without anyio for trio support. I'd understand the motivation for using that as a first step for trio support, but this is just a note that I hope it is clear that we really do want to keep the existing asyncio code in place.

@tomchristie
Copy link
Member Author

Closing this off for now with the same rationale as #47

For now I think the best option is us helping promote Hypercorn as a trio-capable ASGI web server, rather than also trying to fill that spot with this project.

@davidbrochart
Copy link

FYI I started anycorn which is a fork of Hypercorn that uses AnyIO instead of having a separate code base for asyncio and Trio.
Hypercorn/asyncio seems to be around 10% faster than anycorn/asyncio (through anyio) (see davidbrochart/anycorn#12).

@tomchristie
Copy link
Member Author

@davidbrochart Interesting, thanks. You've also prompted me to revisit this...

Is supporting structured concurrency (Trio) an ASGI issue?

Somewhat.

The WSGI-like messaging API isn't fundamentally incompatible, but doesn't neatly match up to how you'd design a request/response + lifespans SC API.

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

No branches or pull requests