diff --git a/noq-proto/src/config/transport.rs b/noq-proto/src/config/transport.rs index 734e0ca31..44b9955ea 100644 --- a/noq-proto/src/config/transport.rs +++ b/noq-proto/src/config/transport.rs @@ -31,6 +31,7 @@ pub struct TransportConfig { pub(crate) max_concurrent_bidi_streams: VarInt, pub(crate) max_concurrent_uni_streams: VarInt, pub(crate) max_idle_timeout: Option, + pub(crate) handshake_idle_timeout: Duration, pub(crate) stream_receive_window: VarInt, pub(crate) receive_window: VarInt, pub(crate) send_window: u64, @@ -118,6 +119,16 @@ impl TransportConfig { self } + /// Maximum duration of inactivity to accept before abandoning a handshake. + /// + /// Used in place of [`max_idle_timeout`](Self::max_idle_timeout) until the handshake + /// completes, so a short idle timeout meant to detect a dead peer quickly does not also cut + /// a handshake off after a single probe. Defaults to 10 seconds. + pub fn handshake_idle_timeout(&mut self, value: Duration) -> &mut Self { + self.handshake_idle_timeout = value; + self + } + /// Maximum number of bytes the peer may transmit without acknowledgement on any one stream /// before becoming blocked. /// @@ -558,6 +569,8 @@ impl Default for TransportConfig { max_concurrent_uni_streams: 100u32.into(), // 30 second default recommended by RFC 9308 ยง 3.2 max_idle_timeout: Some(VarInt(30_000)), + // msquic's HandshakeIdleTimeoutMs default + handshake_idle_timeout: Duration::from_secs(10), stream_receive_window: STREAM_RWND.into(), receive_window: VarInt::MAX, send_window: (8 * STREAM_RWND).into(), @@ -610,6 +623,7 @@ impl fmt::Debug for TransportConfig { max_concurrent_bidi_streams, max_concurrent_uni_streams, max_idle_timeout, + handshake_idle_timeout, stream_receive_window, receive_window, send_window, @@ -647,6 +661,7 @@ impl fmt::Debug for TransportConfig { s.field("max_concurrent_bidi_streams", max_concurrent_bidi_streams) .field("max_concurrent_uni_streams", max_concurrent_uni_streams) .field("max_idle_timeout", max_idle_timeout) + .field("handshake_idle_timeout", handshake_idle_timeout) .field("stream_receive_window", stream_receive_window) .field("receive_window", receive_window) .field("send_window", send_window) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 208540e7a..181b54988 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -3633,11 +3633,18 @@ impl Connection { let path = self.path(path_id)?; let pto_count = path.pto_count; + // The idle timeout the probes have to beat, as `reset_idle_timeout` arms it. + let idle = if self.state.is_handshake() { + Some(self.config.handshake_idle_timeout) + } else { + path.idle_timeout.or(self.idle_timeout) + }; + // Cap the maximum interval between two tail-loss probes. let max_interval = if path.rtt.get() > SLOW_RTT_THRESHOLD { // For slow links we want to increase the interval beyond 2s. (path.rtt.get() * 3) / 2 - } else if let Some(idle) = path.idle_timeout.or(self.idle_timeout) + } else if let Some(idle) = idle && idle <= MIN_IDLE_FOR_FAST_PTO { // If the idle timeout is relatively low, cap at 1s so we get plenty of retries @@ -3913,18 +3920,28 @@ impl Connection { /// enabled there is an additional per-path idle timeout. fn reset_idle_timeout(&mut self, now: Instant, space: SpaceKind, path_id: PathId) { // First reset the global idle timeout. - if let Some(timeout) = self.idle_timeout { - if self.state.is_closed() { - self.timers - .stop(Timer::Conn(ConnTimer::Idle), self.qlog.with_time(now)); - } else { - let dt = cmp::max(timeout, 3 * self.max_pto_for_space(space)); + let timeout = if self.state.is_closed() { + None + } else if self.state.is_handshake() { + Some(self.config.handshake_idle_timeout) + } else { + self.idle_timeout + }; + // A deadline too far out to represent never fires. + let deadline = timeout.and_then(|timeout| { + now.checked_add(cmp::max(timeout, 3 * self.max_pto_for_space(space))) + }); + match deadline { + Some(deadline) => { self.timers.set( Timer::Conn(ConnTimer::Idle), - now + dt, + deadline, self.qlog.with_time(now), ); } + None => self + .timers + .stop(Timer::Conn(ConnTimer::Idle), self.qlog.with_time(now)), } // Now handle the per-path state. @@ -4800,6 +4817,8 @@ impl Connection { self.events.push_back(Event::Connected); self.state.move_to_established(); + // Swap the handshake idle timeout for the negotiated one. + self.reset_idle_timeout(now, SpaceKind::Data, path_id); trace!("established"); // Multipath can only be enabled after the state has reached Established. diff --git a/noq-proto/src/endpoint.rs b/noq-proto/src/endpoint.rs index d4a29c33b..1b9f3d46b 100644 --- a/noq-proto/src/endpoint.rs +++ b/noq-proto/src/endpoint.rs @@ -15,8 +15,8 @@ use thiserror::Error; use tracing::{debug, error, trace, warn}; use crate::{ - Duration, FourTuple, INITIAL_MTU, Instant, MAX_CID_SIZE, MIN_INITIAL_SIZE, PathId, - RESET_TOKEN_SIZE, ResetToken, Side, Transmit, TransportConfig, TransportError, + FourTuple, INITIAL_MTU, Instant, MAX_CID_SIZE, MIN_INITIAL_SIZE, PathId, RESET_TOKEN_SIZE, + ResetToken, Side, Transmit, TransportConfig, TransportError, cid_generator::ConnectionIdGenerator, coding::{BufMutExt, Decodable, Encodable, UnexpectedEnd}, config::{ClientConfig, EndpointConfig, ServerConfig}, @@ -567,12 +567,11 @@ impl Endpoint { let server_config = server_config.unwrap_or_else(|| self.server_config.as_ref().unwrap().clone()); - if server_config - .transport - .max_idle_timeout - .is_some_and(|timeout| { - incoming.received_at + Duration::from_millis(timeout.into()) <= now - }) + // The handshake starts when its Initial arrives, not when it is accepted. + if incoming + .received_at + .checked_add(server_config.transport.handshake_idle_timeout) + .is_some_and(|deadline| deadline <= now) { debug!("abandoning accept of stale initial"); self.index.remove_initial(dst_cid); diff --git a/noq-proto/src/tests/mod.rs b/noq-proto/src/tests/mod.rs index bc0be067e..4fdf517ce 100644 --- a/noq-proto/src/tests/mod.rs +++ b/noq-proto/src/tests/mod.rs @@ -2871,6 +2871,119 @@ fn lost_finished_after_initial_backoff() { assert!(elapsed < Duration::from_secs(1), "{elapsed:?}"); } +/// A short idle timeout does not cut the handshake to two Initial flights. +/// +/// Before an RTT sample the PTO is 999ms, so a 2s idle timeout is armed at 2997ms, while +/// the second probe is due 999ms + 1998ms after the first send, plus however late the first +/// probe went out. The handshake idle timeout governs until the handshake completes. +#[test] +fn handshake_outlasts_short_idle_timeout() { + let _guard = subscribe(); + let mut pair = Pair::default(); + let mut transport = TransportConfig::default(); + transport.max_idle_timeout(Some(Duration::from_secs(2).try_into().unwrap())); + let mut client = client_config(); + client.transport_config(Arc::new(transport)); + + let client_ch = pair.begin_connect(client); + let start = pair.time; + // The first flight is lost, and so is the first probe, which goes out 2ms late. + pair.drive_client(); + pair.server.inbound.clear(); + pair.time = pair.client.next_wakeup().unwrap() + Duration::from_millis(2); + loop { + pair.drive_client(); + pair.server.inbound.clear(); + pair.time = pair.client.next_wakeup().unwrap(); + // Past any pacing: the second probe, due 2999ms after the first send, beyond the + // old 2997ms idle deadline. + if pair.time - start > Duration::from_secs(2) { + break; + } + } + + pair.drive(); + assert_matches!( + pair.client_conn_mut(client_ch).poll(), + Some(Event::HandshakeDataReady) + ); + assert_matches!( + pair.client_conn_mut(client_ch).poll(), + Some(Event::Connected) + ); +} + +/// The handshake idle timeout picks the cap on the probe interval during the handshake, so a +/// short one gets the fast cap even with the idle timeout disabled. +#[test] +fn handshake_idle_timeout_caps_probe_interval() { + let _guard = subscribe(); + let mut pair = Pair::default(); + let mut transport = TransportConfig::default(); + transport + .max_idle_timeout(None) + .handshake_idle_timeout(Duration::from_secs(7)); + let mut client = client_config(); + client.transport_config(Arc::new(transport)); + + let client_ch = pair.begin_connect(client); + let start = pair.time; + // The first flight and the probes at 1s and 3s are lost. + while pair.time - start < Duration::from_secs(5) { + pair.drive_client(); + pair.server.inbound.clear(); + pair.time = pair.client.next_wakeup().unwrap(); + } + + // The fast cap sends the next probe at about 6.05s, inside the 7s handshake idle timeout. + // The normal 2s cap would send it at about 7.05s, after the client gave up. + pair.drive(); + assert_matches!( + pair.client_conn_mut(client_ch).poll(), + Some(Event::HandshakeDataReady) + ); + assert_matches!( + pair.client_conn_mut(client_ch).poll(), + Some(Event::Connected) + ); +} + +/// A handshake idle timeout too long to add to an `Instant` never fires, instead of panicking. +#[test] +fn unbounded_handshake_idle_timeout() { + let _guard = subscribe(); + let mut transport = TransportConfig::default(); + transport.handshake_idle_timeout(Duration::MAX); + let transport = Arc::new(transport); + let mut server = server_config(); + server.transport = transport.clone(); + let mut client = client_config(); + client.transport_config(transport); + + let mut pair = Pair::new(Default::default(), server); + pair.connect_with(client); +} + +/// An Initial left waiting for `accept` past the handshake idle timeout is stale, even +/// though the idle timeout has not expired. +#[test] +fn stale_initial_uses_handshake_idle_timeout() { + let _guard = subscribe(); + let mut pair = Pair::default(); + pair.server.handle_incoming = Box::new(|_| IncomingConnectionBehavior::Wait); + pair.begin_connect(client_config()); + pair.drive_client(); + pair.drive_server(); + let incoming = pair.server.waiting_incoming.pop().unwrap(); + + // Past the 10s handshake idle timeout, inside the 30s idle timeout. + pair.time += Duration::from_secs(11); + assert_matches!( + pair.server.try_accept(incoming, pair.time), + Err(ConnectionError::TimedOut) + ); +} + /// Ensures that the server can respond with 3 initial packets during the handshake /// before the anti-amplification limit kicks in when MTUs are similar. #[test] diff --git a/noq/src/tests.rs b/noq/src/tests.rs index f4bc8e9af..882523229 100755 --- a/noq/src/tests.rs +++ b/noq/src/tests.rs @@ -66,6 +66,7 @@ fn handshake_timeout() { let mut transport_config = TransportConfig::default(); transport_config .max_idle_timeout(Some(IDLE_TIMEOUT.try_into().unwrap())) + .handshake_idle_timeout(IDLE_TIMEOUT) .initial_rtt(Duration::from_millis(10)); client_config.transport_config(Arc::new(transport_config));