From 7cc1be5deba40b7703cc6dc6ed7cb36d8d396c7c Mon Sep 17 00:00:00 2001 From: Dan Robertson Date: Sat, 21 Jan 2023 00:01:26 +0000 Subject: [PATCH 1/2] Add headers to session_start and session_end - Add a headers argument to session_start - Add an optional headers argument to session_end --- neqo-http3/src/client_events.rs | 18 +++++++- neqo-http3/src/connection_client.rs | 3 +- .../src/features/extended_connect/mod.rs | 10 +++- .../tests/webtransport/mod.rs | 30 +++++++++--- .../tests/webtransport/sessions.rs | 46 ++++++++++++++----- .../extended_connect/webtransport_session.rs | 7 +++ neqo-http3/src/server.rs | 6 ++- neqo-http3/src/server_connection_events.rs | 4 ++ neqo-http3/src/server_events.rs | 8 +++- neqo-http3/tests/webtransport.rs | 11 +++-- 10 files changed, 117 insertions(+), 26 deletions(-) diff --git a/neqo-http3/src/client_events.rs b/neqo-http3/src/client_events.rs index e17a29c854..b4fdde8e13 100644 --- a/neqo-http3/src/client_events.rs +++ b/neqo-http3/src/client_events.rs @@ -26,10 +26,12 @@ pub enum WebTransportEvent { Session { stream_id: StreamId, status: u16, + headers: Vec
, }, SessionClosed { stream_id: StreamId, reason: SessionCloseReason, + headers: Option>, }, NewStream { stream_id: StreamId, @@ -181,11 +183,18 @@ impl SendStreamEvents for Http3ClientEvents { } impl ExtendedConnectEvents for Http3ClientEvents { - fn session_start(&self, connect_type: ExtendedConnectType, stream_id: StreamId, status: u16) { + fn session_start( + &self, + connect_type: ExtendedConnectType, + stream_id: StreamId, + status: u16, + headers: Vec
, + ) { if connect_type == ExtendedConnectType::WebTransport { self.insert(Http3ClientEvent::WebTransport(WebTransportEvent::Session { stream_id, status, + headers, })); } else { unreachable!("There is only ExtendedConnectType::WebTransport."); @@ -197,10 +206,15 @@ impl ExtendedConnectEvents for Http3ClientEvents { connect_type: ExtendedConnectType, stream_id: StreamId, reason: SessionCloseReason, + headers: Option>, ) { if connect_type == ExtendedConnectType::WebTransport { self.insert(Http3ClientEvent::WebTransport( - WebTransportEvent::SessionClosed { stream_id, reason }, + WebTransportEvent::SessionClosed { + stream_id, + reason, + headers, + }, )); } else { unreachable!("There are no other types."); diff --git a/neqo-http3/src/connection_client.rs b/neqo-http3/src/connection_client.rs index 2735cf44fd..709a1e0d6b 100644 --- a/neqo-http3/src/connection_client.rs +++ b/neqo-http3/src/connection_client.rs @@ -182,7 +182,8 @@ fn alpn_from_quic_version(version: Version) -> &'static str { /// match event { /// Http3ClientEvent::WebTransport(WebTransportEvent::Session{ /// stream_id, -/// status +/// status, +/// .. /// }) => { /// println!("The response from the server: WebTransport session ID {:?} status={:?}", /// stream_id, diff --git a/neqo-http3/src/features/extended_connect/mod.rs b/neqo-http3/src/features/extended_connect/mod.rs index 7fa1241f9b..6be92dabba 100644 --- a/neqo-http3/src/features/extended_connect/mod.rs +++ b/neqo-http3/src/features/extended_connect/mod.rs @@ -13,6 +13,7 @@ use crate::client_events::Http3ClientEvents; use crate::features::NegotiationState; use crate::settings::{HSettingType, HSettings}; use crate::{CloseType, Http3StreamInfo, Http3StreamType}; +use neqo_common::Header; use neqo_transport::{AppError, StreamId}; use std::fmt::Debug; pub(crate) use webtransport_session::WebTransportSession; @@ -39,12 +40,19 @@ impl From for SessionCloseReason { } pub(crate) trait ExtendedConnectEvents: Debug { - fn session_start(&self, connect_type: ExtendedConnectType, stream_id: StreamId, status: u16); + fn session_start( + &self, + connect_type: ExtendedConnectType, + stream_id: StreamId, + status: u16, + headers: Vec
, + ); fn session_end( &self, connect_type: ExtendedConnectType, stream_id: StreamId, reason: SessionCloseReason, + headers: Option>, ); fn extended_connect_new_stream(&self, stream_info: Http3StreamInfo); fn new_datagram(&self, session_id: StreamId, datagram: Vec); diff --git a/neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs b/neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs index 2e073f6650..58ebfd8a6c 100644 --- a/neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs +++ b/neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs @@ -11,7 +11,7 @@ mod streams; use neqo_common::event::Provider; use crate::{ - features::extended_connect::SessionCloseReason, Error, Http3Client, Http3ClientEvent, + features::extended_connect::SessionCloseReason, Error, Header, Http3Client, Http3ClientEvent, Http3OrWebTransportStream, Http3Parameters, Http3Server, Http3ServerEvent, Http3State, WebTransportEvent, WebTransportRequest, WebTransportServerEvent, WebTransportSessionAcceptAction, @@ -177,8 +177,13 @@ impl WtTest { e, Http3ClientEvent::WebTransport(WebTransportEvent::Session{ stream_id, - status - }) if stream_id == wt_session_id && status == 200 + status, + headers, + }) if ( + stream_id == wt_session_id && + status == 200 && + headers.contains(&Header::new(":status", "200")) + ) ) }; assert!(self.client.events().any(wt_session_negotiated_event)); @@ -210,13 +215,15 @@ impl WtTest { e: &Http3ClientEvent, id: StreamId, expected_reason: &SessionCloseReason, + expected_headers: &Option>, ) -> bool { if let Http3ClientEvent::WebTransport(WebTransportEvent::SessionClosed { stream_id, reason, + headers, }) = e { - *stream_id == id && reason == expected_reason + *stream_id == id && reason == expected_reason && headers == expected_headers } else { false } @@ -226,11 +233,17 @@ impl WtTest { &mut self, wt_session_id: StreamId, expected_reason: &SessionCloseReason, + expected_headers: &Option>, ) { let mut event_found = false; while let Some(event) = self.client.next_event() { - event_found = WtTest::session_closed_client(&event, wt_session_id, expected_reason); + event_found = WtTest::session_closed_client( + &event, + wt_session_id, + expected_reason, + expected_headers, + ); if event_found { break; } @@ -251,9 +264,10 @@ impl WtTest { if let Http3ServerEvent::WebTransport(WebTransportServerEvent::SessionClosed { session, reason, + headers, }) = e { - session.stream_id() == id && reason == expected_reason + session.stream_id() == id && reason == expected_reason && headers.is_none() } else { false } @@ -401,10 +415,12 @@ impl WtTest { Http3ClientEvent::WebTransport(WebTransportEvent::SessionClosed { stream_id, reason, + headers, }) => { close_event = true; assert_eq!(stream_id, expected_session_close.as_ref().unwrap().0); assert_eq!(expected_session_close.as_ref().unwrap().1, reason); + assert!(headers.is_none()); } _ => {} } @@ -536,6 +552,7 @@ impl WtTest { Http3ServerEvent::WebTransport(WebTransportServerEvent::SessionClosed { session, reason, + headers, }) => { close_event = true; assert_eq!( @@ -543,6 +560,7 @@ impl WtTest { expected_session_close.as_ref().unwrap().0 ); assert_eq!(expected_session_close.as_ref().unwrap().1, reason); + assert!(headers.is_none()); } _ => {} } diff --git a/neqo-http3/src/features/extended_connect/tests/webtransport/sessions.rs b/neqo-http3/src/features/extended_connect/tests/webtransport/sessions.rs index 5d7c87baff..65572a1c2a 100644 --- a/neqo-http3/src/features/extended_connect/tests/webtransport/sessions.rs +++ b/neqo-http3/src/features/extended_connect/tests/webtransport/sessions.rs @@ -26,11 +26,15 @@ fn wt_session() { #[test] fn wt_session_reject() { let mut wt = WtTest::new(); - let accept_res = - WebTransportSessionAcceptAction::Reject([Header::new(":status", "404")].to_vec()); + let headers = vec![Header::new(":status", "404")]; + let accept_res = WebTransportSessionAcceptAction::Reject(headers.clone()); let (wt_session_id, _wt_session) = wt.negotiate_wt_session(&accept_res); - wt.check_session_closed_event_client(wt_session_id, &SessionCloseReason::Status(404)); + wt.check_session_closed_event_client( + wt_session_id, + &SessionCloseReason::Status(404), + &Some(headers), + ); } #[test] @@ -54,6 +58,7 @@ fn wt_session_close_server() { wt.check_session_closed_event_client( wt_session.stream_id(), &SessionCloseReason::Error(Error::HttpNoError.code()), + &None, ); } @@ -70,6 +75,7 @@ fn wt_session_close_server_close_send() { error: 0, message: String::new(), }, + &None, ); } @@ -85,6 +91,7 @@ fn wt_session_close_server_stop_sending() { wt.check_session_closed_event_client( wt_session.stream_id(), &SessionCloseReason::Error(Error::HttpNoError.code()), + &None, ); } @@ -100,6 +107,7 @@ fn wt_session_close_server_reset() { wt.check_session_closed_event_client( wt_session.stream_id(), &SessionCloseReason::Error(Error::HttpNoError.code()), + &None, ); } @@ -149,8 +157,13 @@ fn wt_session_response_with_1xx() { e, Http3ClientEvent::WebTransport(WebTransportEvent::Session{ stream_id, - status - }) if stream_id == wt_session_id && status == 200 + status, + headers, + }) if ( + stream_id == wt_session_id && + status == 200 && + headers.contains(&Header::new(":status", "200")) + ) ) }; assert!(wt.client.events().any(wt_session_negotiated_event)); @@ -160,15 +173,18 @@ fn wt_session_response_with_1xx() { #[test] fn wt_session_response_with_redirect() { + let headers = [Header::new(":status", "302"), Header::new("location", "/")].to_vec(); let mut wt = WtTest::new(); - let accept_res = WebTransportSessionAcceptAction::Reject( - [Header::new(":status", "302"), Header::new("location", "/")].to_vec(), - ); + let accept_res = WebTransportSessionAcceptAction::Reject(headers.clone()); let (wt_session_id, _wt_session) = wt.negotiate_wt_session(&accept_res); - wt.check_session_closed_event_client(wt_session_id, &SessionCloseReason::Status(302)); + wt.check_session_closed_event_client( + wt_session_id, + &SessionCloseReason::Status(302), + &Some(headers), + ); } #[test] @@ -212,8 +228,14 @@ fn wt_session_respone_200_with_fin() { e, Http3ClientEvent::WebTransport(WebTransportEvent::SessionClosed{ stream_id, - reason - }) if stream_id == wt_session_id && reason == SessionCloseReason::Clean{ error: 0, message: String::new()} + reason, + headers, + .. + }) if ( + stream_id == wt_session_id && + reason == SessionCloseReason::Clean{ error: 0, message: String::new()} && + headers.is_none() + ) ) }; assert!(wt.client.events().any(wt_session_close_event)); @@ -256,6 +278,7 @@ fn wt_session_close_frame_server() { error: ERROR_NUM, message: ERROR_MESSAGE.to_string(), }, + &None, ); } @@ -331,6 +354,7 @@ fn wt_close_session_frame_broken_client() { wt.check_session_closed_event_client( wt_session.stream_id(), &SessionCloseReason::Error(Error::HttpGeneralProtocolStream.code()), + &None, ); wt.check_session_closed_event_server( &mut wt_session, diff --git a/neqo-http3/src/features/extended_connect/webtransport_session.rs b/neqo-http3/src/features/extended_connect/webtransport_session.rs index eb005a5898..4a412dd27e 100644 --- a/neqo-http3/src/features/extended_connect/webtransport_session.rs +++ b/neqo-http3/src/features/extended_connect/webtransport_session.rs @@ -214,6 +214,7 @@ impl WebTransportSession { ExtendedConnectType::WebTransport, self.session_id, SessionCloseReason::from(close_type), + None, ); } } @@ -242,6 +243,7 @@ impl WebTransportSession { error: 0, message: String::new(), }, + Some(headers), ); self.state = SessionState::Done; } @@ -266,6 +268,7 @@ impl WebTransportSession { error: 0, message: String::new(), }, + Some(headers), ); SessionState::Done } else { @@ -273,6 +276,7 @@ impl WebTransportSession { ExtendedConnectType::WebTransport, self.session_id, status, + headers, ); SessionState::Active } @@ -281,6 +285,7 @@ impl WebTransportSession { ExtendedConnectType::WebTransport, self.session_id, SessionCloseReason::Status(status), + Some(headers), ); SessionState::Done }; @@ -345,6 +350,7 @@ impl WebTransportSession { ExtendedConnectType::WebTransport, self.session_id, SessionCloseReason::Clean { error, message }, + None, ); self.state = if fin { SessionState::Done @@ -359,6 +365,7 @@ impl WebTransportSession { error: 0, message: String::new(), }, + None, ); self.state = SessionState::Done; } diff --git a/neqo-http3/src/server.rs b/neqo-http3/src/server.rs index 474e2907a6..e28b72be4b 100644 --- a/neqo-http3/src/server.rs +++ b/neqo-http3/src/server.rs @@ -217,10 +217,14 @@ impl Http3Server { ); } Http3ServerConnEvent::ExtendedConnectClosed { - stream_id, reason, .. + stream_id, + reason, + headers, + .. } => self.events.webtransport_session_closed( WebTransportRequest::new(conn.clone(), handler.clone(), stream_id), reason, + headers, ), Http3ServerConnEvent::ExtendedConnectNewStream(stream_info) => self .events diff --git a/neqo-http3/src/server_connection_events.rs b/neqo-http3/src/server_connection_events.rs index bc1f1b0376..f56288e204 100644 --- a/neqo-http3/src/server_connection_events.rs +++ b/neqo-http3/src/server_connection_events.rs @@ -53,6 +53,7 @@ pub(crate) enum Http3ServerConnEvent { connect_type: ExtendedConnectType, stream_id: StreamId, reason: SessionCloseReason, + headers: Option>, }, ExtendedConnectNewStream(Http3StreamInfo), ExtendedConnectDatagram { @@ -125,6 +126,7 @@ impl ExtendedConnectEvents for Http3ServerConnEvents { _connect_type: ExtendedConnectType, _stream_id: StreamId, _status: u16, + _headers: Vec
, ) { } @@ -133,11 +135,13 @@ impl ExtendedConnectEvents for Http3ServerConnEvents { connect_type: ExtendedConnectType, stream_id: StreamId, reason: SessionCloseReason, + headers: Option>, ) { self.insert(Http3ServerConnEvent::ExtendedConnectClosed { connect_type, stream_id, reason, + headers, }); } diff --git a/neqo-http3/src/server_events.rs b/neqo-http3/src/server_events.rs index 7f3d9ac5dd..e0cc84ed4c 100644 --- a/neqo-http3/src/server_events.rs +++ b/neqo-http3/src/server_events.rs @@ -378,6 +378,7 @@ pub enum WebTransportServerEvent { SessionClosed { session: WebTransportRequest, reason: SessionCloseReason, + headers: Option>, }, NewStream(Http3OrWebTransportStream), Datagram { @@ -541,9 +542,14 @@ impl Http3ServerEvents { &self, session: WebTransportRequest, reason: SessionCloseReason, + headers: Option>, ) { self.insert(Http3ServerEvent::WebTransport( - WebTransportServerEvent::SessionClosed { session, reason }, + WebTransportServerEvent::SessionClosed { + session, + reason, + headers, + }, )); } diff --git a/neqo-http3/tests/webtransport.rs b/neqo-http3/tests/webtransport.rs index f5ba76ced8..e0556708f1 100644 --- a/neqo-http3/tests/webtransport.rs +++ b/neqo-http3/tests/webtransport.rs @@ -4,7 +4,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use neqo_common::event::Provider; +use neqo_common::{event::Provider, Header}; use neqo_crypto::AuthenticationStatus; use neqo_http3::{ Http3Client, Http3ClientEvent, Http3OrWebTransportStream, Http3Parameters, Http3Server, @@ -122,8 +122,13 @@ fn create_wt_session(client: &mut Http3Client, server: &mut Http3Server) -> WebT e, Http3ClientEvent::WebTransport(WebTransportEvent::Session{ stream_id, - status - }) if stream_id == wt_session_id && status == 200 + status, + headers, + }) if ( + stream_id == wt_session_id && + status == 200 && + headers.contains(&Header::new(":status", "200")) + ) ) }; assert!(client.events().any(wt_session_negotiated_event)); From bc8375c98b1bd221d5ea053379edb914459298fc Mon Sep 17 00:00:00 2001 From: Dan Robertson Date: Wed, 1 Feb 2023 00:07:49 +0000 Subject: [PATCH 2/2] Fix clippy warning Fix clippy warning for duration subtraction. --- neqo-common/src/timer.rs | 5 +++-- neqo-transport/src/cc/tests/cubic.rs | 2 +- neqo-transport/src/cc/tests/new_reno.rs | 2 +- neqo-transport/src/connection/tests/idle.rs | 5 ++++- neqo-transport/src/tracking.rs | 20 +++++++++++++++----- 5 files changed, 24 insertions(+), 10 deletions(-) diff --git a/neqo-common/src/timer.rs b/neqo-common/src/timer.rs index 442f72261b..66e759fbc6 100644 --- a/neqo-common/src/timer.rs +++ b/neqo-common/src/timer.rs @@ -120,7 +120,7 @@ impl Timer { for i in &self.items { debug_assert!(i.is_empty()); } - self.now = time - short_span; + self.now = time.checked_sub(short_span).unwrap(); self.cursor = 0; } @@ -287,7 +287,8 @@ mod test { t.add(near_future, v); assert_eq!(near_future, t.next_time().expect("should return a value")); assert_eq!( - t.take_until(near_future - Duration::from_millis(1)).count(), + t.take_until(near_future.checked_sub(Duration::from_millis(1)).unwrap()) + .count(), 0 ); assert!(t diff --git a/neqo-transport/src/cc/tests/cubic.rs b/neqo-transport/src/cc/tests/cubic.rs index 1e156e5191..c8d7fe58de 100644 --- a/neqo-transport/src/cc/tests/cubic.rs +++ b/neqo-transport/src/cc/tests/cubic.rs @@ -320,5 +320,5 @@ fn congestion_event_congestion_avoidance_test_no_overflow() { assert_eq!(cubic.cwnd(), CWND_INITIAL); // Now ack packet that was send earlier. - ack_packet(&mut cubic, 0, now() - PTO); + ack_packet(&mut cubic, 0, now().checked_sub(PTO).unwrap()); } diff --git a/neqo-transport/src/cc/tests/new_reno.rs b/neqo-transport/src/cc/tests/new_reno.rs index 6376e64bff..0b678ca55e 100644 --- a/neqo-transport/src/cc/tests/new_reno.rs +++ b/neqo-transport/src/cc/tests/new_reno.rs @@ -31,7 +31,7 @@ fn cwnd_is_halved(cc: &ClassicCongestionControl) { fn issue_876() { let mut cc = ClassicCongestionControl::new(NewReno::default()); let time_now = now(); - let time_before = time_now - Duration::from_millis(100); + let time_before = time_now.checked_sub(Duration::from_millis(100)).unwrap(); let time_after = time_now + Duration::from_millis(150); let sent_packets = &[ diff --git a/neqo-transport/src/connection/tests/idle.rs b/neqo-transport/src/connection/tests/idle.rs index 22e9c65ac3..f8f394e030 100644 --- a/neqo-transport/src/connection/tests/idle.rs +++ b/neqo-transport/src/connection/tests/idle.rs @@ -35,7 +35,10 @@ fn test_idle_timeout(client: &mut Connection, server: &mut Connection, timeout: assert_eq!(res, Output::Callback(timeout)); // Still connected after timeout-1 seconds. Idle timer not reset - mem::drop(client.process(None, now + timeout - Duration::from_secs(1))); + mem::drop(client.process( + None, + now + timeout.checked_sub(Duration::from_secs(1)).unwrap(), + )); assert!(matches!(client.state(), State::Confirmed)); mem::drop(client.process(None, now + timeout)); diff --git a/neqo-transport/src/tracking.rs b/neqo-transport/src/tracking.rs index ad928ac0b0..512620904a 100644 --- a/neqo-transport/src/tracking.rs +++ b/neqo-transport/src/tracking.rs @@ -1019,7 +1019,9 @@ mod tests { .unwrap() .set_received(*NOW, 0, true); // The reference time for `ack_time` has to be in the past or we filter out the timer. - assert!(tracker.ack_time(*NOW - Duration::from_millis(1)).is_some()); + assert!(tracker + .ack_time(NOW.checked_sub(Duration::from_millis(1)).unwrap()) + .is_some()); let mut tokens = Vec::new(); let mut stats = FrameStats::default(); @@ -1039,13 +1041,17 @@ mod tests { .get_mut(PacketNumberSpace::Initial) .unwrap() .set_received(*NOW, 1, true); - assert!(tracker.ack_time(*NOW - Duration::from_millis(1)).is_some()); + assert!(tracker + .ack_time(NOW.checked_sub(Duration::from_millis(1)).unwrap()) + .is_some()); // Now drop that space. tracker.drop_space(PacketNumberSpace::Initial); assert!(tracker.get_mut(PacketNumberSpace::Initial).is_none()); - assert!(tracker.ack_time(*NOW - Duration::from_millis(1)).is_none()); + assert!(tracker + .ack_time(NOW.checked_sub(Duration::from_millis(1)).unwrap()) + .is_none()); tracker .write_frame( PacketNumberSpace::Initial, @@ -1070,7 +1076,9 @@ mod tests { .get_mut(PacketNumberSpace::Initial) .unwrap() .set_received(*NOW, 0, true); - assert!(tracker.ack_time(*NOW - Duration::from_millis(1)).is_some()); + assert!(tracker + .ack_time(NOW.checked_sub(Duration::from_millis(1)).unwrap()) + .is_some()); let mut builder = PacketBuilder::short(Encoder::new(), false, []); builder.set_limit(10); @@ -1100,7 +1108,9 @@ mod tests { .get_mut(PacketNumberSpace::Initial) .unwrap() .set_received(*NOW, 2, true); - assert!(tracker.ack_time(*NOW - Duration::from_millis(1)).is_some()); + assert!(tracker + .ack_time(NOW.checked_sub(Duration::from_millis(1)).unwrap()) + .is_some()); let mut builder = PacketBuilder::short(Encoder::new(), false, []); builder.set_limit(32);