Skip to content

cleanup use of PathId constants and access to inner #148

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.

Sign up for GitHub

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
merged 4 commits into from
Oct 15, 2025
Merged

cleanup use of PathId constants and access to inner #148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
880ce4b
use PathId::ZERO whenever possible
divagant-martian Oct 14, 2025
c4334ad
avoid accesing PathId's inner in general
divagant-martian Oct 15, 2025
21fc3fb
fmt
divagant-martian Oct 15, 2025
632e55f
prefer PathId::MAX over max construction
divagant-martian Oct 15, 2025
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
104 changes: 57 additions & 47 deletions quinn-proto/src/connection/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ impl Connection {
allow_server_migration: side.is_client(),
});
let local_cid_state = FxHashMap::from_iter([(
PathId(0),
PathId::ZERO,
CidState::new(
cid_gen.cid_len(),
cid_gen.cid_lifetime(),
Expand All @@ -362,7 +362,7 @@ impl Connection {
rem_handshake_cid: rem_cid,
local_cid_state,
paths: BTreeMap::from_iter([(
PathId(0),
PathId::ZERO,
PathState {
data: path,
prev: None,
Expand Down Expand Up @@ -426,7 +426,7 @@ impl Connection {
),
datagrams: DatagramState::default(),
config,
rem_cids: FxHashMap::from_iter([(PathId(0), CidQueue::new(rem_cid))]),
rem_cids: FxHashMap::from_iter([(PathId::ZERO, CidQueue::new(rem_cid))]),
rng,
stats: ConnectionStats::default(),
version,
Expand All @@ -439,7 +439,7 @@ impl Connection {
abandoned_paths: Default::default(),
};
if path_validated {
this.on_path_validated(PathId(0));
this.on_path_validated(PathId::ZERO);
}
if side.is_client() {
// Kick off the connection
Expand Down Expand Up @@ -920,7 +920,7 @@ impl Connection {
return Some(challenge);
}
let mut space_id = match path_id {
PathId(0) => SpaceId::Initial,
PathId::ZERO => SpaceId::Initial,
_ => SpaceId::Data,
};

Expand Down Expand Up @@ -1159,7 +1159,7 @@ impl Connection {
{
debug_assert!(
is_multipath_enabled || path_id == PathId::ZERO,
"Only PathId(0) allowed without multipath (have {path_id:?})"
"Only PathId::ZERO allowed without multipath (have {path_id:?})"
);
Self::populate_acks(
now,
Expand Down Expand Up @@ -2039,13 +2039,13 @@ impl Connection {
pub fn rtt(&self) -> Duration {
// this should return at worst the same that the poll_transmit logic would use
// TODO(@divma): wrong
self.path_data(PathId(0)).rtt.get()
self.path_data(PathId::ZERO).rtt.get()
}

/// Current state of this connection's congestion controller, for debugging purposes
pub fn congestion_state(&self) -> &dyn Controller {
// TODO(@divma): same as everything, wrong
self.path_data(PathId(0)).congestion.as_ref()
self.path_data(PathId::ZERO).congestion.as_ref()
}

/// Resets path-specific settings.
Expand All @@ -2062,7 +2062,7 @@ impl Connection {
// TODO(@divma): evaluate how this is used
// wrong call in the multipath case anyhow
self.paths
.get_mut(&PathId(0))
.get_mut(&PathId::ZERO)
.expect("this might fail")
.data
.reset(now, &self.config);
Expand Down Expand Up @@ -2099,8 +2099,11 @@ impl Connection {
}
self.max_concurrent_paths = count;

let in_use_count =
(self.local_max_path_id.0 + 1).saturating_sub(self.abandoned_paths.len() as u32);
let in_use_count = self
.local_max_path_id
.next()
.saturating_sub(self.abandoned_paths.len() as u32)
.as_u32();
let extra_needed = count.get().saturating_sub(in_use_count);
let new_max_path_id = self.local_max_path_id.saturating_add(extra_needed);

Expand Down Expand Up @@ -3018,7 +3021,7 @@ impl Connection {
let _guard = span.enter();
debug_assert!(self.side.is_server());
let len = packet.header_data.len() + packet.payload.len();
let path_id = PathId(0);
let path_id = PathId::ZERO;
self.path_data_mut(path_id).total_recvd = len as u64;

match self.state {
Expand All @@ -3028,7 +3031,7 @@ impl Connection {
_ => unreachable!("first packet must be delivered in Handshake state"),
}

// The first packet is always on PathId(0)
// The first packet is always on PathId::ZERO
self.on_packet_authenticated(
now,
SpaceId::Initial,
Expand Down Expand Up @@ -3529,7 +3532,7 @@ impl Connection {
Header::Retry {
src_cid: rem_cid, ..
} => {
debug_assert_eq!(path_id, PathId(0));
debug_assert_eq!(path_id, PathId::ZERO);
if self.side.is_server() {
return Err(TransportError::PROTOCOL_VIOLATION("client sent Retry").into());
}
Expand Down Expand Up @@ -3566,13 +3569,13 @@ impl Connection {
self.retry_src_cid = Some(rem_cid);
self.rem_cids
.get_mut(&path_id)
.expect("PathId(0) not yet abandoned, is_valid_retry would have been false")
.expect("PathId::ZERO not yet abandoned, is_valid_retry would have been false")
.update_initial_cid(rem_cid);
self.rem_handshake_cid = rem_cid;

let space = &mut self.spaces[SpaceId::Initial];
if let Some(info) = space.for_path(PathId(0)).take(0) {
self.on_packet_acked(now, PathId(0), info);
if let Some(info) = space.for_path(PathId::ZERO).take(0) {
self.on_packet_acked(now, PathId::ZERO, info);
};

self.discard_space(now, SpaceId::Initial); // Make sure we clean up after
Expand All @@ -3592,8 +3595,11 @@ impl Connection {
};

// Retransmit all 0-RTT data
let zero_rtt =
mem::take(&mut self.spaces[SpaceId::Data].for_path(PathId(0)).sent_packets);
let zero_rtt = mem::take(
&mut self.spaces[SpaceId::Data]
.for_path(PathId::ZERO)
.sent_packets,
);
for info in zero_rtt.into_values() {
self.paths
.get_mut(&PathId::ZERO)
Expand Down Expand Up @@ -3623,7 +3629,7 @@ impl Connection {
dst_cid: loc_cid,
..
} => {
debug_assert_eq!(path_id, PathId(0));
debug_assert_eq!(path_id, PathId::ZERO);
if rem_cid != self.rem_handshake_cid {
debug!(
"discarding packet with mismatched remote CID: {} != {}",
Expand Down Expand Up @@ -3706,13 +3712,13 @@ impl Connection {
dst_cid: loc_cid,
..
}) => {
debug_assert_eq!(path_id, PathId(0));
debug_assert_eq!(path_id, PathId::ZERO);
if !state.rem_cid_set {
trace!("switching remote CID to {}", rem_cid);
let mut state = state.clone();
self.rem_cids
.get_mut(&path_id)
.expect("PathId(0) not yet abandoned")
.expect("PathId::ZERO not yet abandoned")
.update_initial_cid(rem_cid);
self.rem_handshake_cid = rem_cid;
self.orig_rem_cid = rem_cid;
Expand Down Expand Up @@ -3785,7 +3791,7 @@ impl Connection {
packet: Packet,
) -> Result<(), TransportError> {
debug_assert_ne!(packet.header.space(), SpaceId::Data);
debug_assert_eq!(path_id, PathId(0));
debug_assert_eq!(path_id, PathId::ZERO);
let payload_len = packet.payload.len();
let mut ack_eliciting = false;
for result in frame::Iter::new(packet.payload.freeze())? {
Expand Down Expand Up @@ -4461,7 +4467,7 @@ impl Connection {
/// Handle a change in the local address, i.e. an active migration
pub fn local_address_changed(&mut self) {
// TODO(flub): if multipath is enabled this needs to create a new path entirely.
self.update_rem_cid(PathId(0));
self.update_rem_cid(PathId::ZERO);
self.ping();
}

Expand Down Expand Up @@ -4508,8 +4514,8 @@ impl Connection {
fn issue_first_cids(&mut self, now: Instant) {
if self
.local_cid_state
.get(&PathId(0))
.expect("PathId(0) exists when the connection is created")
.get(&PathId::ZERO)
.expect("PathId::ZERO exists when the connection is created")
.cid_len()
== 0
{
Expand All @@ -4525,24 +4531,25 @@ impl Connection {
}
}
self.endpoint_events
.push_back(EndpointEventInner::NeedIdentifiers(PathId(0), now, n));
.push_back(EndpointEventInner::NeedIdentifiers(PathId::ZERO, now, n));
}

/// Issues an initial set of CIDs for paths that have not yet had any CIDs issued
///
/// Later CIDs are issued when CIDs expire or are retired by the peer.
fn issue_first_path_cids(&mut self, now: Instant) {
if let Some(PathId(max_path_id)) = self.max_path_id() {
let start_path_id = self.max_path_id_with_cids.0 + 1;
for n in start_path_id..=max_path_id {
if let Some(max_path_id) = self.max_path_id() {
let mut path_id = self.max_path_id_with_cids.next();
while path_id <= max_path_id {
self.endpoint_events
.push_back(EndpointEventInner::NeedIdentifiers(
PathId(n),
path_id,
now,
self.peer_params.issue_cids_limit(),
));
path_id = path_id.next();
}
self.max_path_id_with_cids = PathId(max_path_id);
self.max_path_id_with_cids = max_path_id;
}
}

Expand Down Expand Up @@ -4633,7 +4640,7 @@ impl Connection {
{
debug_assert!(
is_multipath_negotiated || path_id == PathId::ZERO,
"Only PathId(0) allowed without multipath (have {path_id:?})"
"Only PathId::ZERO allowed without multipath (have {path_id:?})"
);
Self::populate_acks(
now,
Expand Down Expand Up @@ -4955,7 +4962,7 @@ impl Connection {
id = %issued.id,
"NEW_CONNECTION_ID"
);
debug_assert_eq!(issued.path_id, PathId(0));
debug_assert_eq!(issued.path_id, PathId::ZERO);
self.stats.frame_tx.new_connection_id += 1;
None
}
Expand All @@ -4975,7 +4982,7 @@ impl Connection {
let retire_cid_bound = frame::RetireConnectionId::size_bound(is_multipath_negotiated);
while !path_exclusive_only && buf.remaining_mut() > retire_cid_bound {
let (path_id, sequence) = match space.pending.retire_cids.pop() {
Some((PathId(0), seq)) if !is_multipath_negotiated => {
Some((PathId::ZERO, seq)) if !is_multipath_negotiated => {
trace!(sequence = seq, "RETIRE_CONNECTION_ID");
self.stats.frame_tx.retire_connection_id += 1;
(None, seq)
Expand Down Expand Up @@ -5165,7 +5172,7 @@ impl Connection {
trace!("negotiated max idle timeout {:?}", self.idle_timeout);

if let Some(ref info) = params.preferred_address {
// During the handshake PathId(0) exists.
// During the handshake PathId::ZERO exists.
self.rem_cids.get_mut(&PathId::ZERO).expect("not yet abandoned").insert(frame::NewConnectionId {
path_id: None,
sequence: 1,
Expand Down Expand Up @@ -5327,13 +5334,13 @@ impl Connection {
#[cfg(test)]
pub(crate) fn bytes_in_flight(&self) -> u64 {
// TODO(@divma): consider including for multipath?
self.path_data(PathId(0)).in_flight.bytes
self.path_data(PathId::ZERO).in_flight.bytes
}

/// Number of bytes worth of non-ack-only packets that may be sent
#[cfg(test)]
pub(crate) fn congestion_window(&self) -> u64 {
let path = self.path_data(PathId(0));
let path = self.path_data(PathId::ZERO);
path.congestion
.window()
.saturating_sub(path.in_flight.bytes)
Expand Down Expand Up @@ -5361,18 +5368,21 @@ impl Connection {
/// Whether explicit congestion notification is in use on outgoing packets.
#[cfg(test)]
pub(crate) fn using_ecn(&self) -> bool {
self.path_data(PathId(0)).sending_ecn
self.path_data(PathId::ZERO).sending_ecn
}

/// The number of received bytes in the current path
#[cfg(test)]
pub(crate) fn total_recvd(&self) -> u64 {
self.path_data(PathId(0)).total_recvd
self.path_data(PathId::ZERO).total_recvd
}

#[cfg(test)]
pub(crate) fn active_local_cid_seq(&self) -> (u64, u64) {
self.local_cid_state.get(&PathId(0)).unwrap().active_seq()
self.local_cid_state
.get(&PathId::ZERO)
.unwrap()
.active_seq()
}

#[cfg(test)]
Expand All @@ -5390,23 +5400,23 @@ impl Connection {
pub(crate) fn rotate_local_cid(&mut self, v: u64, now: Instant) {
let n = self
.local_cid_state
.get_mut(&PathId(0))
.get_mut(&PathId::ZERO)
.unwrap()
.assign_retire_seq(v);
self.endpoint_events
.push_back(EndpointEventInner::NeedIdentifiers(PathId(0), now, n));
.push_back(EndpointEventInner::NeedIdentifiers(PathId::ZERO, now, n));
}

/// Check the current active remote CID sequence for `PathId(0)`
/// Check the current active remote CID sequence for `PathId::ZERO`
#[cfg(test)]
pub(crate) fn active_rem_cid_seq(&self) -> u64 {
self.rem_cids.get(&PathId(0)).unwrap().active_seq()
self.rem_cids.get(&PathId::ZERO).unwrap().active_seq()
}

/// Returns the detected maximum udp payload size for the current path
#[cfg(test)]
pub(crate) fn path_mtu(&self) -> u16 {
self.path_data(PathId(0)).current_mtu()
self.path_data(PathId::ZERO).current_mtu()
}

/// Whether we have 1-RTT data to send
Expand Down Expand Up @@ -5455,7 +5465,7 @@ impl Connection {
/// Buffers passed to [`Connection::poll_transmit`] should be at least this large.
pub fn current_mtu(&self) -> u16 {
// TODO(@divma): fix
self.path_data(PathId(0)).current_mtu()
self.path_data(PathId::ZERO).current_mtu()
}

/// Size of non-frame data for a 1-RTT packet
Expand Down
2 changes: 1 addition & 1 deletion quinn-proto/src/connection/packet_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ impl<'a, 'b> PacketBuilder<'a, 'b> {
self.buf.put_bytes(0, packet_crypto.tag_len());
let encode_start = self.partial_encode.start;
let packet_buf = &mut self.buf.as_mut_slice()[encode_start..];
// for packet protection, PathId(0) and no path are equivalent.
// for packet protection, PathId::ZERO and no path are equivalent.
self.partial_encode.finish(
packet_buf,
header_crypto,
Expand Down
11 changes: 10 additions & 1 deletion quinn-proto/src/connection/paths.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ use crate::{
use qlog::events::quic::MetricsUpdated;

/// Id representing different paths when using multipath extension
// TODO(@divma): improve docs, reconsider access to inner
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy, Default, Hash)]
pub struct PathId(pub(crate) u32);

Expand Down Expand Up @@ -61,6 +60,16 @@ impl PathId {
let inner = self.0.saturating_sub(rhs.0);
Self(inner)
}

/// Get the next [`PathId`]
pub(crate) fn next(&self) -> Self {
self.saturating_add(Self(1))
}

/// Get the underlying u32
pub(crate) fn as_u32(&self) -> u32 {
self.0
}
}

impl std::fmt::Display for PathId {
Expand Down
Loading
Loading