quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.76k stars 380 forks source link

Single threaded runtime support #1681

Closed vlovich closed 8 months ago

vlovich commented 11 months ago

Perusing the implementation, I see that there's a baked in assumption of a work-stealing async runtime. Would there be any interest in having a feature flag that supports a single-threaded (e.g. tokio rt, glommio, monoio, etc) async runtimes? I imagine primarily that will involve dropping the requirement for Send for Runtime futures, replacing Arc with Rc, Mutex with a RefCell, etc).

Ralith commented 11 months ago

I see that there's a baked in assumption of a work-stealing async runtime

Where do you see that? There are several examples of quinn on tokio's single-threaded mode in this repo alone.

vlovich commented 11 months ago

For example, AsyncUdpSocket requires Send+Sync but nothing about Glommio nor monio is Send + Sync. Am I missing something?

vlovich commented 11 months ago

Also, could you point me at the single-threaded mode examples? My grep is failing me but I may be looking for the wrong thing.

Ralith commented 11 months ago

For example, AsyncUdpSocket requires Send+Sync

That allows Quinn to support multithreaded runtimes. It does not require them.

Also, could you point me at the single-threaded mode examples? My grep is failing me but I may be looking for the wrong thing.

Tokio calls its single-threaded mode the "current thread" runtime. Grep for current_thread.

vlovich commented 11 months ago

That allows Quinn to support multithreaded runtimes. It does not require them.

I must be missing something. When I go to add glommio.rs in the runtime/ folder, the build fails saying that Glommio’s UDP socket struct doesn’t implement Send+Sync as required by the AsyncUdpSocket trait. Sync is “solvable” through gratuitous Arc+Mutex wrappers, but send isn’t (eg Glommio has an Rc within it’s UdpSocket class).

I think Tokio’s single threaded runtime might still export Send+Sync for its various classes to avoid duplication which is why tokio works. But I don’t see how the Runtime and AsyncUdpSocket traits having Send+Sync don’t infect into requiring runtime implementations to expose Send+Sync types too.

Ralith commented 11 months ago

Whether a runtime is multithreaded is, as you can see from tokio, a separate concern from whether its I/O primitives are Send + Sync. Common strategies for adapting generic libraries to !Send + !Sync primitives include arranging for operations to panic if invoked on the wrong thread (e.g. using SendWrapper) or unsafely promising never to actually move things between threads with a newtype.

vlovich commented 11 months ago

So the other problem is I hadn't realized that quinn makes direct networking syscalls instead of dispatching to the underlying socket implementation. That means that it's incompatible with completion based I/O (https://github.com/quinn-rs/quinn/issues/915) which is what glommio is so I'm going to stop this experiment for now.

Ralith commented 11 months ago

Quinn dispatches through a AsyncUdpSocket trait object, in which you can provide whatever logic you like, including adaptation to a completion-based layer.

vlovich commented 11 months ago

Quinn itself invokes blocking syscalls directly on the socket if you're using udp::UdpSocketState. That negates the entire point of using a completion based layer (unless you bypass . I think the Runtime abstractions would have to change substantially to delegate I/O operations to the AsyncUdpSocket itself among other changes (as the other thread points out, unless the author is mistaken, there’s a bunch of inherent additional memcpy operations taking place which again negates the point of using something like io_uring).

Are you saying that a completion-based layer should duplicate the logic of udp::UdpSocketState?

djc commented 11 months ago

@vlovich are you looking at a release or the main branch? IIRC the hierarchy of the trait etc was refactored substantially after the last release.

vlovich commented 11 months ago

Pretty sure I'm on main. On e1e1e6e392a382fbded42ca010505fecb8fe3655. I'm following the examples set in tokio and async-std runtimes, both of which dispatch polling to UdpSocketState which then calls sendmsg` (or sendmmsg for non-Apple platforms).

Ralith commented 11 months ago

Quinn itself invokes blocking syscalls directly on the socket

This is incorrect. Quinn performs no blocking I/O on any Runtime; that would rather defeat the point of being an async lib.

Are you saying that a completion-based layer should duplicate the logic of udp::UdpSocketState

You can implement AsyncUdpSocket's interface however you like. If you want to use a runtime different than tokio or async-std, then you will need to use an implementation different from theirs.

vlovich commented 11 months ago

Sorry. I was imprecise. It makes syscalls directly on the socket which makes the complexity of implementing a completion-based API much harder.

Ralith commented 11 months ago

Again, quinn does not issue syscalls directly. All I/O is mediated through AsyncUdpSocket, which you can implement however you like, including by adapting to compleiton-based I/O.

vlovich commented 11 months ago

Let’s say I have an async fn called send. To implement poll_send does that mean I have to box the future for a send and keep it alive until the poll resolves? Is there a reason it’s implemented as a poll interface instead of an async fn that could potentially avoid the need for boxing of futures?

Ralith commented 11 months ago

To implement poll_send does that mean I have to box the future for a send and keep it alive until the poll resolves

If your future is !Unpin, as those constructed by async functions/blocks are, then yes. We could probably modify AsyncUdpSocket methods to take Pin<&mut self> to mitigate this, but you'll have a hard time encapsulating async fn futures without type erasure anyway until TAIT is stabilized. You can pretty easily reuse the same Box indefinitely with e.g. futures::stream::unfold, so this shouldn't matter.

Is there a reason it’s implemented as a poll interface instead of an async fn

Yes; async fn is not supported in traits, let alone trait objects.

vlovich commented 11 months ago

Yes; async fn is not supported in traits, let alone trait objects.

That's actually supposed to be stabilized next month according to https://blog.rust-lang.org/inside-rust/2023/05/03/stabilizing-async-fn-in-trait.html and the target stabilization date according to https://releases.rs/docs/1.74.0/.

Pin<&mut self> to mitigate this, but you'll have a hard time encapsulating async fn futures without type erasure anyway until TAIT is stabilized

I think Pin<&mut self> would actually go a long way because then I can do a poll_send into the specific UdpSocket implementation and avoid the need for bridging polling code with async. Would you be open to this change?

Ralith commented 11 months ago

That's actually supposed to be stabilized next month

Without trait object support. The workaround or this is the same thing you have to do today, and the same thing that eventual native support will probably be sugar for: boxing. A polling API will probably remain more efficient, as it can box once internally rather than on every call.

Would you be open to this change?

I'm interested in it, since we'll never move these values around after polling anyway, but changing from &self to any form of &mut self may not be a trivial rubber-stamp. There's a couple wrinkles:

dignifiedquire commented 11 months ago

I am generally interested in using alternative runtimes like glomio myself, so I am supportive in general of improving the situation. I will have to take a closer look at our code, but I believe the important piece will be habing the same for send and recv.

dignifiedquire commented 11 months ago

Looking at the current abstraction of Runtime and AsyncUdpSocket, I think the source of the challenges in this issue, is that the traits do not allow for any relationship expression. Making it hard to say this type of socket will only work with this kind of runtime. And because of the stringent requirements for a multithreaded runtime, everything else has to adhere to them.

The challenge is that when trying to tie them in some form together, you run into the issue of non object safe traits (both with generics and associated types on the Runtime), so I don't have any good ideas on how to solve this yet.

djc commented 11 months ago

We could also make it so that the Runtime is exclusively responsible for producing the dyn AsyncUdpSockets, I suppose?

dignifiedquire commented 11 months ago

We could also make it so that the Runtime is exclusively responsible for producing the dyn AsyncUdpSockets, I suppose?

Yes, so far I only got a version which adds two generics to almost all structs in quinn :/ but with default types the user api wouldn't have to change.

Looking at improving the situation around Send abstractions the only real solution I have found is https://github.com/rust-lang/rfcs/issues/2190. If using a feature flag would be acceptable (which it might be, given the way that runtimes are selected based on feature flags) https://docs.rs/maybe-sync could allow enforcing Send bounds based on a feature flag, avoiding the need to hack in Send when it is not actually true.

Ralith commented 11 months ago

It is not necessary to produce AsyncUdpSockets that can work on any runtime. As discussed above, you are free to e.g. use SendWrapper to implement AsyncUdpSocket for a !Send backend. This will make it a logic error for you to create an endpoint or connection in an environment incompatible with your implementation, but that's fine.

Ralith commented 11 months ago

@vlovich, it sounds like @dignifiedquire doesn't have an objection to tweaking AsyncUdpSocket to work in terms of Pin<&mut self>, so I'd be happy to review that change if it makes your life easier. Perhaps in the future we'll need a mechanism for duplicating sockets, but we can deal with that when we get there.

dignifiedquire commented 11 months ago

This will make it a logic error for you to create an endpoint or connection in an environment incompatible with your implementation, but that's fine.

Yes, it would just be much nicer if we could use this cool type system to express this 😓

Ralith commented 8 months ago

Closing as non-actionable, since single-threaded runtime support was never missing and tangential discussion has wrapped up. Feel free to open a PR for Pin<&mut self> if it'd help. Note also incoming changes to the socket trait in #1729.