From 51a0019cab6270a583e61116783cd96630727e33 Mon Sep 17 00:00:00 2001 From: Madeleine Date: Tue, 2 May 2023 09:58:30 -0700 Subject: [PATCH 1/5] Translated error --- .../connection/connection_container/tests.rs | 4 +++ .../src/connection/connection_impl.rs | 25 ++++++++++++++++++- .../src/connection/connection_trait.rs | 2 ++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/quic/s2n-quic-transport/src/connection/connection_container/tests.rs b/quic/s2n-quic-transport/src/connection/connection_container/tests.rs index 13d8a25dec..3aee25ec97 100644 --- a/quic/s2n-quic-transport/src/connection/connection_container/tests.rs +++ b/quic/s2n-quic-transport/src/connection/connection_container/tests.rs @@ -306,6 +306,10 @@ impl connection::Trait for TestConnection { None } + fn translate_connection_err(&self, _error: connection::Error) -> Result<(), connection::Error> { + todo!() + } + fn query_event_context(&self, _query: &mut dyn query::Query) { todo!() } diff --git a/quic/s2n-quic-transport/src/connection/connection_impl.rs b/quic/s2n-quic-transport/src/connection/connection_impl.rs index 02c2d57103..ccac7d2869 100644 --- a/quic/s2n-quic-transport/src/connection/connection_impl.rs +++ b/quic/s2n-quic-transport/src/connection/connection_impl.rs @@ -1656,6 +1656,24 @@ impl connection::Trait for ConnectionImpl { self.accept_state = AcceptState::Active; } + // Translates protocol errors into more useful user errors + fn translate_connection_err(&self, error: connection::Error) -> Result<(), connection::Error> { + match error { + // The connection closed without an error + connection::Error::Closed { .. } => return Ok(()), + // The application closed the connection + connection::Error::Transport { code, .. } + if code == transport::Error::APPLICATION_ERROR.code => + { + return Ok(()) + } + // The local connection's idle timer expired + connection::Error::IdleTimerExpired { .. } => return Ok(()), + // Otherwise return the real error to the user + _=> return Err(error), + } + } + fn interests(&self) -> ConnectionInterests { use crate::connection::finalization::Provider as _; use timer::Provider as _; @@ -1742,7 +1760,12 @@ impl connection::Trait for ConnectionImpl { stream_type: Option, context: &Context, ) -> Poll, connection::Error>> { - self.error?; + if let Err(error) = self.error { + match self.translate_connection_err(error) { + Ok(_) => return Ok(None).into(), + Err(err) => return Err(err).into(), + }; + } let (space, _) = self .space_manager diff --git a/quic/s2n-quic-transport/src/connection/connection_trait.rs b/quic/s2n-quic-transport/src/connection/connection_trait.rs index 23a45a6a27..395e7e8e6a 100644 --- a/quic/s2n-quic-transport/src/connection/connection_trait.rs +++ b/quic/s2n-quic-transport/src/connection/connection_trait.rs @@ -444,6 +444,8 @@ pub trait ConnectionTrait: 'static + Send + Sized { fn error(&self) -> Option; + fn translate_connection_err(&self, error: connection::Error) -> Result<(), connection::Error>; + fn query_event_context(&self, query: &mut dyn query::Query); fn query_event_context_mut(&mut self, query: &mut dyn query::QueryMut); From 5d9d85f98353238d2e72a617cf5d9ef85fb6cb6d Mon Sep 17 00:00:00 2001 From: Madeleine Date: Tue, 2 May 2023 10:30:44 -0700 Subject: [PATCH 2/5] cargo fmt --- quic/s2n-quic-transport/src/connection/connection_impl.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/quic/s2n-quic-transport/src/connection/connection_impl.rs b/quic/s2n-quic-transport/src/connection/connection_impl.rs index ccac7d2869..17ef3800d3 100644 --- a/quic/s2n-quic-transport/src/connection/connection_impl.rs +++ b/quic/s2n-quic-transport/src/connection/connection_impl.rs @@ -1670,7 +1670,7 @@ impl connection::Trait for ConnectionImpl { // The local connection's idle timer expired connection::Error::IdleTimerExpired { .. } => return Ok(()), // Otherwise return the real error to the user - _=> return Err(error), + _ => return Err(error), } } From e033103e1085fbd93a02ad060a27a599c35d482e Mon Sep 17 00:00:00 2001 From: Madeleine Date: Tue, 2 May 2023 10:46:58 -0700 Subject: [PATCH 3/5] cargo clippy --- quic/s2n-quic-transport/src/connection/connection_impl.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/quic/s2n-quic-transport/src/connection/connection_impl.rs b/quic/s2n-quic-transport/src/connection/connection_impl.rs index 17ef3800d3..570c5fa312 100644 --- a/quic/s2n-quic-transport/src/connection/connection_impl.rs +++ b/quic/s2n-quic-transport/src/connection/connection_impl.rs @@ -1660,17 +1660,17 @@ impl connection::Trait for ConnectionImpl { fn translate_connection_err(&self, error: connection::Error) -> Result<(), connection::Error> { match error { // The connection closed without an error - connection::Error::Closed { .. } => return Ok(()), + connection::Error::Closed { .. } => Ok(()), // The application closed the connection connection::Error::Transport { code, .. } if code == transport::Error::APPLICATION_ERROR.code => { - return Ok(()) + Ok(()) } // The local connection's idle timer expired - connection::Error::IdleTimerExpired { .. } => return Ok(()), + connection::Error::IdleTimerExpired { .. } => Ok(()), // Otherwise return the real error to the user - _ => return Err(error), + _ => Err(error), } } From 02dd073f0be15c373d1b212968b90732511f2fc1 Mon Sep 17 00:00:00 2001 From: Madeleine Date: Tue, 2 May 2023 11:25:32 -0700 Subject: [PATCH 4/5] PR feedback --- quic/s2n-quic-core/src/connection/error.rs | 19 ++++++++++++++++ .../connection/connection_container/tests.rs | 4 ---- .../src/connection/connection_impl.rs | 22 ++----------------- .../src/connection/connection_trait.rs | 2 -- quic/s2n-quic-transport/src/stream/manager.rs | 20 +++++------------ 5 files changed, 27 insertions(+), 40 deletions(-) diff --git a/quic/s2n-quic-core/src/connection/error.rs b/quic/s2n-quic-core/src/connection/error.rs index 5185518d9f..1c0957e57d 100644 --- a/quic/s2n-quic-core/src/connection/error.rs +++ b/quic/s2n-quic-core/src/connection/error.rs @@ -281,6 +281,25 @@ impl Error { let source = panic::Location::caller(); Error::Unspecified { source } } + + #[inline] + #[doc(hidden)] + pub fn into_accept_error(error: connection::Error) -> Result<(), connection::Error> { + match error { + // The connection closed without an error + connection::Error::Closed { .. } => Ok(()), + // The application closed the connection + connection::Error::Transport { code, .. } + if code == transport::Error::APPLICATION_ERROR.code => + { + Ok(()) + } + // The local connection's idle timer expired + connection::Error::IdleTimerExpired { .. } => Ok(()), + // Otherwise return the real error to the user + _ => Err(error), + } + } } /// Returns a CONNECTION_CLOSE frame for the given connection Error, if any diff --git a/quic/s2n-quic-transport/src/connection/connection_container/tests.rs b/quic/s2n-quic-transport/src/connection/connection_container/tests.rs index 3aee25ec97..13d8a25dec 100644 --- a/quic/s2n-quic-transport/src/connection/connection_container/tests.rs +++ b/quic/s2n-quic-transport/src/connection/connection_container/tests.rs @@ -306,10 +306,6 @@ impl connection::Trait for TestConnection { None } - fn translate_connection_err(&self, _error: connection::Error) -> Result<(), connection::Error> { - todo!() - } - fn query_event_context(&self, _query: &mut dyn query::Query) { todo!() } diff --git a/quic/s2n-quic-transport/src/connection/connection_impl.rs b/quic/s2n-quic-transport/src/connection/connection_impl.rs index 570c5fa312..59e029d6ee 100644 --- a/quic/s2n-quic-transport/src/connection/connection_impl.rs +++ b/quic/s2n-quic-transport/src/connection/connection_impl.rs @@ -34,7 +34,7 @@ use core::{ use s2n_quic_core::{ application, application::ServerName, - connection::{id::Generator as _, InitialId, PeerId}, + connection::{id::Generator as _, InitialId, PeerId, error::Error}, crypto::{tls, CryptoSuite}, datagram::{Receiver, Sender}, event::{ @@ -1656,24 +1656,6 @@ impl connection::Trait for ConnectionImpl { self.accept_state = AcceptState::Active; } - // Translates protocol errors into more useful user errors - fn translate_connection_err(&self, error: connection::Error) -> Result<(), connection::Error> { - match error { - // The connection closed without an error - connection::Error::Closed { .. } => Ok(()), - // The application closed the connection - connection::Error::Transport { code, .. } - if code == transport::Error::APPLICATION_ERROR.code => - { - Ok(()) - } - // The local connection's idle timer expired - connection::Error::IdleTimerExpired { .. } => Ok(()), - // Otherwise return the real error to the user - _ => Err(error), - } - } - fn interests(&self) -> ConnectionInterests { use crate::connection::finalization::Provider as _; use timer::Provider as _; @@ -1761,7 +1743,7 @@ impl connection::Trait for ConnectionImpl { context: &Context, ) -> Poll, connection::Error>> { if let Err(error) = self.error { - match self.translate_connection_err(error) { + match Error::into_accept_error(error) { Ok(_) => return Ok(None).into(), Err(err) => return Err(err).into(), }; diff --git a/quic/s2n-quic-transport/src/connection/connection_trait.rs b/quic/s2n-quic-transport/src/connection/connection_trait.rs index 395e7e8e6a..23a45a6a27 100644 --- a/quic/s2n-quic-transport/src/connection/connection_trait.rs +++ b/quic/s2n-quic-transport/src/connection/connection_trait.rs @@ -444,8 +444,6 @@ pub trait ConnectionTrait: 'static + Send + Sized { fn error(&self) -> Option; - fn translate_connection_err(&self, error: connection::Error) -> Result<(), connection::Error>; - fn query_event_context(&self, query: &mut dyn query::Query); fn query_event_context_mut(&mut self, query: &mut dyn query::QueryMut); diff --git a/quic/s2n-quic-transport/src/stream/manager.rs b/quic/s2n-quic-transport/src/stream/manager.rs index e07ddf856a..79e46375e7 100644 --- a/quic/s2n-quic-transport/src/stream/manager.rs +++ b/quic/s2n-quic-transport/src/stream/manager.rs @@ -24,7 +24,7 @@ use core::{ }; use futures_core::ready; use s2n_quic_core::{ - ack, endpoint, + ack, connection::error::Error, endpoint, frame::{ stream::StreamRef, DataBlocked, MaxData, MaxStreamData, MaxStreams, ResetStream, StopSending, StreamDataBlocked, StreamsBlocked, @@ -524,19 +524,11 @@ impl AbstractStreamManager { return Ok(Some(stream_id)).into(); }); - match self.inner.close_reason { - // The connection closed without an error - Some(connection::Error::Closed { .. }) => return Ok(None).into(), - // Translate application closes to end of stream - Some(connection::Error::Transport { code, .. }) - if code == transport::Error::APPLICATION_ERROR.code => - { - return Ok(None).into() - } - // Translate idle timer expiration to end of stream - Some(connection::Error::IdleTimerExpired { .. }) => return Ok(None).into(), - Some(reason) => return Err(reason).into(), - None => {} + if let Some(close_reason) = self.inner.close_reason { + match Error::into_accept_error(close_reason) { + Ok(_) => return Ok(None).into(), + Err(err) => return Err(err).into(), + }; } // Store the `Waker` for notifying the application if we accept a Stream From f90b35c107b0341bf87b9f9ce07c4db25da943c5 Mon Sep 17 00:00:00 2001 From: Madeleine Date: Tue, 2 May 2023 11:34:07 -0700 Subject: [PATCH 5/5] cargo fmt --- quic/s2n-quic-transport/src/connection/connection_impl.rs | 2 +- quic/s2n-quic-transport/src/stream/manager.rs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/quic/s2n-quic-transport/src/connection/connection_impl.rs b/quic/s2n-quic-transport/src/connection/connection_impl.rs index 59e029d6ee..50a4d1e9ba 100644 --- a/quic/s2n-quic-transport/src/connection/connection_impl.rs +++ b/quic/s2n-quic-transport/src/connection/connection_impl.rs @@ -34,7 +34,7 @@ use core::{ use s2n_quic_core::{ application, application::ServerName, - connection::{id::Generator as _, InitialId, PeerId, error::Error}, + connection::{error::Error, id::Generator as _, InitialId, PeerId}, crypto::{tls, CryptoSuite}, datagram::{Receiver, Sender}, event::{ diff --git a/quic/s2n-quic-transport/src/stream/manager.rs b/quic/s2n-quic-transport/src/stream/manager.rs index 79e46375e7..3e5b1c7c86 100644 --- a/quic/s2n-quic-transport/src/stream/manager.rs +++ b/quic/s2n-quic-transport/src/stream/manager.rs @@ -24,7 +24,9 @@ use core::{ }; use futures_core::ready; use s2n_quic_core::{ - ack, connection::error::Error, endpoint, + ack, + connection::error::Error, + endpoint, frame::{ stream::StreamRef, DataBlocked, MaxData, MaxStreamData, MaxStreams, ResetStream, StopSending, StreamDataBlocked, StreamsBlocked,