fix: No time-based loss detection before largest_acked & fixes

neqo-bin/src/client/mod.rs

@@ -247,7 +247,7 @@ impl Args {
             }
             "resumption" => {
                 if self.urls.len() < 2 {
-                    qerror!("Warning: resumption test won't work without >1 URL");
+                    qerror!("Warning: resumption tests won't work without >1 URL");
                     exit(127);
                 }
                 self.shared.use_old_http = true;

neqo-transport/src/cc/classic_cc.rs

@@ -337,6 +337,41 @@ impl<T: WindowAdjustment> CongestionControl for ClassicCongestionControl<T> {
         congestion || persistent_congestion
     }
 
+    /// Handle a congestion event.
+    /// Returns true if this was a true congestion event.
+    fn on_congestion_event(&mut self, last_packet: &SentPacket) -> bool {
+        // Start a new congestion event if lost or ECN CE marked packet was sent
+        // after the start of the previous congestion recovery period.
+        if !self.after_recovery_start(last_packet) {
+            return false;
+        }
+
+        let (cwnd, acked_bytes) = self.cc_algorithm.reduce_cwnd(
+            self.congestion_window,
+            self.acked_bytes,
+            self.max_datagram_size(),
+        );
+        self.congestion_window = max(cwnd, self.cwnd_min());
+        self.acked_bytes = acked_bytes;
+        self.ssthresh = self.congestion_window;
+        qdebug!(
+            [self],
+            "Cong event -> recovery; cwnd {}, ssthresh {}",
+            self.congestion_window,
+            self.ssthresh
+        );
+        qlog::metrics_updated(
+            &self.qlog,
+            &[
+                QlogMetric::CongestionWindow(self.congestion_window),
+                QlogMetric::SsThresh(self.ssthresh),
+                QlogMetric::InRecovery(true),
+            ],
+        );
+        self.set_state(State::RecoveryStart);
+        true
+    }
+
     /// Report received ECN CE mark(s) to the congestion controller as a
     /// congestion event.
     ///
@@ -537,41 +572,6 @@ impl<T: WindowAdjustment> ClassicCongestionControl<T> {
         !self.state.transient() && self.recovery_start.map_or(true, |pn| packet.pn() >= pn)
     }
 
-    /// Handle a congestion event.
-    /// Returns true if this was a true congestion event.
-    fn on_congestion_event(&mut self, last_packet: &SentPacket) -> bool {
-        // Start a new congestion event if lost or ECN CE marked packet was sent
-        // after the start of the previous congestion recovery period.
-        if !self.after_recovery_start(last_packet) {
-            return false;
-        }
-
-        let (cwnd, acked_bytes) = self.cc_algorithm.reduce_cwnd(
-            self.congestion_window,
-            self.acked_bytes,
-            self.max_datagram_size(),
-        );
-        self.congestion_window = max(cwnd, self.cwnd_min());
-        self.acked_bytes = acked_bytes;
-        self.ssthresh = self.congestion_window;
-        qdebug!(
-            [self],
-            "Cong event -> recovery; cwnd {}, ssthresh {}",
-            self.congestion_window,
-            self.ssthresh
-        );
-        qlog::metrics_updated(
-            &self.qlog,
-            &[
-                QlogMetric::CongestionWindow(self.congestion_window),
-                QlogMetric::SsThresh(self.ssthresh),
-                QlogMetric::InRecovery(true),
-            ],
-        );
-        self.set_state(State::RecoveryStart);
-        true
-    }
-
     fn app_limited(&self) -> bool {
         if self.bytes_in_flight >= self.congestion_window {
             false

neqo-transport/src/cc/mod.rs

@@ -62,6 +62,11 @@ pub trait CongestionControl: Display + Debug {
         lost_packets: &[SentPacket],
     ) -> bool;
 
+    /// Initiate a congestion response.
+    ///
+    /// Returns true if the congestion window was reduced.
+    fn on_congestion_event(&mut self, last_packet: &SentPacket) -> bool;
+
     /// Returns true if the congestion window was reduced.
     fn on_ecn_ce_received(&mut self, largest_acked_pkt: &SentPacket) -> bool;
 

neqo-transport/src/connection/mod.rs

@@ -634,8 +634,8 @@ impl Connection {
     /// only use it where a more precise value is not important.
     fn pto(&self) -> Duration {
         self.paths.primary().map_or_else(
-            || RttEstimate::default().pto(PacketNumberSpace::ApplicationData),
-            |p| p.borrow().rtt().pto(PacketNumberSpace::ApplicationData),
+            || RttEstimate::default().pto(self.confirmed()),
+            |p| p.borrow().rtt().pto(self.confirmed()),
         )
     }
 
@@ -1058,7 +1058,7 @@ impl Connection {
         if let Some(p) = self.paths.primary() {
             let path = p.borrow();
             let rtt = path.rtt();
-            let pto = rtt.pto(PacketNumberSpace::ApplicationData);
+            let pto = rtt.pto(self.confirmed());
 
             let idle_time = self.idle_timeout.expiry(now, pto);
             qtrace!([self], "Idle/keepalive timer {:?}", idle_time);
@@ -1525,7 +1525,7 @@ impl Connection {
         let mut dcid = None;
 
         qtrace!([self], "{} input {}", path.borrow(), hex(&**d));
-        let pto = path.borrow().rtt().pto(PacketNumberSpace::ApplicationData);
+        let pto = path.borrow().rtt().pto(self.confirmed());
 
         // Handle each packet in the datagram.
         while !slc.is_empty() {
@@ -2138,7 +2138,7 @@ impl Connection {
             // or the PTO timer fired: probe.
             true
         } else {
-            let pto = path.borrow().rtt().pto(PacketNumberSpace::ApplicationData);
+            let pto = path.borrow().rtt().pto(self.confirmed());
             if !builder.packet_empty() {
                 // The packet only contains an ACK.  Check whether we want to
                 // force an ACK with a PING so we can stop tracking packets.
@@ -2419,13 +2419,15 @@ impl Connection {
                 self.loss_recovery.on_packet_sent(path, sent);
             }
 
-            if *space == PacketNumberSpace::Handshake
-                && self.role == Role::Server
-                && self.state == State::Confirmed
-            {
-                // We could discard handshake keys in set_state,
-                // but wait until after sending an ACK.
-                self.discard_keys(PacketNumberSpace::Handshake, now);
+            if *space == PacketNumberSpace::Handshake {
+                if self.role == Role::Client {
+                    // We're sending a Handshake packet, so we can discard Initial keys.
+                    self.discard_keys(PacketNumberSpace::Initial, now);
+                } else if self.role == Role::Server && self.state == State::Confirmed {
+                    // We could discard handshake keys in set_state,
+                    // but wait until after sending an ACK.
+                    self.discard_keys(PacketNumberSpace::Handshake, now);
+                }
             }
         }
 
@@ -2776,18 +2778,17 @@ impl Connection {
                 self.set_initial_limits();
             }
             if self.crypto.install_keys(self.role)? {
-                if self.role == Role::Client {
-                    // We won't acknowledge Initial packets as a result of this, but the
-                    // server can rely on implicit acknowledgment.
-                    self.discard_keys(PacketNumberSpace::Initial, now);
-                }
                 self.saved_datagrams.make_available(CryptoSpace::Handshake);
             }
         }
 
         Ok(())
     }
 
+    fn confirmed(&self) -> bool {
+        self.state == State::Confirmed
+    }
+
     fn set_confirmed(&mut self) -> Res<()> {
         self.set_state(State::Confirmed);
         if self.conn_params.pmtud_enabled() {

neqo-transport/src/connection/tests/ecn.rs

@@ -85,10 +85,19 @@ fn handshake_delay_with_ecn_blackhole() {
         drop_ecn_marked_datagrams(),
     );
 
+    let pto = DEFAULT_RTT * 3;
+    let half_rtt = DEFAULT_RTT / 2;
     assert_eq!(
-        (finish - start).as_millis() / DEFAULT_RTT.as_millis(),
-        15,
-        "expected 6 RTT for client to detect blackhole, 6 RTT for server to detect blackhole and 3 RTT for handshake to be confirmed.",
+        (finish - start),
+        (1 + 2 + 4) * pto + // Client RTOs its CI w/ECN twice (3x total)
+        half_rtt + // Fourth CI w/o ECN being delivered
+        (1 + 2 + 4) * pto + // Server RTOs its CI w/ECN twice (3x total)
+        half_rtt + // Fourth SI w/o ECN being delivered
+        half_rtt + // Client ACK
+        half_rtt + // Client Handshake/Short
+        half_rtt + // Server HandshakeDone
+        half_rtt + // Client ACK
+        half_rtt
     );
 }