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

API design suggestion #2078

Closed
Jimmy-Z opened this issue Dec 20, 2023 · 2 comments
Closed

API design suggestion #2078

Jimmy-Z opened this issue Dec 20, 2023 · 2 comments

Comments

@Jimmy-Z
Copy link

Jimmy-Z commented Dec 20, 2023

s2n-quic's API uses the method chaining paradigm, which is - dare I say - often implemented poorly, and I believe s2n-quic is one of them.

The current design works fine if the caller wants to just hardcode (almost) everything, like in the examples, but what if the caller want something configured at runtime?

  1. let's see the example of .with_gso_disabled(), the caller would need to write something like:
let mut builder = Builder::new();
if !args.gso {
    builder = builder.with_gso_disabled();
}
let server = builder.build();

which does not really conform to the method chaining paradigm.
and it doesn't work for some methods since the method might return a different type, which I gave an example here: https://users.rust-lang.org/t/a-question-about-method-chaining-with-different-return-types/104196

I think a better design would be a .with_gso() method that takes a bool value (or a enum with auto|force_on|force_off or something like that), like this:

let server = Server::builder()
    .with_gso(args.gso)
    // ...
    .start();
  1. some methods, like the .with_congestion_controller() looks like it works like that, but since Cubic and Bbr are actually different types, we can't really do this:
builder.with_congestion_controller(if args.bbr {Bbr::default()} else {Cubic::default()}) ...

Sorry for the critical tone and strong wording.

And it's also entirely possible that it's just that my rust-fu is too weak, and this problem is entirely solvable from the caller side, if then, my sincere apologies and please show me how.

@camshaft
Copy link
Contributor

Thanks for the suggestions! I've implemented the first item in #2091.

For the second item, it's a bit trickier. But I think if you're ok with doing runtime dispatch, then we can provide a helper enum that forwards the congestion controller methods on to the correct impls. I've opened #2092 to track that.

@toidiu
Copy link
Contributor

toidiu commented Feb 8, 2024

The work to ergonomically switch CC is being tracked in #2092

@toidiu toidiu closed this as completed Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants