diff --git a/neqo-transport/src/connection/tests/ecn.rs b/neqo-transport/src/connection/tests/ecn.rs index 87957297e5..ff86a458f3 100644 --- a/neqo-transport/src/connection/tests/ecn.rs +++ b/neqo-transport/src/connection/tests/ecn.rs @@ -12,11 +12,11 @@ use test_fixture::{ fixture_init, now, DEFAULT_ADDR_V4, }; -use super::send_something_with_modifier; +use super::{send_something_with_modifier, DEFAULT_RTT}; use crate::{ connection::tests::{ connect_force_idle, connect_force_idle_with_modifier, default_client, default_server, - migration::get_cid, new_client, new_server, send_something, + handshake_with_modifier, migration::get_cid, new_client, new_server, send_something, }, ecn::ECN_TEST_COUNT, ConnectionId, ConnectionParameters, StreamType, @@ -67,6 +67,30 @@ fn drop() -> fn(Datagram) -> Option { |_| None } +fn drop_ecn_marked_datagrams() -> fn(Datagram) -> Option { + |d| (!d.tos().is_ecn_marked()).then_some(d) +} + +#[test] +fn handshake_delay_with_ecn_blackhole() { + let start = now(); + let mut client = default_client(); + let mut server = default_server(); + let finish = handshake_with_modifier( + &mut client, + &mut server, + start, + DEFAULT_RTT, + drop_ecn_marked_datagrams(), + ); + + 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.", + ); +} + #[test] fn disables_on_loss() { let now = now(); diff --git a/neqo-transport/src/connection/tests/mod.rs b/neqo-transport/src/connection/tests/mod.rs index e156d8c0c1..3ac4ba7251 100644 --- a/neqo-transport/src/connection/tests/mod.rs +++ b/neqo-transport/src/connection/tests/mod.rs @@ -194,7 +194,6 @@ fn handshake_with_modifier( let mut did_ping = enum_map! {_ => false}; while !is_done(a) { _ = maybe_authenticate(a); - let had_input = input.is_some(); // Insert a PING frame into the first application data packet an endpoint sends, // in order to force the peer to ACK it. For the server, this is depending on the // client's connection state, which is accessible during the tests. @@ -213,12 +212,7 @@ fn handshake_with_modifier( a.test_frame_writer = None; did_ping[a.role()] = true; } - assert!(had_input || output.is_some()); - if let Some(d) = output { - input = modifier(d); - } else { - input = output; - } + input = output.and_then(modifier); qtrace!("handshake: t += {:?}", rtt / 2); now += rtt / 2; mem::swap(&mut a, &mut b); diff --git a/neqo-transport/src/ecn.rs b/neqo-transport/src/ecn.rs index 65b014c7cc..3329b37070 100644 --- a/neqo-transport/src/ecn.rs +++ b/neqo-transport/src/ecn.rs @@ -9,18 +9,30 @@ use std::ops::{AddAssign, Deref, DerefMut, Sub}; use enum_map::EnumMap; use neqo_common::{qdebug, qinfo, qwarn, IpTosEcn}; -use crate::{packet::PacketNumber, recovery::SentPacket}; +use crate::{ + packet::{PacketNumber, PacketType}, + recovery::SentPacket, +}; /// The number of packets to use for testing a path for ECN capability. pub const ECN_TEST_COUNT: usize = 10; +/// The number of packets to use for testing a path for ECN capability when exchanging +/// Initials during the handshake. This is a lower number than [`ECN_TEST_COUNT`] to avoid +/// unnecessarily delaying the handshake; we would otherwise double the PTO [`ECN_TEST_COUNT`] +/// times. +const ECN_TEST_COUNT_INITIAL_PHASE: usize = 3; + /// The state information related to testing a path for ECN capability. /// See RFC9000, Appendix A.4. #[derive(Debug, PartialEq, Clone)] enum EcnValidationState { /// The path is currently being tested for ECN capability, with the number of probes sent so /// far on the path during the ECN validation. - Testing(usize), + Testing { + probes_sent: usize, + initial_probes_lost: usize, + }, /// The validation test has concluded but the path's ECN capability is not yet known. Unknown, /// The path is known to **not** be ECN capable. @@ -31,7 +43,10 @@ enum EcnValidationState { impl Default for EcnValidationState { fn default() -> Self { - Self::Testing(0) + Self::Testing { + probes_sent: 0, + initial_probes_lost: 0, + } } } @@ -112,7 +127,7 @@ impl EcnInfo { /// We do not implement the part of the RFC that says to exit ECN validation if the time since /// the start of ECN validation exceeds 3 * PTO, since this seems to happen much too quickly. pub fn on_packet_sent(&mut self) { - if let EcnValidationState::Testing(ref mut probes_sent) = &mut self.state { + if let EcnValidationState::Testing { probes_sent, .. } = &mut self.state { *probes_sent += 1; qdebug!("ECN probing: sent {} probes", probes_sent); if *probes_sent == ECN_TEST_COUNT { @@ -138,6 +153,28 @@ impl EcnInfo { && (self.baseline - prev_baseline)[IpTosEcn::Ce] > 0 } + pub fn on_packets_lost(&mut self, lost_packets: &[SentPacket]) { + if let EcnValidationState::Testing { + probes_sent, + initial_probes_lost: probes_lost, + } = &mut self.state + { + *probes_lost += lost_packets + .iter() + .filter(|p| p.packet_type() == PacketType::Initial && p.ecn_mark().is_ecn_marked()) + .count(); + // If we have lost all initial probes a bunch of times, we can conclude that the path + // is not ECN capable and likely drops all ECN marked packets. + if probes_sent == probes_lost && *probes_lost == ECN_TEST_COUNT_INITIAL_PHASE { + qdebug!( + "ECN validation failed, all {} initial marked packets were lost", + probes_lost + ); + self.state = EcnValidationState::Failed; + } + } + } + /// After the ECN validation test has ended, check if the path is ECN capable. pub fn validate_ack_ecn_and_update( &mut self, diff --git a/neqo-transport/src/path.rs b/neqo-transport/src/path.rs index b8d3256f19..a8d96d7e5a 100644 --- a/neqo-transport/src/path.rs +++ b/neqo-transport/src/path.rs @@ -1002,6 +1002,7 @@ impl Path { now: Instant, ) { debug_assert!(self.is_primary()); + self.ecn_info.on_packets_lost(lost_packets); let cwnd_reduced = self.sender.on_packets_lost( self.rtt.first_sample_time(), prev_largest_acked_sent,