[Feature]: Make configuration fields in quinn-proto public

[BastiDood]

Some Arc Shenanigans

To initialize a server/client endpoint, Quinn first requires configuration (e.g. via TransportConfig, ServerConfig, etc.). As mentioned in the documentation, the default options are sufficient for most use cases. Otherwise, we have to go through some hoops with setter methods to achieve the behavior we want.

use quinn::{ServerConfig, TransportConifg, VarInt};
use std::{sync::Arc, time::Duration};

// Configure some special ALPN protocols via `rustls`
let mut server_config = ServerConfig::with_crypto(crypto);

// Create a new transport configuration, and modify its defaults
let mut transport_config = TransportConfig::default();
transport_config
    .keep_alive_interval(Some(Duration::from_secs(30)))
    .max_idle_timeout(Some(VarInt::from(60u32).into()));

// And then... replace the old transport config? This is necessary
// because there's no public constructor for `ServerConfig` that accepts a
// `TransportConfig` at the moment; there is only one for the `crypto` field.
server_config.transport = Arc::new(transport_config);

// Alternatively, we may also modify through the `Arc` directly
// since we're the sole owner of the reference at the moment.
// But, now we're forced to use a nasty `unwrap` in the code...
//
// We may, of course, succumb to the `unsafe` version: `unwrap_unchecked`.
// Although this is technically legal, the Quinn public API documents
// no guarantees about the reference count of the `transport` field. Hence,
// a version bump may unintentionally invoke undefined behavior if we're not careful.
Arc::get_mut(&mut server_config.transport)
    .unwrap()
    .keep_alive_interval(Some(Duration::from_secs(30)))
    .max_idle_timeout(Some(VarInt::from(60u32).into()));

// Finally initialize the endpoint
let (endpoint, incoming) = Endpoint::server(server_config, addr)?;

I hope the example above demonstrates my nitpicks with the current API. This is exactly what I faced in one of my projects which required a custom protocol (mostly for network efficiency's sake because JSON-over-HTTP is too verbose).

My Proposed Solution

Instead, I propose that all fields of the various *Config structs be made public. After all, most of their methods are solely transparent setters. We may as well treat the fields to be public.

To accommodate for default values, we simply use the default spread syntax (..Default::default()) when initializing from the user code.

use quinn::{ServerConfig, TransportConifg, VarInt};
use std::{sync::Arc, time::Duration};

// NOTE: Observe that `mut` is no longer necessary.
let server_config = ServerConfig {
    // Configure some special ALPN protocols via `rustls`
    crypto: Arc::new(rustls::server::ServerConfig { ... }),
    transport: Arc::new(TransportConfig {
        keep_alive_interval: Some(Duration::from_secs(30)),
        max_idle_timeout: Some(VarInt::from(60u32).into()),
        // 🎉 Hooray for default values! 🎉
        ..Default::default()
    }),
    // 🎉 Hooray for **more** default values! 🎉
    // ASIDE: `ServerConfig` does not currently implement `Default`
    // as of Quinn 0.8. This should be addressed before proceeding.
    ..Default::default()
};

// Finally initialize the endpoint
let (endpoint, incoming) = Endpoint::server(server_config, addr)?;

Here, I demonstrate the fact that we no longer have to jump through the hoops of indirection. In particular, direct initialization allows us to remove the Arc shenanigans altogether.

This, to me, is a significantly more ergonomic API than using the original setters. Of course, we don't have to remove the setters from the library. Though, I am not totally against deprecation.

I would love to know your thoughts about this API change. I'd love to send in a PR that addresses this. Just wanted to collect some feedback before committing my time.

[Ralith]

Thanks for your interest! We prefer setters over public fields because it's much easier to maintain stability and perform fine-grained error checking that way, since the public API is decoupled from the internal representation. That said, I agree that the status quo is idiosyncratic, since it mixes both approaches. Perhaps we should privatize all fields and introduce setters for the transport and cryptographic configs.

Note that you can use Arc::make_mut to avoid the unwrap if modifying the existing field in-place.

[BastiDood]

Perhaps we should privatize all fields and introduce setters for the transport and cryptographic configs.

Yes, this would be awesome! I would like to add that not just new setters are required, but also new constructors. The whole reason why I had to dance around with Arc was because there didn't seem to be a public constructor that configured both the crypto and transport fields for the ServerConfig struct at the same time in one fell swoop.

Note that you can use Arc::make_mut to avoid the unwrap if modifying the existing field in-place.

Yup, I'm aware that this exists, but doesn't it clone-if-not-unique-ownership? It certainly makes sense now that the internal implementation initializes the transport field with a reference count of one. However, the issue lies in the fact that the documentation makes no such guarantees.

What if—in a future version bump—the ServerConfig internally clones the Arc elsewhere before passing it back to the user? Then, that would make Arc::make_mut return a &mut to a cloned TransportConfig, and hence performing any mutations do not actually reflect on the original transport field.

[Ralith]

I would like to add that not just new setters are required, but also new constructors

I'm open to this. It's a bit tricky to decide what exactly the constructors should be since we have to balance convenience for the common case against preventing advanced cases from being too awkward, but we're probably not at the sweet spot yet. Modifying e.g. ServerConfig::new to also take an Arc<TransportConfig> seems defensible, since the common case will be using with_single_cert anyway.

doesn't [make_mut] clone-if-not-unique-ownership?

Yes. Specifically, it replaces the Arc you call it on with an Arc that is guaranteed to have exactly one reference. Mutations to Arc::make_mut(&mut config.transport) therefore do affect (only) config.transport`. It wouldn't be a very useful function if it always threw away the result.

[BastiDood]

Yes. Specifically, it replaces the Arc you call it on with an Arc that is guaranteed to have exactly one reference. Mutations to Arc::make_mut(&mut config.transport) therefore do affect (only) config.transport`.

Ah, you're right. I have misunderstood the semantics. Reading the source code again, I have found that the this argument indeed gets overwritten—albeit cloned if necessary, which in our case is not an issue since config.transport will now point to that new allocation.

It's a bit tricky to decide what exactly the constructors should be since we have to balance convenience for the common case against preventing advanced cases from being too awkward, but we're probably not at the sweet spot yet.

Nice! That's good to hear. Luckily, there are only two constructor arguments here in question: transport and crypto. Passing two arguments into a constructor does not seem like a deal-breaker to me as well, especially if the parameters implement Default.

Some crates in the ecosystem follow such a convention, where configuration structs have a default value, then in the constructor, we typically invoke something along the lines of: ServerConfig::new(Default::default(), ...). Though, I must concede that it does read rather verbose. Considering a future of various constructors, I understand why you feel cautious against it.

This is actually why I proposed that we allow construction via public fields. Most users may still invoke the usual constructors. But for advanced configurations, it would certainly be appreciated to at least have the option to tailor the struct granularly.

We thereby work around the discussion of "which constructors to expose" because the public fields already enable various permutations of configuration. In case there are certain fields that require additional checking, then it's totally fine to keep them private. Otherwise, I motion for public visibility.

[Ralith]

Luckily, there are only two constructor arguments here in question: transport and crypto. Passing two arguments into a constructor does not seem like a deal-breaker to me as well, especially if the parameters implement Default.

Note that token_key must also be passed in, because it cannot be defaulted without requiring optional dependencies. This is particularly tricky as its manual construction is non-obvious.

[BastiDood]

Ah, forgot about that. Just to throw some ideas: we may conditionally implement Default for ServerConfig if cfg(feature = "ring") since the default spread syntax is arguably an optional "convenience" from ring (due to the token_key). Would this be feasible?

[Ralith]

Defaulting the whole of ServerConfig is impossible because the rustls config, even if available, is cannot be Defaulted (some certificate must be supplied for the config to be well-formed).

[BastiDood]

That's unfortunate. I suppose the dedicated constructors seem to be the only workable path forward (which I'm also fine with).

[Ralith]

Actually, make_mut doesn't work here because TransportConfig: !Clone due to dyn CongestionControllerFactory.

[BastiDood]

Right... Then that would mean the unwrap for Arc::get_mut would be the only way to attain a &mut config.transport.

In practice, the unwrap should not panic since (at the moment) the Arc is unique upon construction, but I'm sure you would agree that having a dedicated constructor reads more pleasantly than an unwrap. 😅

[djc]

I think #1301 would cover this?

[BastiDood]

Yup, it almost does. There is one missing feature, though: the ServerConfig::with_crypto_and_transport constructor is missing. It would be awesome if there were direct constructors for the Arc-wrapped fields so that it wouldn't be necessary to construct a whole new Arc just to replace it shortly after (via the builder methods).

For the other fields, however, I'm totally fine with them being left out since they should be relatively cheap to overwrite via the builder methods. This is not the case for Arc, which has some underlying allocation semantics that are not-so-cheap, hence my preference to directly initializing them instead of overwriting.

[BastiDood]

Alternatively, we may include the transport as an added argument to ServerConfig::new, but I doubt that would be as negotiable since it is quite a breaking change. Though, it does sound promising!

[Ralith]

I don't think it makes sense to go out of the way to avoid building Arcs; constructing quinn configuration objects is not going to be a performance bottleneck in your application.

[BastiDood]

That's a fair point. Indeed, the one-time initialization cost shouldn't be too much to bend over backwards for. Stylistically, I would still prefer the direct initialization, but it's not a deal-breaker.

[Ralith]

Though to be clear, I have no objection to adding it to new, I just don't think that will be used often in practice since it's relatively awkward to roll your own token key construction.

[BastiDood]

That is true. Just to throw an idea, though: what if we use the new function as the base constructor for with_crypto and friends? Internally, the with_* constructors would just be specialized invocations for new. Then, for the funny rare cases (like mine), we could at least give the user the option to manually call new themselves. In fact, it's probably best to rename new to from_raw or something similar at this point (if we choose to pursue this path).

[Ralith]

That is already how new is used.

[BastiDood]

Oh... Hadn't realized. 😅

Well, in any case, I'll put my two cents here in saying that I'd still appreciate introducing the extra argument to new.

[djc]

Revised the PR, please have another look (and leave feedback in the PR's changes rather than here, please).