Skip to content

keep track of the local and remote max path id #63

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions quinn-proto/src/config/transport.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use std::{fmt, sync::Arc};

use crate::{
Duration, INITIAL_MTU, MAX_UDP_PAYLOAD, VarInt, VarIntBoundsExceeded, address_discovery,
congestion,
Duration, INITIAL_MTU, MAX_UDP_PAYLOAD, PathId, VarInt, VarIntBoundsExceeded,
address_discovery, congestion,
};

/// Parameters governing the core QUIC state machine
Expand Down Expand Up @@ -49,7 +49,7 @@ pub struct TransportConfig {

pub(crate) address_discovery_role: address_discovery::Role,

pub(crate) initial_max_path_id: Option<u32>,
pub(crate) initial_max_path_id: Option<PathId>,
}

impl TransportConfig {
Expand Down Expand Up @@ -355,7 +355,7 @@ impl TransportConfig {
// this as max_concurrent_multipath_paths a bit like max_concurrent_bidi_streams etc
// exist now. But this will do for now.
pub fn initial_max_path_id(&mut self, value: Option<u32>) -> &mut Self {
self.initial_max_path_id = value;
self.initial_max_path_id = value.map(Into::into);
self
}
}
Expand Down
63 changes: 53 additions & 10 deletions quinn-proto/src/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ pub struct Connection {
/// This needs to be ordered because [`Connection::poll_transmit`] needs to
/// deterministically select the next PathId to send on.
///
/// TODO(flub): well does it really? But deterministic is nice for now.
// TODO(flub): well does it really? But deterministic is nice for now.
paths: BTreeMap<PathId, PathState>,
/// Whether MTU detection is supported in this environment
allow_mtud: bool,
Expand Down Expand Up @@ -244,6 +244,20 @@ pub struct Connection {
stats: ConnectionStats,
/// QUIC version used for the connection.
version: u32,

/// Local maximum [`PathId`] to be used.
///
/// This is initially set to [`TransportConfig::initial_max_path_id`] when multipath is enabled, or to
/// [`PathId::ZERO`] otherwise.
// TODO(@divma): we probably need an API to imcrease upon the original value
local_max_path_id: PathId,

/// Remote's maximum [`PathId`] to be used.
///
/// This is initially set to the peer's [`TransportParameters::initial_max_path_id`] when
/// multipath is enabled, or to [`PathId::ZERO`] otherwise. A peer may increase this limit by
/// sending [`Frame::MaxPathId`] frames.
remote_max_path_id: PathId,
}

struct PathState {
Expand Down Expand Up @@ -377,6 +391,9 @@ impl Connection {
rng,
stats: ConnectionStats::default(),
version,
// peer params are not yet known, so multipath is not enabled
local_max_path_id: PathId::ZERO,
remote_max_path_id: PathId::ZERO,
};
if path_validated {
this.on_path_validated(PathId(0));
Expand Down Expand Up @@ -2894,9 +2911,7 @@ impl Connection {

// Multipath can only be enabled after the state has reached Established.
// So this can not happen any earlier.
if self.is_multipath_enabled() {
self.issue_first_path_cids(now);
}
self.issue_first_path_cids(now);
Ok(())
}
Header::Initial(InitialHeader {
Expand Down Expand Up @@ -3396,8 +3411,15 @@ impl Connection {
Frame::PathAvailable(_) => {
// TODO(@divma): do stuff
}
Frame::MaxPathId(_) => {
// TODO(@divma): do stuff
Frame::MaxPathId(path_id) => {
if self.is_multipath_enabled() {
// frames that do not increase the path id are ignored
self.remote_max_path_id = self.remote_max_path_id.max(path_id);
} else {
return Err(TransportError::PROTOCOL_VIOLATION(
"received MAX_PATH_ID frame when not multipath was not negotiated",
));
}
}
Frame::PathsBlocked(_) => {
// TODO(@divma): do stuff
Expand Down Expand Up @@ -3566,14 +3588,14 @@ impl Connection {
.push_back(EndpointEventInner::NeedIdentifiers(PathId(0), now, n));
}

/// Issues an initial set of CIDs to the peer for PathId > 0
/// Issues an initial set of CIDs to the peer for PathId > 0; CIDs for [`PathId::ZERO`] are
/// issued earlier in the connection process.
// TODO(flub): Whenever we close paths we need to issue new CIDs as well. Once we do
// that we probably want to re-use this function but with a max_path_id parameter or
// something.
fn issue_first_path_cids(&mut self, now: Instant) {
debug_assert!(self.is_multipath_enabled());
if let Some(max_path_id) = self.config.initial_max_path_id {
for n in 1..(max_path_id + 1) {
if let Some(PathId(max_path_id)) = self.max_path_id() {
for n in 1..=max_path_id {
self.endpoint_events
.push_back(EndpointEventInner::NeedIdentifiers(
PathId(n),
Expand Down Expand Up @@ -4072,6 +4094,15 @@ impl Connection {
self.set_reset_token(remote, info.stateless_reset_token);
}
self.ack_frequency.peer_max_ack_delay = get_max_ack_delay(&params);

if let (Some(local_max_path_id), Some(remote_max_path_id)) =
(self.config.initial_max_path_id, params.initial_max_path_id)
{
// multipath is enabled, register the local and remote maximums
self.local_max_path_id = local_max_path_id;
self.remote_max_path_id = remote_max_path_id;
}

self.peer_params = params;
let peer_max_udp_payload_size =
u16::try_from(self.peer_params.max_udp_payload_size.into_inner()).unwrap_or(u16::MAX);
Expand Down Expand Up @@ -4411,6 +4442,18 @@ impl Connection {
new_tokens.push(remote_addr);
}
}

/// Returns the maximum [`PathId`] to be used in this connection.
///
/// This is calculated as minimum between the local and remote's maximums when multipath is
/// enabled, or `None` when disabled.
fn max_path_id(&self) -> Option<PathId> {
if self.is_multipath_enabled() {
Some(self.remote_max_path_id.min(self.local_max_path_id))
} else {
None
}
}
}

impl fmt::Debug for Connection {
Expand Down
3 changes: 3 additions & 0 deletions quinn-proto/src/connection/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ impl PathId {
/// The maximum path ID allowed.
pub const MAX: Self = Self(u32::MAX);

/// The 0 path id.
pub const ZERO: Self = Self(0);

pub(crate) fn size(&self) -> usize {
VarInt(self.0 as u64).size()
}
Expand Down
3 changes: 1 addition & 2 deletions quinn-proto/src/transport_parameters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,7 @@ impl TransportParameters {
order
}),
address_discovery_role: config.address_discovery_role,
// TODO(@divma): TransportConfig or..?
initial_max_path_id: config.initial_max_path_id.map(PathId::from),
initial_max_path_id: config.initial_max_path_id,
..Self::default()
}
}
Expand Down
Loading