Pluggable congestion control

[Ralith]

As discussed in #693. Draft until the abstractness of the interface has been validated by the addition of at least one other controller (probably CUBIC/HyStart++).

[codecov]

Codecov Report

Merging #759 into master will decrease coverage by 0.16%.
The diff coverage is 32.53%.

@@            Coverage Diff             @@
##           master     #759      +/-   ##
==========================================
- Coverage   70.90%   70.74%   -0.17%     
==========================================
  Files          73       74       +1     
  Lines       12917    12960      +43     
==========================================
+ Hits         9159     9168       +9     
- Misses       3758     3792      +34     

Impacted Files	Coverage Δ
interop/src/main.rs	0.00% <ø> (ø)
quinn-proto/src/lib.rs	83.33% <ø> (ø)
quinn-proto/src/config.rs	28.73% <4.34%> (-2.13%)	⬇️
quinn-proto/src/congestion/new_reno.rs	39.02% <39.02%> (ø)
quinn-proto/src/connection/mod.rs	79.81% <52.63%> (+0.07%)	⬆️
quinn-proto/src/packet.rs	86.42% <0.00%> (-0.24%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17d9788...f065a5a. Read the comment docs.

[djc]

I think I'm okay with the config Arc now. Can you sketch what your explicit trait alternative for the factory thing would look like? I can't quite wrap my head around what the constraints are.

(Otherwise, things look pretty good to me, though I'll do a final pass once we figure out the factory thing.)

[Ralith]

The explicit trait approach:

pub trait CongestionControllerFactory {
    fn new_controller(&self) -> Box<dyn CongestionController>;
}
// ...
impl CongestionControllerFactory for Arc<NewRenoConfig> {
    fn new_controller(&self) -> Box<dyn CongestionController> {
        Box::new(NewReno::new(self.clone(), Instant::now()))
    }
}

In use:

config.congestion_controller_factory(Arc::new(config));

[djc]

How would the maybe_rebinding case look for that? I think it'd need a separate impl? How do you feel about current approach vs explicit trait?

[Ralith]

(self.config.congestion_controller_factory)() would be replaced with self.config.congestion_controller_factory.new_controller(); no other changes. A trait with one method that takes &self is always algebraically equivalent to an analogous Fn.

Between the two, I'm leaning towards the explicit trait right now, because config.congestion_controller_factory(Arc::new(config)); seems like considerably less ceremony than config.congestion_controller_factory(|| Box::new(NewReno::new(config, Instant::now()))). It's more boilerplate if you want to invoke complicated custom logic, but that seems like an extremely niche case.

[djc]

Cool, I think I agree.

[Ralith]

Updated with an explicit factory trait. I also refactored things to make the congestion module public rather than reexporting its contents, to ease discoverability of different algorithms we add in the future, and to limit growth of clutter in the root namespace.

[djc]

Sorry, reviewed the trait in a little more detail.

[djc]

Awesome work!