From 2b037cc47a4ca8342e06c0f4a61c2f8757a39820 Mon Sep 17 00:00:00 2001 From: Kershaw Chang Date: Wed, 8 May 2024 09:44:54 +0200 Subject: [PATCH 1/6] make expiry return longer timeout --- neqo-transport/src/connection/idle.rs | 26 ++++++++++++--------- neqo-transport/src/connection/mod.rs | 3 +-- neqo-transport/src/connection/tests/idle.rs | 22 +++++++++++++++++ 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/neqo-transport/src/connection/idle.rs b/neqo-transport/src/connection/idle.rs index e33f3defb3..4686520d92 100644 --- a/neqo-transport/src/connection/idle.rs +++ b/neqo-transport/src/connection/idle.rs @@ -46,20 +46,14 @@ impl IdleTimeout { self.timeout = min(self.timeout, peer_timeout); } - pub fn expiry(&self, now: Instant, pto: Duration, keep_alive: bool) -> Instant { + pub fn expiry(&self, now: Instant, pto: Duration) -> Instant { let start = match self.state { IdleTimeoutState::Init => now, IdleTimeoutState::PacketReceived(t) | IdleTimeoutState::AckElicitingPacketSent(t) => t, }; - let delay = if keep_alive && !self.keep_alive_outstanding { - // For a keep-alive timer, wait for half the timeout interval, but be sure - // not to wait too little or we will send many unnecessary probes. - max(self.timeout / 2, pto) - } else { - max(self.timeout, pto * 3) - }; + let delay = max(self.timeout, pto * 3); qtrace!( - "IdleTimeout::expiry@{now:?} pto={pto:?}, ka={keep_alive} => {t:?}", + "IdleTimeout::expiry@{now:?} pto={pto:?} => {t:?}", t = start + delay ); start + delay @@ -92,7 +86,17 @@ impl IdleTimeout { } pub fn expired(&self, now: Instant, pto: Duration) -> bool { - now >= self.expiry(now, pto, false) + now >= self.expiry(now, pto) + } + + fn keep_alive_timeout(&self, now: Instant, pto: Duration) -> Instant { + let start = match self.state { + IdleTimeoutState::Init => now, + IdleTimeoutState::PacketReceived(t) | IdleTimeoutState::AckElicitingPacketSent(t) => t, + }; + // For a keep-alive timer, wait for half the timeout interval, but be sure + // not to wait too little or we will send many unnecessary probes. + start + max(self.timeout / 2, pto) } pub fn send_keep_alive( @@ -101,7 +105,7 @@ impl IdleTimeout { pto: Duration, tokens: &mut Vec, ) -> bool { - if !self.keep_alive_outstanding && now >= self.expiry(now, pto, true) { + if !self.keep_alive_outstanding && now >= self.keep_alive_timeout(now, pto) { self.keep_alive_outstanding = true; tokens.push(RecoveryToken::KeepAlive); true diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 732cd31cf4..61e7bc0052 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1029,8 +1029,7 @@ impl Connection { let rtt = path.rtt(); let pto = rtt.pto(PacketNumberSpace::ApplicationData); - let keep_alive = self.streams.need_keep_alive(); - let idle_time = self.idle_timeout.expiry(now, pto, keep_alive); + let idle_time = self.idle_timeout.expiry(now, pto); qtrace!([self], "Idle/keepalive timer {:?}", idle_time); delays.push(idle_time); diff --git a/neqo-transport/src/connection/tests/idle.rs b/neqo-transport/src/connection/tests/idle.rs index 5d01131541..22e3d19c62 100644 --- a/neqo-transport/src/connection/tests/idle.rs +++ b/neqo-transport/src/connection/tests/idle.rs @@ -742,3 +742,25 @@ fn keep_alive_with_ack_eliciting_packet_lost() { assert!(matches!(out, Output::None)); assert!(matches!(client.state(), State::Closed(_))); } + +#[test] +fn keep_alive_with_unresponsive_server() { + let mut client = default_client(); + let mut server = default_server(); + connect(&mut client, &mut server); + + let mut now = now(); + let client_stream = client.stream_create(StreamType::BiDi).unwrap(); + client.stream_keep_alive(client_stream, true).unwrap(); + + for _ in 0..100 { + if client.stream_send(client_stream, &[0x0; 500]).is_err() { + break; + } + if let Output::Callback(t) = client.process_output(now) { + now += t; + } + } + // Connection should be closed due to idle timeout. + assert!(matches!(client.state(), State::Closed(_))); +} From 0037ec57604c40fb2ce020176bed7c3c26aed115 Mon Sep 17 00:00:00 2001 From: Kershaw Chang Date: Mon, 13 May 2024 12:35:51 +0200 Subject: [PATCH 2/6] fix tests --- neqo-transport/src/connection/idle.rs | 23 +++++------ neqo-transport/src/connection/tests/idle.rs | 43 ++++++++------------- 2 files changed, 27 insertions(+), 39 deletions(-) diff --git a/neqo-transport/src/connection/idle.rs b/neqo-transport/src/connection/idle.rs index 4686520d92..9fdc1482fc 100644 --- a/neqo-transport/src/connection/idle.rs +++ b/neqo-transport/src/connection/idle.rs @@ -46,17 +46,18 @@ impl IdleTimeout { self.timeout = min(self.timeout, peer_timeout); } - pub fn expiry(&self, now: Instant, pto: Duration) -> Instant { - let start = match self.state { + fn start(&self, now: Instant) -> Instant { + match self.state { IdleTimeoutState::Init => now, IdleTimeoutState::PacketReceived(t) | IdleTimeoutState::AckElicitingPacketSent(t) => t, - }; + } + } + + pub fn expiry(&self, now: Instant, pto: Duration) -> Instant { let delay = max(self.timeout, pto * 3); - qtrace!( - "IdleTimeout::expiry@{now:?} pto={pto:?} => {t:?}", - t = start + delay - ); - start + delay + let t = self.start(now) + delay; + qtrace!("IdleTimeout::expiry@{now:?} pto={pto:?} => {t:?}"); + t } pub fn on_packet_sent(&mut self, now: Instant) { @@ -90,13 +91,9 @@ impl IdleTimeout { } fn keep_alive_timeout(&self, now: Instant, pto: Duration) -> Instant { - let start = match self.state { - IdleTimeoutState::Init => now, - IdleTimeoutState::PacketReceived(t) | IdleTimeoutState::AckElicitingPacketSent(t) => t, - }; // For a keep-alive timer, wait for half the timeout interval, but be sure // not to wait too little or we will send many unnecessary probes. - start + max(self.timeout / 2, pto) + self.start(now) + max(self.timeout / 2, pto) } pub fn send_keep_alive( diff --git a/neqo-transport/src/connection/tests/idle.rs b/neqo-transport/src/connection/tests/idle.rs index 22e3d19c62..894365c2bf 100644 --- a/neqo-transport/src/connection/tests/idle.rs +++ b/neqo-transport/src/connection/tests/idle.rs @@ -415,9 +415,8 @@ fn keep_alive_initiator() { let stream = create_stream_idle(&mut server, &mut client); let mut now = now(); - // Marking the stream for keep-alive changes the idle timeout. server.stream_keep_alive(stream, true).unwrap(); - assert_idle(&mut server, now, default_timeout() / 2); + assert_idle(&mut server, now, default_timeout()); // Wait that long and the server should send a PING frame. now += default_timeout() / 2; @@ -431,8 +430,8 @@ fn keep_alive_initiator() { let out = server.process(out.as_ref(), now).dgram(); assert!(client.process(out.as_ref(), now).dgram().is_none()); - // Check that there will be next keep-alive ping after default_timeout() / 2. - assert_idle(&mut server, now, default_timeout() / 2); + // Check that there will be next keep-alive ping after default_timeout(). + assert_idle(&mut server, now, default_timeout()); now += default_timeout() / 2; let pings_before2 = server.stats().frame_tx.ping; let ping = server.process_output(now).dgram(); @@ -449,9 +448,8 @@ fn keep_alive_lost() { let stream = create_stream_idle(&mut server, &mut client); let mut now = now(); - // Marking the stream for keep-alive changes the idle timeout. server.stream_keep_alive(stream, true).unwrap(); - assert_idle(&mut server, now, default_timeout() / 2); + assert_idle(&mut server, now, default_timeout()); // Wait that long and the server should send a PING frame. now += default_timeout() / 2; @@ -480,7 +478,7 @@ fn keep_alive_lost() { // return some small timeout for the recovry although it does not have // any outstanding data. Therefore we call it after AT_LEAST_PTO. now += AT_LEAST_PTO; - assert_idle(&mut server, now, default_timeout() / 2 - AT_LEAST_PTO); + assert_idle(&mut server, now, default_timeout() - AT_LEAST_PTO); } /// The other peer can also keep it alive. @@ -492,9 +490,8 @@ fn keep_alive_responder() { let stream = create_stream_idle(&mut server, &mut client); let mut now = now(); - // Marking the stream for keep-alive changes the idle timeout. client.stream_keep_alive(stream, true).unwrap(); - assert_idle(&mut client, now, default_timeout() / 2); + assert_idle(&mut client, now, default_timeout()); // Wait that long and the client should send a PING frame. now += default_timeout() / 2; @@ -513,7 +510,7 @@ fn keep_alive_unmark() { let stream = create_stream_idle(&mut client, &mut server); client.stream_keep_alive(stream, true).unwrap(); - assert_idle(&mut client, now(), default_timeout() / 2); + assert_idle(&mut client, now(), default_timeout()); client.stream_keep_alive(stream, false).unwrap(); assert_idle(&mut client, now(), default_timeout()); @@ -543,11 +540,11 @@ fn keep_alive_close() { let stream = create_stream_idle(&mut client, &mut server); client.stream_keep_alive(stream, true).unwrap(); - assert_idle(&mut client, now(), default_timeout() / 2); + assert_idle(&mut client, now(), default_timeout()); client.stream_close_send(stream).unwrap(); transfer_force_idle(&mut client, &mut server); - assert_idle(&mut client, now(), default_timeout() / 2); + assert_idle(&mut client, now(), default_timeout()); server.stream_close_send(stream).unwrap(); transfer_force_idle(&mut server, &mut client); @@ -564,17 +561,18 @@ fn keep_alive_reset() { let stream = create_stream_idle(&mut client, &mut server); client.stream_keep_alive(stream, true).unwrap(); - assert_idle(&mut client, now(), default_timeout() / 2); + assert_idle(&mut client, now(), default_timeout()); client.stream_close_send(stream).unwrap(); transfer_force_idle(&mut client, &mut server); - assert_idle(&mut client, now(), default_timeout() / 2); + assert_idle(&mut client, now(), default_timeout()); server.stream_reset_send(stream, 0).unwrap(); transfer_force_idle(&mut server, &mut client); assert_idle(&mut client, now(), default_timeout()); // The client will fade away from here. + eprintln!("The client will fade away from here."); let t = now() + (default_timeout() / 2); assert_eq!(client.process_output(t).callback(), default_timeout() / 2); let t = now() + default_timeout(); @@ -590,7 +588,7 @@ fn keep_alive_stop_sending() { let stream = create_stream_idle(&mut client, &mut server); client.stream_keep_alive(stream, true).unwrap(); - assert_idle(&mut client, now(), default_timeout() / 2); + assert_idle(&mut client, now(), default_timeout()); client.stream_close_send(stream).unwrap(); client.stream_stop_sending(stream, 0).unwrap(); @@ -614,14 +612,14 @@ fn keep_alive_multiple_stop() { let stream = create_stream_idle(&mut client, &mut server); client.stream_keep_alive(stream, true).unwrap(); - assert_idle(&mut client, now(), default_timeout() / 2); + assert_idle(&mut client, now(), default_timeout()); let other = client.stream_create(StreamType::BiDi).unwrap(); client.stream_keep_alive(other, true).unwrap(); - assert_idle(&mut client, now(), default_timeout() / 2); + assert_idle(&mut client, now(), default_timeout()); client.stream_keep_alive(stream, false).unwrap(); - assert_idle(&mut client, now(), default_timeout() / 2); + assert_idle(&mut client, now(), default_timeout()); client.stream_keep_alive(other, false).unwrap(); assert_idle(&mut client, now(), default_timeout()); @@ -692,9 +690,8 @@ fn keep_alive_with_ack_eliciting_packet_lost() { // Create a stream. let stream = client.stream_create(StreamType::BiDi).unwrap(); - // Marking the stream for keep-alive changes the idle timeout. client.stream_keep_alive(stream, true).unwrap(); - assert_idle(&mut client, now, IDLE_TIMEOUT / 2); + assert_idle(&mut client, now, IDLE_TIMEOUT); // Send data on the stream that will be lost. _ = client.stream_send(stream, DEFAULT_STREAM_DATA).unwrap(); @@ -709,12 +706,6 @@ fn keep_alive_with_ack_eliciting_packet_lost() { let retransmit = client.process_output(now).dgram(); assert!(retransmit.is_some()); - // The next callback should be for an idle PING. - assert_eq!( - client.process_output(now).callback(), - IDLE_TIMEOUT / 2 - pto - ); - // Wait that long and the client should send a PING frame. now += IDLE_TIMEOUT / 2 - pto; let pings_before = client.stats().frame_tx.ping; From b76e55a927162c72651856114c291d317a65d0a6 Mon Sep 17 00:00:00 2001 From: Kershaw Chang Date: Mon, 13 May 2024 12:43:50 +0200 Subject: [PATCH 3/6] again --- neqo-http3/src/connection_client.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/neqo-http3/src/connection_client.rs b/neqo-http3/src/connection_client.rs index 18e513e743..9b82df2849 100644 --- a/neqo-http3/src/connection_client.rs +++ b/neqo-http3/src/connection_client.rs @@ -2620,7 +2620,7 @@ mod tests { force_idle(&mut client, &mut server); let idle_timeout = ConnectionParameters::default().get_idle_timeout(); - assert_eq!(client.process_output(now()).callback(), idle_timeout / 2); + assert_eq!(client.process_output(now()).callback(), idle_timeout); } // Helper function: read response when a server sends HTTP_RESPONSE_2. @@ -5115,7 +5115,7 @@ mod tests { assert!(!fin); force_idle(&mut client, &mut server); - assert_eq!(client.process_output(now()).callback(), idle_timeout / 2); + assert_eq!(client.process_output(now()).callback(), idle_timeout); } #[test] From 06cd57c8045b52046574d663bd1cb0886acca243 Mon Sep 17 00:00:00 2001 From: Kershaw Chang Date: Tue, 14 May 2024 12:39:11 +0200 Subject: [PATCH 4/6] address comments --- neqo-transport/src/connection/tests/idle.rs | 23 +++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/neqo-transport/src/connection/tests/idle.rs b/neqo-transport/src/connection/tests/idle.rs index 894365c2bf..2c45fbb6a9 100644 --- a/neqo-transport/src/connection/tests/idle.rs +++ b/neqo-transport/src/connection/tests/idle.rs @@ -5,6 +5,7 @@ // except according to those terms. use std::{ + cmp::min, mem, time::{Duration, Instant}, }; @@ -572,7 +573,6 @@ fn keep_alive_reset() { assert_idle(&mut client, now(), default_timeout()); // The client will fade away from here. - eprintln!("The client will fade away from here."); let t = now() + (default_timeout() / 2); assert_eq!(client.process_output(t).callback(), default_timeout() / 2); let t = now() + default_timeout(); @@ -706,15 +706,30 @@ fn keep_alive_with_ack_eliciting_packet_lost() { let retransmit = client.process_output(now).dgram(); assert!(retransmit.is_some()); - // Wait that long and the client should send a PING frame. + // The timeout is the smaller of: + // * the idle timeout, less the time we've already waited (one PTO) + // * twice the PTO, because we've already sent one probe + assert_eq!( + client.process_output(now).callback(), + min(IDLE_TIMEOUT - pto, pto * 2), + ); + + // Wait for half the idle timeout (less the PTO we've already waited) + // so that we get a keep-alive. The actual timeout is the smaller of this + // duration or PTO*2, whichever is smaller. now += IDLE_TIMEOUT / 2 - pto; let pings_before = client.stats().frame_tx.ping; let ping = client.process_output(now).dgram(); assert!(ping.is_some()); assert_eq!(client.stats().frame_tx.ping, pings_before + 1); - // The next callback is for a PTO, the PTO timer is 2 * pto now. - assert_eq!(client.process_output(now).callback(), pto * 2); + // The timeout is the smaller of: + // * the idle timeout, less the time we've already waited (IDLE_TIMEOUT / 2) + // * twice the PTO, because we've already sent one probe + assert_eq!( + client.process_output(now).callback(), + min(IDLE_TIMEOUT / 2, pto * 2), + ); now += pto * 2; // Now we will retransmit stream data. let retransmit = client.process_output(now).dgram(); From b0c176c75304989611539ef30c7106b9713540df Mon Sep 17 00:00:00 2001 From: Kershaw Chang Date: Wed, 15 May 2024 15:22:41 +0200 Subject: [PATCH 5/6] only compare pto --- neqo-transport/src/connection/tests/idle.rs | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/neqo-transport/src/connection/tests/idle.rs b/neqo-transport/src/connection/tests/idle.rs index 2c45fbb6a9..4aac186074 100644 --- a/neqo-transport/src/connection/tests/idle.rs +++ b/neqo-transport/src/connection/tests/idle.rs @@ -5,7 +5,6 @@ // except according to those terms. use std::{ - cmp::min, mem, time::{Duration, Instant}, }; @@ -706,13 +705,8 @@ fn keep_alive_with_ack_eliciting_packet_lost() { let retransmit = client.process_output(now).dgram(); assert!(retransmit.is_some()); - // The timeout is the smaller of: - // * the idle timeout, less the time we've already waited (one PTO) - // * twice the PTO, because we've already sent one probe - assert_eq!( - client.process_output(now).callback(), - min(IDLE_TIMEOUT - pto, pto * 2), - ); + // The timeout is the twice the PTO, because we've already sent one probe. + assert_eq!(client.process_output(now).callback(), pto * 2,); // Wait for half the idle timeout (less the PTO we've already waited) // so that we get a keep-alive. The actual timeout is the smaller of this @@ -723,13 +717,8 @@ fn keep_alive_with_ack_eliciting_packet_lost() { assert!(ping.is_some()); assert_eq!(client.stats().frame_tx.ping, pings_before + 1); - // The timeout is the smaller of: - // * the idle timeout, less the time we've already waited (IDLE_TIMEOUT / 2) - // * twice the PTO, because we've already sent one probe - assert_eq!( - client.process_output(now).callback(), - min(IDLE_TIMEOUT / 2, pto * 2), - ); + // The next callback is for a PTO, the PTO timer is 2 * pto now. + assert_eq!(client.process_output(now).callback(), pto * 2,); now += pto * 2; // Now we will retransmit stream data. let retransmit = client.process_output(now).dgram(); From ac395f3f81b6b1facf5f8002425ca1fbaa2b5c25 Mon Sep 17 00:00:00 2001 From: Kershaw Chang Date: Thu, 16 May 2024 10:21:48 +0200 Subject: [PATCH 6/6] address comments --- neqo-transport/src/connection/tests/idle.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/neqo-transport/src/connection/tests/idle.rs b/neqo-transport/src/connection/tests/idle.rs index 4aac186074..336648f776 100644 --- a/neqo-transport/src/connection/tests/idle.rs +++ b/neqo-transport/src/connection/tests/idle.rs @@ -706,11 +706,10 @@ fn keep_alive_with_ack_eliciting_packet_lost() { assert!(retransmit.is_some()); // The timeout is the twice the PTO, because we've already sent one probe. - assert_eq!(client.process_output(now).callback(), pto * 2,); + assert_eq!(client.process_output(now).callback(), pto * 2); // Wait for half the idle timeout (less the PTO we've already waited) - // so that we get a keep-alive. The actual timeout is the smaller of this - // duration or PTO*2, whichever is smaller. + // so that we get a keep-alive. now += IDLE_TIMEOUT / 2 - pto; let pings_before = client.stats().frame_tx.ping; let ping = client.process_output(now).dgram(); @@ -718,7 +717,7 @@ fn keep_alive_with_ack_eliciting_packet_lost() { assert_eq!(client.stats().frame_tx.ping, pings_before + 1); // The next callback is for a PTO, the PTO timer is 2 * pto now. - assert_eq!(client.process_output(now).callback(), pto * 2,); + assert_eq!(client.process_output(now).callback(), pto * 2); now += pto * 2; // Now we will retransmit stream data. let retransmit = client.process_output(now).dgram();