quinn-rs / quinn

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

Impl Runtime for Box<dyn Runtime> #1530

Closed kpp closed 1 year ago

kpp commented 1 year ago

In order to be able to hide quinn types from a user library we need to be able to create Endpoints with a dynamicly typed Runtimes. So far there is quinn::Endpoint::new function which is not usable for boxed Runtime's. This PR adds some flexibility to the API allowing a user to pass Box<dyn Runtime> into quinn::Endpoint::new function.

Please take a look at the discussion: https://github.com/libp2p/rust-libp2p/pull/3454#discussion_r1109515886

djc commented 1 year ago

This solution doesn't make sense to me for your stated goal. quinn::Endpoint::new() accepts impl Runtime, so you can provide any type that implements quinn::Runtime to it, including any type that you define in a downstream crate. Why, then, do you need to wrap your runtime in a layer of Box?

(Note that Endpoint::new_with_runtime() which new() calls under the covers, wraps the impl Runtime in Arc, so passing a Box<impl Runtime> to it would result in double-boxing.)

kpp commented 1 year ago

One way to hide quinn types from public API is to hide runtimes behind enum. I cannot extract an impl Runtime from a enum. However I can do something like:

let runtime = P::runtime();
match runtime {
   Tokio(runtime) => quinn::Endpoint::new(endpoint_config, server_config, socket, runtime)?,
   AsyncStd(runtime) => quinn::Endpoint::new(endpoint_config, server_config, socket, runtime)?,
}

But it looks like a boilerplate. So I chose to move to a dynamic polymorphism.

Would you please share a better idea with me?

djc commented 1 year ago

But it looks like a boilerplate. So I chose to move to a dynamic polymorphism.

Honestly, this much boilerplate doesn't seem like much of an issue? IMO this is a pretty reasonable snippet (given that it's only necessary in a few places for Endpoint construction).

Ralith commented 1 year ago

I think we should modify these APIs to accept Arc<dyn Runtime>, since they construct that internally anyway. This came up previously in the BoringSSL work.

djc commented 1 year ago

@kpp I assume #1534 would address your use case?

kpp commented 1 year ago

@djc Yes, thank you!

I used match over enum this time, will fix the code when new release is done.