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

Single threaded runtime support #1681

Closed
vlovich opened this issue Oct 10, 2023 · 26 comments
Closed

Single threaded runtime support #1681

vlovich opened this issue Oct 10, 2023 · 26 comments

Comments

@vlovich
Copy link

vlovich commented Oct 10, 2023

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
Copy link
Collaborator

Ralith commented Oct 10, 2023

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
Copy link
Author

vlovich commented Oct 10, 2023

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

@vlovich
Copy link
Author

vlovich commented Oct 10, 2023

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
Copy link
Collaborator

Ralith commented Oct 10, 2023

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
Copy link
Author

vlovich commented Oct 10, 2023

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
Copy link
Collaborator

Ralith commented Oct 10, 2023

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
Copy link
Author

vlovich commented Oct 11, 2023

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 (#915) which is what glommio is so I'm going to stop this experiment for now.

@Ralith
Copy link
Collaborator

Ralith commented Oct 11, 2023

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

@vlovich
Copy link
Author

vlovich commented Oct 11, 2023 via email

@djc
Copy link
Member

djc commented Oct 11, 2023

@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
Copy link
Author

vlovich commented Oct 11, 2023

Pretty sure I'm on main. On e1e1e6e. 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
Copy link
Collaborator

Ralith commented Oct 11, 2023

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
Copy link
Author

vlovich commented Oct 11, 2023

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

@Ralith
Copy link
Collaborator

Ralith commented Oct 11, 2023

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
Copy link
Author

vlovich commented Oct 12, 2023

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
Copy link
Collaborator

Ralith commented Oct 12, 2023

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
Copy link
Author

vlovich commented Oct 12, 2023

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
Copy link
Collaborator

Ralith commented Oct 13, 2023

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:

  • Do not require &mut self in AsyncUdpSocket::poll_send #1519 previously removed &mut from poll_send. Consistency aside, I'm not sure what the motivation for this was, but it'd be nice to be sure we're not breaking someone. Maybe @dignifiedquire can comment on whether this would be difficult for them to adapt to?
  • Future work to improve scalability may involve sending from multiple tasks in parallel. Does this change complicate that? I suspect poll_send wouldn't be sufficient in its current form either, since it can't reasonably track multiple wakers as-is, but the problem merits some careful thought.

@dignifiedquire
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

djc commented Oct 15, 2023

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

@dignifiedquire
Copy link
Contributor

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 rust-lang/rfcs#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
Copy link
Collaborator

Ralith commented Oct 16, 2023

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
Copy link
Collaborator

Ralith commented Oct 16, 2023

@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
Copy link
Contributor

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
Copy link
Collaborator

Ralith commented Jan 7, 2024

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.

@Ralith Ralith closed this as not planned Won't fix, can't repro, duplicate, stale Jan 7, 2024
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

No branches or pull requests

4 participants