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

async-std support #1343

Closed
wants to merge 1 commit into from
Closed

Conversation

yu-re-ka
Copy link

@yu-re-ka yu-re-ka commented Apr 13, 2022

#502

No ECN or GSO support yet, but it shouldn't be too difficult

@yu-re-ka yu-re-ka force-pushed the feature/async-std branch from 8637b3e to ea2da52 Compare April 13, 2022 22:33
@yu-re-ka
Copy link
Author

Hopefully fixed the lint?

I first wanted to put runtime-tokio into the default features, but it turns out you can not disable default features when running examples? So then you wouldn't be able to run the examples with async-std runtime. This way (no runtime-tokio in default features) means everyone has to explicitly enable either runtime.

@Ralith
Copy link
Collaborator

Ralith commented Apr 14, 2022

Thanks for working on this! Unfortunately, mutually exclusive cargo features are not an acceptable solution, as cargo features must be composable, else libraries that work fine in isolation can cause failures when included anywhere in the same dependency graph.

A viable approach would instead formally abstract the crate over the runtime. We fundamentally need only a few features from the host runtime:

  • The capacity to register for wake-ups when a file descriptor (or winsock handle) is readable or writable
  • The capacity to register for wake-ups at a particular time in the future
  • The capacity to spawn tasks

There should be no deep complexity in constructing an abstraction in these terms.

This could be approached in stages, starting by factoring out runtime-specific logic from quinn-udp, which should be usable (and therefore permit ECN/GSO/etc) on any runtime. I also don't think tokio's sync primitives are runtime-specific, so those can be left in on the first pass to simplify review.

@yu-re-ka yu-re-ka closed this Apr 14, 2022
@spacekookie
Copy link

@Ralith while I do agree that mutually exclusive cargo feature aren't great, I think sometimes this is a reasonable path to take. You can still default to tokio if you want to be backwards compatible.

But aysnc-std support is important.
And I can promise you that people who use async-std are already extremely vigilant about some library that has a hard dependency on tokio and will just willy-nilly load in a second runtime into their project.

Furthermore I don't really understand how your proposed solution is going to fix this issue...

@djc
Copy link
Member

djc commented Apr 14, 2022

@Ralith while I do agree that mutually exclusive cargo feature aren't great, I think sometimes this is a reasonable path to take. You can still default to tokio if you want to be backwards compatible.

Making this not-mutually exclusive doesn't seem like a big ask to me. I usually even avoid building additive Cargo features that change behavior without any API changes, because I've found that to become an issue as well. To me, making Cargo features additive and requiring an API change as well as the relevant Cargo feature is just good engineering, more so for low-level libraries that might be somewhat far removed from the application in the dependency graph.

But aysnc-std support is important. And I can promise you that people who use async-std are already extremely vigilant about some library that has a hard dependency on tokio and will just willy-nilly load in a second runtime into their project.

I mean, async-std is important to what so far is a relatively small part of our audience, and neither of the maintainers is intrinsically motivated to work on this. Note that @Ralith was not saying that there needs to be a tokio dependency in the long run, but that that might be a way to stage the work. And no one said anything about "willy-nilly load in a second runtime", obviously no one here wants that to happen.

Furthermore I don't really understand how your proposed solution is going to fix this issue...

Have you read the discussion in #502? It contains some further discussion. One approach might be to only port quinn-udp to abstract over the runtime and fork the quinn crate if you feel like that's an easier approach.

@Ralith
Copy link
Collaborator

Ralith commented Apr 14, 2022

Furthermore I don't really understand how your proposed solution is going to fix this issue...

What part is unclear, exactly? I gave a list of capabilities that need to be abstracted. If that is accomplished, any environment supplying those capabilities can be supported.

@yu-re-ka
Copy link
Author

#1345

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.

4 participants