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

Add a trio-based worker. #118

Closed
wants to merge 1 commit into from
Closed

Conversation

miracle2k
Copy link

This is definitely experimental at this point. Just wanted to show this off, and ask if this is something that makes sense as part of uvicorn. It could well be a separate package which imports the uvicorn protocol classes instead. On the other hand, uvicorn is currently a small-enough package that I could see it expand it's scope a little bit.

@tomchristie
Copy link
Member

Interesting. Note that the protocol class uses asyncio.get_event_loop() and loop.create_task() which I assume aren’t working as expected in this context?

@tomchristie
Copy link
Member

I guess you could fix that with an adapter that takes the nursery, and exposes a create_task(...) method that actually calls nursery.start_soon(...).

Then include this to the protocol class arguments: ‘loop=NuseryAdapter(nursery)’, right?

@miracle2k
Copy link
Author

The way I am doing this is by implementing the interface of the Protocol and Transport classes:

https://github.com/miracle2k/trio-protocol is a dependency.

Again, this is all very experimental for now, I just hacked it together in the last 3 hours or so. However, it does serve simple requests!

@miracle2k
Copy link
Author

FWIW, it does pass in a custom loop object that implements create_task exactly as you say, but that alone wouldn't be enough, because we still have to deal with the fact that the protocol wants to call, say, transport.write() and expects data to be written.

@tomchristie
Copy link
Member

Nice!

@tomchristie
Copy link
Member

we still have to deal with the fact that the protocol wants to call, say, transport.write() and expects data to be written.

I didn’t understand this point. Does trio expose a differing transport interface?

@miracle2k
Copy link
Author

Both trio and curio do not have protocols and transports - as far as I understand, that is sort of their point. In asyncio something like transport.write is synchronous, and the developer needs to implement things like pause_reading and pause_writing to handle backpressure correctly. This blog post introduced me to the concept:

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

In trio you just have a socket-like object that you can read and write from, and both of those are async-functions which block.

@tomchristie
Copy link
Member

Right. Tho we’re taking care of flow control, and the application only ever sees ‘send’ and ‘receive’ async interfaces, so we don’t really need to be concerned that trio offers a differently designed interface for I/O. (Since apps never see that level of abstraction anyways)

One thing I particularly like about ASGI is that it removes the application developer from having to be concerned about the asyncio interface itself - instead you’re just in async/await land.

The one point where that doesn’t hold is when applications want to spawn a task. It’s possible that ASGI could mandate a particular async interface for that, in order to provide:

  • Managed lifetimes. Ie everything within an ASGI app runs in the equivalent of a trio nursery context. (asyncio implementions would need to watch the collection of tasks spawned within each ASGI lifetime, and provide an equivalent to MultiException)
  • async-implementation independence.

@davidism
Copy link

davidism commented Jul 7, 2018

At PyCon I discussed this point with Andrew and other framework devs and I think we decided that ASGI should provide some mechanism for the app to request a worker (async or thread executor for sync) from the server. This was not only for background tasks but also so that an app could dispatch both sync and async view functions (it would request an executor if it saw a sync view).

I'm probably getting the details wrong, it's been a while. I don't think this is in the spec yet, so it would probably be a good time to open a discussion in the spec repo.

@miracle2k
Copy link
Author

miracle2k commented Jul 7, 2018

One thing I particularly like about ASGI is that it removes the application developer from having to be concerned about the asyncio interface itself - instead you’re just in async/await land.

I might be misunderstanding - my apologies - but it needs to be trio all the way down, right? (or asyncio all the way down). So, if the code that reads http requests from the socket (i.e. the worker) runs in the asyncio main loop, then my app, behind ASGI, needs to use asyncio as well. I cannot call await trio.sleep(1) - uvicorn running an asyncio loop couldn't handle that.

@tomchristie
Copy link
Member

Yes. I’m just exploring what interface would need to be provided in order to provide a subset of async functionality, where the application doesn’t care what the underlying async implementation is.

Eg if awaitable ‘sleep’ and ‘create_task’ interfaces are passed in on object in the scope then applications could call those, rather than calling ‘asyncio’ or ‘trio’.

For a very large slice of stuff ASGI apps shouldn’t need any of the low level APIs exposed by trio/asyncio/curio. (Since that’s all taken care of in the server.)

@tomchristie
Copy link
Member

Presumably database drivers, redis interfaces, and HTTP client tooling would have to be specific to the async implementation, since they’ll necessarily use low-level async APIs.

Other stuff such as middleware or application code might be able to be async-implementation-independent, so long as it followed a style of using an interface explicitly passed to it by the server.

(This is all just speculative stuff of course)

@miracle2k
Copy link
Author

miracle2k commented Jul 8, 2018

I see. Intriguing idea. There is the https://github.com/theelous3/multio library for prior art that might be of interest.

It might all be possible, but I am personally not hopeful it will work out so well in practice. At the end of every await call stack we are always waiting for the event loop to do something. The options I can think of:

  • a sleep
  • a file or socket read (maybe a subprocess)
  • an internal framework-specific trap (if that is the right word) to handle a Queue or an Event becoming ready

All of these are specific to the framework used, and without them we can't do a lot with async, and it seems quite a burden for the ASGI spec to carry these quite specific things. It would also now require all middleware and application authors to use a new API defined by ASGI (or the web framework implementing ASGI) instead of working directly with trio or asyncio. I fear that in pursuit of making middleware interoperable, we are just adding one more API into the mix.

@tomchristie
Copy link
Member

There are some things we could do to provide a degree of interoperability tho. Eg provide the ‘loop’ to the scope, and recommend that apps use that explicitly rather than ‘asyncio.get_event_loop’ (which for instance allows trio-protocol to transparently work with ASGI apps that create tasks.)

More broadly the question would be: what set of additional constraints that trio or curio have designed for might we also want want to apply to async-within-ASGI. Trio’s nursery constraint can roughly be encapsulated within ASGI if we follow some additional “only create tasks using the loop interface provided in the scope”, as the server is then in a position to monitor the lifetimes of any sub tasks created.

@tomchristie
Copy link
Member

So I guess if we could modify main.py enough that we could have trio as an option then I'd be up for doing this. (Tho we'd want to end up doing it in a way that it's kept as a third party implementation, rather than a built-in.)

Obv it'll be less interoperable than other ASGI implementations, but for anything where it's just using the ASGI interface and async/await then it's a non-issue. (In other words it's only if applications or middleware start creating tasks that it'd lose interoperability.)

@miracle2k
Copy link
Author

A while ago I started the process of publishing it as a standalone package (https://github.com/miracle2k/trio-asgi-server), but I never got around to finishing that - I also wanted to finish the main.py integration.

I'm happy to add it to uvicorn instead, though.

@tomchristie
Copy link
Member

Nice one. I reckon the best tack is probably to keep this entirely third-party, so let’s close this off.

However:

  1. Happy to discuss any API modifications that would make that easier.
  2. Very happy to link to your project from our docs, once your happy with the state of it. Just open a PR or issue.

Cheers! 😃

@tomchristie tomchristie mentioned this pull request Jul 30, 2018
@tomchristie
Copy link
Member

tomchristie commented Jul 30, 2018

@miracle2k On re-review I think I'd be pretty happy to see a trio implementation in the core, tho I'd probably start from the POV of main, rather than the gunicorn worker class. Some initial thoughts against #169.

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.

3 participants