quinn-rs / quinn

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

async-std support #1343

Closed yu-re-ka closed 2 years ago

yu-re-ka commented 2 years ago

502

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

yu-re-ka commented 2 years ago

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 commented 2 years ago

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:

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.

spacekookie commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

https://github.com/quinn-rs/quinn/pull/1345