From aa795366a36f6bb25a43157cf71ef769b95dcbb5 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Wed, 25 Jun 2025 10:23:34 +0200 Subject: [PATCH] Refuse to close a path if it is the last path available. --- quinn-proto/src/connection/mod.rs | 43 +++++++++++++++++++++++++----- quinn-proto/src/lib.rs | 8 +++--- quinn-proto/src/tests/multipath.rs | 17 ++++++++++-- quinn-proto/src/transport_error.rs | 13 +++++++++ quinn/src/path.rs | 12 +++++---- 5 files changed, 76 insertions(+), 17 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index a2c144c97..6527a9e55 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -530,8 +530,27 @@ impl Connection { Ok(next_path_id) } - /// Closes a path - pub fn close_path(&mut self, _id: PathId, _error_code: VarInt) { + /// Closes a path by sending a PATH_ABANDON frame + /// + /// This will allow closing the last path. Once the corresponding PATH_ABANDON is + /// received from the peer the connection will be closed if there is no other open path. + pub fn close_path( + &mut self, + path_id: PathId, + _error_code: VarInt, + ) -> Result<(), ClosePathError> { + let _path = self + .paths + .get_mut(&path_id) + .ok_or(ClosePathError::ClosedPath)?; + if self.paths.len() < 2 { + return Err(ClosePathError::LastOpenPath); + } + // - send PATH_ABANDON + // - retire received CIDs for this path - this means no more sending anything using + // it. + // - Now set a timer for 3 * PTO to remove the rest of the state? and set the reset + // token. todo!() } @@ -1545,7 +1564,8 @@ impl Connection { Timer::PathIdle(path_id) => { // TODO(flub): TransportErrorCode::NO_ERROR but where's the API to get // that into a VarInt? - self.close_path(path_id, VarInt::from_u32(0)); + self.close_path(path_id, TransportErrorCode::NO_ERROR.into()) + .ok(); } Timer::KeepAlive => { trace!("sending keep-alive"); @@ -3615,16 +3635,16 @@ impl Connection { }; if self.side.is_server() - && path_id == PathId(0) + && path_id == PathId::ZERO && self .rem_cids - .get(&PathId(0)) + .get(&PathId::ZERO) .map(|cids| cids.active_seq() == 0) .unwrap_or_default() { // We're a server still using the initial remote CID for the client, so // let's switch immediately to enable clientside stateless resets. - self.update_rem_cid(PathId(0)); + self.update_rem_cid(PathId::ZERO); } } Frame::NewToken(NewToken { token }) => { @@ -4943,6 +4963,17 @@ impl fmt::Debug for Connection { } } +/// Errors triggered when abandoning a path +#[derive(Debug, Error, Clone, Eq, PartialEq)] +pub enum ClosePathError { + /// The path is already closed or was never opened + #[error("closed path")] + ClosedPath, + /// This is the last path, which can not be abandoned + #[error("last open path")] + LastOpenPath, +} + #[derive(Debug, Copy, Clone, PartialEq, Eq)] enum PathBlocked { No, diff --git a/quinn-proto/src/lib.rs b/quinn-proto/src/lib.rs index 60738eab3..71a92027a 100644 --- a/quinn-proto/src/lib.rs +++ b/quinn-proto/src/lib.rs @@ -43,10 +43,10 @@ pub use bloom_token_log::BloomTokenLog; mod connection; pub use crate::connection::{ - Chunk, Chunks, ClosedPath, ClosedStream, Connection, ConnectionError, ConnectionStats, - Datagrams, Event, FinishError, FrameStats, OpenPathError, PathEvent, PathId, PathStats, - PathStatus, ReadError, ReadableError, RecvStream, RttEstimator, SendDatagramError, SendStream, - ShouldTransmit, StreamEvent, Streams, UdpStats, WriteError, Written, + Chunk, Chunks, ClosePathError, ClosedPath, ClosedStream, Connection, ConnectionError, + ConnectionStats, Datagrams, Event, FinishError, FrameStats, OpenPathError, PathEvent, PathId, + PathStats, PathStatus, ReadError, ReadableError, RecvStream, RttEstimator, SendDatagramError, + SendStream, ShouldTransmit, StreamEvent, Streams, UdpStats, WriteError, Written, }; #[cfg(feature = "rustls")] diff --git a/quinn-proto/src/tests/multipath.rs b/quinn-proto/src/tests/multipath.rs index bb39e659a..ab5545b98 100644 --- a/quinn-proto/src/tests/multipath.rs +++ b/quinn-proto/src/tests/multipath.rs @@ -6,8 +6,8 @@ use std::sync::Arc; use tracing::info; use crate::{ - ClientConfig, ConnectionHandle, ConnectionId, ConnectionIdGenerator, Endpoint, EndpointConfig, - PathId, PathStatus, ServerConfig, TransportConfig, + ClientConfig, ClosePathError, ConnectionHandle, ConnectionId, ConnectionIdGenerator, Endpoint, + EndpointConfig, PathId, PathStatus, ServerConfig, TransportConfig, }; use super::util::subscribe; @@ -123,3 +123,16 @@ fn path_status() { assert_eq!(server_stats.frame_rx.path_available, 0); assert_eq!(server_stats.frame_rx.path_backup, 1); } + +#[test] +fn path_close_last_path() { + let _guard = subscribe(); + let (mut pair, client_ch, _server_ch) = multipath_pair(); + + let client_conn = pair.client_conn_mut(client_ch); + let err = client_conn + .close_path(PathId::ZERO, 0u8.into()) + .err() + .unwrap(); + assert!(matches!(err, ClosePathError::LastOpenPath)); +} diff --git a/quinn-proto/src/transport_error.rs b/quinn-proto/src/transport_error.rs index d942d76af..325f30632 100644 --- a/quinn-proto/src/transport_error.rs +++ b/quinn-proto/src/transport_error.rs @@ -3,6 +3,7 @@ use std::fmt; use bytes::{Buf, BufMut}; use crate::{ + VarInt, coding::{self, BufExt, BufMutExt}, frame, }; @@ -69,6 +70,18 @@ impl From for u64 { } } +impl From for Code { + fn from(value: VarInt) -> Self { + Self(value.0) + } +} + +impl From for VarInt { + fn from(value: Code) -> Self { + VarInt(value.0) + } +} + macro_rules! errors { {$($name:ident($val:expr) $desc:expr;)*} => { #[allow(non_snake_case, unused)] diff --git a/quinn/src/path.rs b/quinn/src/path.rs index 5d6803102..e3d8d225a 100644 --- a/quinn/src/path.rs +++ b/quinn/src/path.rs @@ -3,7 +3,9 @@ use std::pin::Pin; use std::task::{Context, Poll, ready}; use std::time::Duration; -use proto::{ClosedPath, ConnectionError, OpenPathError, PathId, PathStatus, VarInt}; +use proto::{ + ClosePathError, ClosedPath, ConnectionError, OpenPathError, PathId, PathStatus, VarInt, +}; use tokio::sync::oneshot; use crate::connection::ConnectionRef; @@ -89,17 +91,17 @@ impl Path { /// /// The passed in `error_code` is sent to the remote. /// The future will resolve to the `error_code` received from the remote. - pub fn close(&self, error_code: VarInt) -> ClosePath { + pub fn close(&self, error_code: VarInt) -> Result { let (on_path_close_send, on_path_close_recv) = oneshot::channel(); { let mut state = self.conn.state.lock("close_path"); - state.inner.close_path(self.id, error_code); + state.inner.close_path(self.id, error_code)?; state.close_path.insert(self.id, on_path_close_send); } - ClosePath { + Ok(ClosePath { closed: on_path_close_recv, - } + }) } /// Sets the keep_alive_interval for a specific path