Closed BastiDood closed 2 years ago
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.
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.
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.
Yes. Specifically, it replaces the
Arc
you call it on with anArc
that is guaranteed to have exactly one reference. Mutations toArc::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.
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.
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?
Default
ing the whole of ServerConfig
is impossible because the rustls config, even if available, is cannot be Default
ed (some certificate must be supplied for the config to be well-formed).
That's unfortunate. I suppose the dedicated constructors seem to be the only workable path forward (which I'm also fine with).
Actually, make_mut
doesn't work here because TransportConfig: !Clone
due to dyn CongestionControllerFactory
.
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
. 😅
I think #1301 would cover this?
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.
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!
I don't think it makes sense to go out of the way to avoid building Arc
s; constructing quinn configuration objects is not going to be a performance bottleneck in your application.
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.
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.
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).
That is already how new
is used.
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
.
Revised the PR, please have another look (and leave feedback in the PR's changes rather than here, please).
Some
Arc
ShenanigansTo 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.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.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.