diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 6914d3b2a9..1d68880f2c 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1040,7 +1040,7 @@ impl Connection { qtrace!([self], "Idle/keepalive timer {:?}", idle_time); delays.push(idle_time); - if let Some(lr_time) = self.loss_recovery.next_timeout(rtt) { + if let Some(lr_time) = self.loss_recovery.next_timeout(&path) { qtrace!([self], "Loss recovery timer {:?}", lr_time); delays.push(lr_time); } @@ -1664,7 +1664,9 @@ impl Connection { .clone(), ), ); - path.borrow_mut().set_valid(now); + if self.role == Role::Client { + path.borrow_mut().set_valid(now); + } } /// If the path isn't permanent, assign it a connection ID to make it so. @@ -2375,7 +2377,7 @@ impl Connection { } if encoder.is_empty() { - qdebug!("TX blocked, profile={:?} ", profile); + qdebug!("TX blocked, profile={:?}", profile); Ok(SendOption::No(profile.paced())) } else { // Perform additional padding for Initial packets as necessary. diff --git a/neqo-transport/src/connection/tests/handshake.rs b/neqo-transport/src/connection/tests/handshake.rs index fce6e0bf97..3249c1664a 100644 --- a/neqo-transport/src/connection/tests/handshake.rs +++ b/neqo-transport/src/connection/tests/handshake.rs @@ -783,19 +783,15 @@ fn anti_amplification() { let c_init = client.process_output(now).dgram(); now += DEFAULT_RTT / 2; let s_init1 = server.process(c_init.as_ref(), now).dgram().unwrap(); - assert_eq!(s_init1.len(), client.plpmtu()); + assert_eq!(s_init1.len(), server.plpmtu()); let s_init2 = server.process_output(now).dgram().unwrap(); assert_eq!(s_init2.len(), server.plpmtu()); - - // Skip the gap for pacing here. - let s_pacing = server.process_output(now).callback(); - assert_ne!(s_pacing, Duration::new(0, 0)); - now += s_pacing; - let s_init3 = server.process_output(now).dgram().unwrap(); assert_eq!(s_init3.len(), server.plpmtu()); let cb = server.process_output(now).callback(); - assert_ne!(cb, Duration::new(0, 0)); + + // We are blocked by the amplification limit now. + assert_eq!(cb, server.conn_params.get_idle_timeout()); now += DEFAULT_RTT / 2; client.process_input(&s_init1, now); diff --git a/neqo-transport/src/connection/tests/recovery.rs b/neqo-transport/src/connection/tests/recovery.rs index a94a3c0b5d..80e715ff10 100644 --- a/neqo-transport/src/connection/tests/recovery.rs +++ b/neqo-transport/src/connection/tests/recovery.rs @@ -584,6 +584,10 @@ fn loss_time_past_largest_acked() { let s_hs3 = server.process(None, now + s_pto).dgram(); assert!(s_hs3.is_some()); + // We are blocked by the amplification limit now. + let cb = server.process_output(now).callback(); + assert_eq!(cb, server.conn_params.get_idle_timeout()); + // Get some Handshake packets from the client. // We need one to be left unacknowledged before one that is acknowledged. // So that the client engages the loss recovery timer. diff --git a/neqo-transport/src/path.rs b/neqo-transport/src/path.rs index a8d96d7e5a..ec56ebdb81 100644 --- a/neqo-transport/src/path.rs +++ b/neqo-transport/src/path.rs @@ -1016,6 +1016,14 @@ impl Path { } } + /// Determine whether we should be setting a PTO for this path. This is true when either the + /// path is valid or when there is enough remaining in the amplification limit to fit a + /// full-sized path (i.e., the path MTU). + pub fn pto_possible(&self) -> bool { + // See the implementation of `amplification_limit` for details. + self.amplification_limit() >= self.plpmtu() + } + /// Get the number of bytes that can be written to this path. pub fn amplification_limit(&self) -> usize { if matches!(self.state, ProbeState::Failed) { diff --git a/neqo-transport/src/recovery/mod.rs b/neqo-transport/src/recovery/mod.rs index e697e78695..b95279a625 100644 --- a/neqo-transport/src/recovery/mod.rs +++ b/neqo-transport/src/recovery/mod.rs @@ -735,9 +735,14 @@ impl LossRecovery { /// Calculate when the next timeout is likely to be. This is the earlier of the loss timer /// and the PTO timer; either or both might be disabled, so this can return `None`. - pub fn next_timeout(&self, rtt: &RttEstimate) -> Option { + pub fn next_timeout(&self, path: &Path) -> Option { + let rtt = path.rtt(); let loss_time = self.earliest_loss_time(rtt); - let pto_time = self.earliest_pto(rtt); + let pto_time = if path.pto_possible() { + self.earliest_pto(rtt) + } else { + None + }; qtrace!( [self], "next_timeout loss={:?} pto={:?}", @@ -1013,7 +1018,7 @@ mod tests { } pub fn next_timeout(&self) -> Option { - self.lr.next_timeout(self.path.borrow().rtt()) + self.lr.next_timeout(&self.path.borrow()) } pub fn discard(&mut self, space: PacketNumberSpace, now: Instant) {