Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/4232/docs/iroh/ Last updated: 2026-05-06T10:15:36Z |
|
@mcginty could you take a look at this? The current path selection is a bit limited especially once you have custom transports, but really also if you have more complex scenarios involving just ip and relay transports. This is my attempt to make the selection API more flexible. |
0a79c3c to
7f35601
Compare
| // Apply our new path | ||
| if let Some((addr, rtt)) = selected_path { | ||
| // Apply the selector's primary path. Multi-path selection is not yet | ||
| // supported on the lifecycle side; only the primary is honoured. |
There was a problem hiding this comment.
The comment sounds a bit weird with "lifecycle side", let's just say we only support a single selected path atm.
| /// has chosen. When iroh wants to make this configurable per non-relay transport, this | ||
| /// will move to a method on [`PathSelector`] with a default body that matches the rule | ||
| /// here. | ||
| fn path_status_for(addr: &transports::Addr) -> noq_proto::PathStatus { |
|
|
||
| #[cfg_attr(not(feature = "unstable-custom-transports"), allow(unreachable_pub))] | ||
| impl<'a> PathSelectionContext<'a> { | ||
| /// Module-private — only `select_path` in this file builds one, and the parameter |
There was a problem hiding this comment.
Remove the comment, we never do comments like this
| } | ||
|
|
||
| /// The path currently considered the preferred path to the remote endpoint, if any. | ||
| pub fn current(&self) -> Option<&'a transports::Addr> { |
There was a problem hiding this comment.
Maybe call this previous? not sure, current works too I guess.
| /// | ||
| /// The same address may appear more than once when it is a path on multiple | ||
| /// connections to the remote. Selectors that care should aggregate as appropriate. | ||
| pub fn paths(&self) -> impl Iterator<Item = PathSelectionData<'a>> + '_ { |
There was a problem hiding this comment.
So this iterates over all connections, then over all paths, and yield PathSelectionData for each. This is a bit weird, you can get the same path multiple times with different stats but no further details. Maybe we should just deduplicate and take the min RTT among all path stats for a transport addr?
| #[cfg_attr(not(feature = "unstable-custom-transports"), allow(unreachable_pub))] | ||
| impl<'a> PathSelectionData<'a> { | ||
| /// The address of the candidate path. | ||
| pub fn addr(&self) -> &'a transports::Addr { |
There was a problem hiding this comment.
We usually expose this as TransportAddr but then we'd have to clone, so it's fine I think.
| } | ||
| } | ||
|
|
||
| impl std::fmt::Debug for PathSelectionData<'_> { |
| self.addr | ||
| } | ||
|
|
||
| /// QUIC path statistics: rtt, cwnd, loss, mtu, etc. |
There was a problem hiding this comment.
Let's not have this list of abbreviations there, instead link to [PathStats]
| /// in real networks is non-trivial. | ||
| #[cfg_attr(not(feature = "unstable-custom-transports"), allow(unreachable_pub))] | ||
| pub trait PathSelector: Send + Sync + std::fmt::Debug + 'static { | ||
| /// Picks a path among the candidates known for a remote endpoint. |
There was a problem hiding this comment.
"Pick the selected path to carry application data among the currently open network paths to the remote endpoint"
|
|
||
| /// The set of paths a [`PathSelector`] has chosen. | ||
| /// | ||
| /// Today this holds at most one path. Build via [`PathSelection::default`] + |
There was a problem hiding this comment.
"Currently only one path is supported. Additional paths will be ignored and emit a warn log" or such.
| } | ||
|
|
||
| /// All paths in this selection (today: 0 or 1). | ||
| #[allow(dead_code)] // only reached via the public re-export behind the unstable feature |
There was a problem hiding this comment.
Remove dead code, and use allow(unreachable_pub) or gate the dead_code with cfg_attr
| #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | ||
| enum TransportType { | ||
| /// A primary path: used whenever available. | ||
| Primary = 0, |
| let mut best: Option<(&Addr, (TransportType, i128))> = None; | ||
| let mut current_key: Option<(TransportType, i128)> = None; | ||
|
|
||
| for psd in ctx.paths() { |
| not(feature = "unstable-custom-transports"), | ||
| allow(unreachable_pub, unused_imports) | ||
| )] | ||
| pub use self::remote_state::{ |
There was a problem hiding this comment.
Maybe do a pub use .. gated behind the feature flag and a use .. gated behind not(feature) instead, would be more explicit IMO
| hooks: Default::default(), | ||
| transport_bias: Default::default(), | ||
| path_selector: Arc::new( | ||
| crate::socket::biased_rtt_path_selector::BiasedRttPathSelector::default(), |
There was a problem hiding this comment.
import instead of full path at call site
Frando
left a comment
There was a problem hiding this comment.
I like it. This is cleaner and more extensible than the previous transport bias solution. Some comments inline, mostly nits.
Description
An attempt to make path selection more flexible for complex custom transport use cases.
Path selection is now via a dynable trait
PathSelector. The only fn select takes aPathSelectionContextthat has the current path as well as a way to iterate over a flat list ofPathSelectionDatastructs.PathSelectionDatacurrently just has the address and the stats, but in the future could be extended to contain more detailed information such as the congestion controller metricsnoq_proto::ControllerMetricsetc. I think cc metrics would be quite helpful to make good decisions about path selection.selectreturns a PathSelection struct that is at this time a newtype over an Option. add is first writer wins, all subsequent calls will ignore the addr and emit a warning. But we can change this in the future to allow multiple addrs.The entire mechanism is only pub under the
unstable-custom-transportsflag, so we can change it in the future. But there are some additive changes we could do without breakage. For example we could allow PathSelection to also keep track of paths to be closed.Breaking Changes
Removes some public methods on the endpoint builder that are too opinionated if you want to make path selection fully generic:
endpoint::Builder::transport_bias(kind, bias): removedendpoint::transports::TransportBias: removedAnd some changes involving the unstable_custom_transport API.
Notes & open questions
Note: the new signature tries to anticipate that in the future we might want to select multiple transports, but as of now it will just ignore these.
Change checklist
quic-rpciroh-gossipiroh-blobsdumbpipesendme