From 2935e593e34073b443c3238a463cc5f00dbd3b62 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 17 Apr 2024 14:50:04 +0200 Subject: [PATCH 1/6] feat(qns): add resumption testcase Test failure reported in https://github.com/mozilla/neqo/pull/1786#issuecomment-2060058643. Signed-off-by: Max Inden --- .github/workflows/qns.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/qns.yml b/.github/workflows/qns.yml index fc5e45b555..ef7936d112 100644 --- a/.github/workflows/qns.yml +++ b/.github/workflows/qns.yml @@ -71,6 +71,6 @@ jobs: name: 'neqo-latest' image: ${{ steps.docker_build_and_push.outputs.imageID }} url: https://github.com/mozilla/neqo - test: handshake,ecn,keyupdate + test: handshake,ecn,keyupdate,resumption client: neqo-latest,quic-go,ngtcp2,neqo,msquic server: neqo-latest,quic-go,ngtcp2,neqo,msquic From ea370cd37018f61021fc7bc3b689a8cd06cebd76 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 18 Apr 2024 15:35:31 +0200 Subject: [PATCH 2/6] fix: fallback for servers not sending NEW_TOKEN frame See https://github.com/mozilla/neqo/blob/main/neqo-transport/src/connection/mod.rs#L665-L676 for details. Fixes regression introduced in https://github.com/mozilla/neqo/pull/1676. --- neqo-bin/src/client/http09.rs | 10 +++----- neqo-bin/src/client/http3.rs | 10 +++----- neqo-bin/src/client/mod.rs | 48 +++++++++++++++-------------------- 3 files changed, 29 insertions(+), 39 deletions(-) diff --git a/neqo-bin/src/client/http09.rs b/neqo-bin/src/client/http09.rs index 89945e94d1..113d74ea0f 100644 --- a/neqo-bin/src/client/http09.rs +++ b/neqo-bin/src/client/http09.rs @@ -95,12 +95,10 @@ impl<'a> super::Handler for Handler<'a> { Ok(false) } - fn take_token(&mut self) -> Option { - self.token.take() - } - - fn has_token(&self) -> bool { - self.token.is_some() + fn take_token(&mut self, client: &mut Self::Client) -> Option { + self.token + .take() + .or_else(|| client.take_resumption_token(Instant::now())) } } diff --git a/neqo-bin/src/client/http3.rs b/neqo-bin/src/client/http3.rs index 5ba5cc4b20..5ca97eb761 100644 --- a/neqo-bin/src/client/http3.rs +++ b/neqo-bin/src/client/http3.rs @@ -223,12 +223,10 @@ impl<'a> super::Handler for Handler<'a> { Ok(self.url_handler.done()) } - fn take_token(&mut self) -> Option { - self.token.take() - } - - fn has_token(&self) -> bool { - self.token.is_some() + fn take_token(&mut self, client: &mut Self::Client) -> Option { + self.token + .take() + .or_else(|| client.take_resumption_token(Instant::now())) } } diff --git a/neqo-bin/src/client/mod.rs b/neqo-bin/src/client/mod.rs index 04fe09e1ab..2f49970031 100644 --- a/neqo-bin/src/client/mod.rs +++ b/neqo-bin/src/client/mod.rs @@ -341,8 +341,7 @@ trait Handler { type Client: Client; fn handle(&mut self, client: &mut Self::Client) -> Res; - fn take_token(&mut self) -> Option; - fn has_token(&self) -> bool; + fn take_token(&mut self, client: &mut Self::Client) -> Option; } /// Network client, e.g. [`neqo_transport::Connection`] or [`neqo_http3::Http3Client`]. @@ -374,35 +373,20 @@ struct Runner<'a, H: Handler> { impl<'a, H: Handler> Runner<'a, H> { async fn run(mut self) -> Res> { - loop { + let close_reason = loop { let handler_done = self.handler.handle(&mut self.client)?; - - match (handler_done, self.args.resume, self.handler.has_token()) { - // Handler isn't done. Continue. - (false, _, _) => {}, - // Handler done. Resumption token needed but not present. Continue. - (true, true, false) => { - qdebug!("Handler done. Waiting for resumption token."); - } - // Handler is done, no resumption token needed. Close. - (true, false, _) | - // Handler is done, resumption token needed and present. Close. - (true, true, true) => { - self.client.close(Instant::now(), 0, "kthxbye!"); - } - } - self.process_output().await?; - if let Some(reason) = self.client.is_closed() { - if self.args.stats { - qinfo!("{:?}", self.client.stats()); + match (handler_done, self.client.is_closed()) { + // more work + (false, _) => {} + // no more work, closing connection + (true, None) => { + self.client.close(Instant::now(), 0, "kthxbye!"); + continue; } - return match reason { - ConnectionError::Transport(TransportError::NoError) - | ConnectionError::Application(0) => Ok(self.handler.take_token()), - _ => Err(reason.into()), - }; + // no more work, connection closed, terminating + (true, Some(reason)) => break reason, } if self.client.has_events() { @@ -415,6 +399,16 @@ impl<'a, H: Handler> Runner<'a, H> { self.timeout = None; } } + }; + + if self.args.stats { + qinfo!("{:?}", self.client.stats()); + } + + match close_reason { + ConnectionError::Transport(TransportError::NoError) + | ConnectionError::Application(0) => Ok(self.handler.take_token(&mut self.client)), + _ => Err(close_reason.into()), } } From 483e09e5c9aea455009d2eb4891fc6e2abcf7dad Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 18 Apr 2024 16:02:35 +0200 Subject: [PATCH 3/6] Trigger CI From 710c5008bf9d710ac44716e53777cf62c1448f09 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 18 Apr 2024 17:03:06 +0200 Subject: [PATCH 4/6] Still wait if there is none --- neqo-bin/src/client/http09.rs | 14 +++++++--- neqo-bin/src/client/http3.rs | 14 +++++++--- neqo-bin/src/client/mod.rs | 48 ++++++++++++++++++++--------------- 3 files changed, 47 insertions(+), 29 deletions(-) diff --git a/neqo-bin/src/client/http09.rs b/neqo-bin/src/client/http09.rs index 113d74ea0f..74f1dc431d 100644 --- a/neqo-bin/src/client/http09.rs +++ b/neqo-bin/src/client/http09.rs @@ -95,10 +95,16 @@ impl<'a> super::Handler for Handler<'a> { Ok(false) } - fn take_token(&mut self, client: &mut Self::Client) -> Option { - self.token - .take() - .or_else(|| client.take_resumption_token(Instant::now())) + fn take_token(&mut self) -> Option { + self.token.take() + } + + fn has_token(&mut self, client: &mut Self::Client) -> bool { + if self.token.is_some() { + return true; + } + self.token = client.take_resumption_token(Instant::now()); + self.token.is_some() } } diff --git a/neqo-bin/src/client/http3.rs b/neqo-bin/src/client/http3.rs index 5ca97eb761..61719fefbe 100644 --- a/neqo-bin/src/client/http3.rs +++ b/neqo-bin/src/client/http3.rs @@ -223,10 +223,16 @@ impl<'a> super::Handler for Handler<'a> { Ok(self.url_handler.done()) } - fn take_token(&mut self, client: &mut Self::Client) -> Option { - self.token - .take() - .or_else(|| client.take_resumption_token(Instant::now())) + fn take_token(&mut self) -> Option { + self.token.take() + } + + fn has_token(&mut self, client: &mut Self::Client) -> bool { + if self.token.is_some() { + return true; + } + self.token = client.take_resumption_token(Instant::now()); + self.token.is_some() } } diff --git a/neqo-bin/src/client/mod.rs b/neqo-bin/src/client/mod.rs index 2f49970031..7b9ed84c37 100644 --- a/neqo-bin/src/client/mod.rs +++ b/neqo-bin/src/client/mod.rs @@ -341,7 +341,8 @@ trait Handler { type Client: Client; fn handle(&mut self, client: &mut Self::Client) -> Res; - fn take_token(&mut self, client: &mut Self::Client) -> Option; + fn take_token(&mut self) -> Option; + fn has_token(&mut self, client: &mut Self::Client) -> bool; } /// Network client, e.g. [`neqo_transport::Connection`] or [`neqo_http3::Http3Client`]. @@ -373,20 +374,35 @@ struct Runner<'a, H: Handler> { impl<'a, H: Handler> Runner<'a, H> { async fn run(mut self) -> Res> { - let close_reason = loop { + loop { let handler_done = self.handler.handle(&mut self.client)?; - self.process_output().await?; - match (handler_done, self.client.is_closed()) { - // more work - (false, _) => {} - // no more work, closing connection - (true, None) => { + match (handler_done, self.args.resume, self.handler.has_token(&mut self.client)) { + // Handler isn't done. Continue. + (false, _, _) => {}, + // Handler done. Resumption token needed but not present. Continue. + (true, true, false) => { + qdebug!("Handler done. Waiting for resumption token."); + } + // Handler is done, no resumption token needed. Close. + (true, false, _) | + // Handler is done, resumption token needed and present. Close. + (true, true, true) => { self.client.close(Instant::now(), 0, "kthxbye!"); - continue; } - // no more work, connection closed, terminating - (true, Some(reason)) => break reason, + } + + self.process_output().await?; + + if let Some(reason) = self.client.is_closed() { + if self.args.stats { + qinfo!("{:?}", self.client.stats()); + } + return match reason { + ConnectionError::Transport(TransportError::NoError) + | ConnectionError::Application(0) => Ok(self.handler.take_token()), + _ => Err(reason.into()), + }; } if self.client.has_events() { @@ -399,16 +415,6 @@ impl<'a, H: Handler> Runner<'a, H> { self.timeout = None; } } - }; - - if self.args.stats { - qinfo!("{:?}", self.client.stats()); - } - - match close_reason { - ConnectionError::Transport(TransportError::NoError) - | ConnectionError::Application(0) => Ok(self.handler.take_token(&mut self.client)), - _ => Err(close_reason.into()), } } From 8bec58fdb4f656f6e7c3d0b6d39008e943be0652 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 18 Apr 2024 18:13:26 +0200 Subject: [PATCH 5/6] Revert "Still wait if there is none" This reverts commit 710c5008bf9d710ac44716e53777cf62c1448f09. --- neqo-bin/src/client/http09.rs | 14 +++------- neqo-bin/src/client/http3.rs | 14 +++------- neqo-bin/src/client/mod.rs | 48 +++++++++++++++-------------------- 3 files changed, 29 insertions(+), 47 deletions(-) diff --git a/neqo-bin/src/client/http09.rs b/neqo-bin/src/client/http09.rs index 74f1dc431d..113d74ea0f 100644 --- a/neqo-bin/src/client/http09.rs +++ b/neqo-bin/src/client/http09.rs @@ -95,16 +95,10 @@ impl<'a> super::Handler for Handler<'a> { Ok(false) } - fn take_token(&mut self) -> Option { - self.token.take() - } - - fn has_token(&mut self, client: &mut Self::Client) -> bool { - if self.token.is_some() { - return true; - } - self.token = client.take_resumption_token(Instant::now()); - self.token.is_some() + fn take_token(&mut self, client: &mut Self::Client) -> Option { + self.token + .take() + .or_else(|| client.take_resumption_token(Instant::now())) } } diff --git a/neqo-bin/src/client/http3.rs b/neqo-bin/src/client/http3.rs index 61719fefbe..5ca97eb761 100644 --- a/neqo-bin/src/client/http3.rs +++ b/neqo-bin/src/client/http3.rs @@ -223,16 +223,10 @@ impl<'a> super::Handler for Handler<'a> { Ok(self.url_handler.done()) } - fn take_token(&mut self) -> Option { - self.token.take() - } - - fn has_token(&mut self, client: &mut Self::Client) -> bool { - if self.token.is_some() { - return true; - } - self.token = client.take_resumption_token(Instant::now()); - self.token.is_some() + fn take_token(&mut self, client: &mut Self::Client) -> Option { + self.token + .take() + .or_else(|| client.take_resumption_token(Instant::now())) } } diff --git a/neqo-bin/src/client/mod.rs b/neqo-bin/src/client/mod.rs index 7b9ed84c37..2f49970031 100644 --- a/neqo-bin/src/client/mod.rs +++ b/neqo-bin/src/client/mod.rs @@ -341,8 +341,7 @@ trait Handler { type Client: Client; fn handle(&mut self, client: &mut Self::Client) -> Res; - fn take_token(&mut self) -> Option; - fn has_token(&mut self, client: &mut Self::Client) -> bool; + fn take_token(&mut self, client: &mut Self::Client) -> Option; } /// Network client, e.g. [`neqo_transport::Connection`] or [`neqo_http3::Http3Client`]. @@ -374,35 +373,20 @@ struct Runner<'a, H: Handler> { impl<'a, H: Handler> Runner<'a, H> { async fn run(mut self) -> Res> { - loop { + let close_reason = loop { let handler_done = self.handler.handle(&mut self.client)?; - - match (handler_done, self.args.resume, self.handler.has_token(&mut self.client)) { - // Handler isn't done. Continue. - (false, _, _) => {}, - // Handler done. Resumption token needed but not present. Continue. - (true, true, false) => { - qdebug!("Handler done. Waiting for resumption token."); - } - // Handler is done, no resumption token needed. Close. - (true, false, _) | - // Handler is done, resumption token needed and present. Close. - (true, true, true) => { - self.client.close(Instant::now(), 0, "kthxbye!"); - } - } - self.process_output().await?; - if let Some(reason) = self.client.is_closed() { - if self.args.stats { - qinfo!("{:?}", self.client.stats()); + match (handler_done, self.client.is_closed()) { + // more work + (false, _) => {} + // no more work, closing connection + (true, None) => { + self.client.close(Instant::now(), 0, "kthxbye!"); + continue; } - return match reason { - ConnectionError::Transport(TransportError::NoError) - | ConnectionError::Application(0) => Ok(self.handler.take_token()), - _ => Err(reason.into()), - }; + // no more work, connection closed, terminating + (true, Some(reason)) => break reason, } if self.client.has_events() { @@ -415,6 +399,16 @@ impl<'a, H: Handler> Runner<'a, H> { self.timeout = None; } } + }; + + if self.args.stats { + qinfo!("{:?}", self.client.stats()); + } + + match close_reason { + ConnectionError::Transport(TransportError::NoError) + | ConnectionError::Application(0) => Ok(self.handler.take_token(&mut self.client)), + _ => Err(close_reason.into()), } } From 6e85564a21a80ad352e5cc25bf08a4fb870ecc82 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 18 Apr 2024 18:32:50 +0200 Subject: [PATCH 6/6] Refactor resumption logic --- neqo-bin/src/client/http09.rs | 32 +++++++++++++++++++------------ neqo-bin/src/client/http3.rs | 18 ++++++++++-------- neqo-bin/src/client/mod.rs | 36 +++++++++++++++++------------------ 3 files changed, 48 insertions(+), 38 deletions(-) diff --git a/neqo-bin/src/client/http09.rs b/neqo-bin/src/client/http09.rs index 113d74ea0f..e9de5915a7 100644 --- a/neqo-bin/src/client/http09.rs +++ b/neqo-bin/src/client/http09.rs @@ -87,18 +87,22 @@ impl<'a> super::Handler for Handler<'a> { } } - if self.streams.is_empty() && self.url_queue.is_empty() { - // Handler is done. - return Ok(true); + if !self.streams.is_empty() || !self.url_queue.is_empty() { + return Ok(false); } - Ok(false) + if self.args.resume && self.token.is_none() { + let Some(token) = client.take_resumption_token(Instant::now()) else { + return Ok(false); + }; + self.token = Some(token); + } + + Ok(true) } - fn take_token(&mut self, client: &mut Self::Client) -> Option { - self.token - .take() - .or_else(|| client.take_resumption_token(Instant::now())) + fn take_token(&mut self) -> Option { + self.token.take() } } @@ -159,11 +163,15 @@ impl super::Client for Connection { } } - fn is_closed(&self) -> Option { - if let State::Closed(err) = self.state() { - return Some(err.clone()); + fn is_closed(&self) -> Result { + match self.state() { + State::Closed( + ConnectionError::Transport(neqo_transport::Error::NoError) + | ConnectionError::Application(0), + ) => Ok(true), + State::Closed(err) => Err(err.clone()), + _ => Ok(false), } - None } fn stats(&self) -> neqo_transport::Stats { diff --git a/neqo-bin/src/client/http3.rs b/neqo-bin/src/client/http3.rs index 5ca97eb761..5a77c92f0b 100644 --- a/neqo-bin/src/client/http3.rs +++ b/neqo-bin/src/client/http3.rs @@ -106,11 +106,15 @@ pub(crate) fn create_client( } impl super::Client for Http3Client { - fn is_closed(&self) -> Option { - if let Http3State::Closed(err) = self.state() { - return Some(err); + fn is_closed(&self) -> Result { + match self.state() { + Http3State::Closed( + ConnectionError::Transport(neqo_transport::Error::NoError) + | ConnectionError::Application(0), + ) => Ok(true), + Http3State::Closed(err) => Err(err.clone()), + _ => Ok(false), } - None } fn process_output(&mut self, now: Instant) -> Output { @@ -223,10 +227,8 @@ impl<'a> super::Handler for Handler<'a> { Ok(self.url_handler.done()) } - fn take_token(&mut self, client: &mut Self::Client) -> Option { - self.token - .take() - .or_else(|| client.take_resumption_token(Instant::now())) + fn take_token(&mut self) -> Option { + self.token.take() } } diff --git a/neqo-bin/src/client/mod.rs b/neqo-bin/src/client/mod.rs index 2f49970031..fd77785c20 100644 --- a/neqo-bin/src/client/mod.rs +++ b/neqo-bin/src/client/mod.rs @@ -27,7 +27,7 @@ use neqo_crypto::{ init, Cipher, ResumptionToken, }; use neqo_http3::Output; -use neqo_transport::{AppError, ConnectionError, ConnectionId, Error as TransportError, Version}; +use neqo_transport::{AppError, ConnectionError, ConnectionId, Version}; use qlog::{events::EventImportance, streamer::QlogStreamer}; use tokio::time::Sleep; use url::{Origin, Url}; @@ -137,7 +137,7 @@ pub struct Args { /// Save contents of fetched URLs to a directory output_dir: Option, - #[arg(short = 'r', long)] + #[arg(short = 'r', long, hide = true)] /// Client attempts to resume by making multiple connections to servers. /// Requires that 2 or more URLs are listed for each server. /// Use this for 0-RTT: the stack always attempts 0-RTT on resumption. @@ -227,6 +227,11 @@ impl Args { exit(127) } + if self.resume { + qerror!("internal option resume set by user"); + exit(127) + } + // Only use v1 for most QNS tests. self.shared.quic_parameters.quic_version = vec![Version::Version1]; match testcase.as_str() { @@ -341,7 +346,7 @@ trait Handler { type Client: Client; fn handle(&mut self, client: &mut Self::Client) -> Res; - fn take_token(&mut self, client: &mut Self::Client) -> Option; + fn take_token(&mut self) -> Option; } /// Network client, e.g. [`neqo_transport::Connection`] or [`neqo_http3::Http3Client`]. @@ -358,7 +363,7 @@ trait Client { /// /// Note that connection was closed without error on /// [`Some(ConnectionError::Transport(TransportError::NoError))`]. - fn is_closed(&self) -> Option; + fn is_closed(&self) -> Result; fn stats(&self) -> neqo_transport::Stats; } @@ -373,24 +378,23 @@ struct Runner<'a, H: Handler> { impl<'a, H: Handler> Runner<'a, H> { async fn run(mut self) -> Res> { - let close_reason = loop { + loop { let handler_done = self.handler.handle(&mut self.client)?; self.process_output().await?; + if self.client.has_events() { + continue; + } - match (handler_done, self.client.is_closed()) { + match (handler_done, self.client.is_closed()?) { // more work (false, _) => {} // no more work, closing connection - (true, None) => { + (true, false) => { self.client.close(Instant::now(), 0, "kthxbye!"); continue; } // no more work, connection closed, terminating - (true, Some(reason)) => break reason, - } - - if self.client.has_events() { - continue; + (true, true) => break, } match ready(self.socket, self.timeout.as_mut()).await? { @@ -399,17 +403,13 @@ impl<'a, H: Handler> Runner<'a, H> { self.timeout = None; } } - }; + } if self.args.stats { qinfo!("{:?}", self.client.stats()); } - match close_reason { - ConnectionError::Transport(TransportError::NoError) - | ConnectionError::Application(0) => Ok(self.handler.take_token(&mut self.client)), - _ => Err(close_reason.into()), - } + Ok(self.handler.take_token()) } async fn process_output(&mut self) -> Result<(), io::Error> {