feat(swarm): make stream uprade errors more ergonomic

The currently provided `ConnectionHandlerUpgrErr` is very hard to use. Not only does it have a long name, it also features 3 levels of nesting which results in a lot of boilerplate. Last but not least, it exposes `multistream-select` as a dependency to all protocols.

We fix all of the above by renaming the type to `StreamUpgradeError` and flattening out its interface. Unrecoverable errors during protocol selection are hidden within the `Io` variant.

Related: #3759.

Pull-Request: #3882.

swarm/src/connection.rs

@@ -32,7 +32,7 @@ use crate::handler::{
     FullyNegotiatedOutbound, ListenUpgradeError,
 };
 use crate::upgrade::{InboundUpgradeSend, OutboundUpgradeSend, SendWrapper};
-use crate::{ConnectionHandlerEvent, ConnectionHandlerUpgrErr, KeepAlive, SubstreamProtocol};
+use crate::{ConnectionHandlerEvent, KeepAlive, StreamUpgradeError, SubstreamProtocol};
 use futures::stream::FuturesUnordered;
 use futures::FutureExt;
 use futures::StreamExt;
@@ -41,9 +41,11 @@ use instant::Instant;
 use libp2p_core::connection::ConnectedPoint;
 use libp2p_core::multiaddr::Multiaddr;
 use libp2p_core::muxing::{StreamMuxerBox, StreamMuxerEvent, StreamMuxerExt, SubstreamBox};
-use libp2p_core::upgrade::{InboundUpgradeApply, OutboundUpgradeApply};
+use libp2p_core::upgrade;
+use libp2p_core::upgrade::{
+    InboundUpgradeApply, NegotiationError, OutboundUpgradeApply, ProtocolError,
+};
 use libp2p_core::Endpoint;
-use libp2p_core::{upgrade, UpgradeError};
 use libp2p_identity::PeerId;
 use std::future::Future;
 use std::sync::atomic::{AtomicUsize, Ordering};
@@ -220,7 +222,7 @@ where
                     handler.on_connection_event(ConnectionEvent::DialUpgradeError(
                         DialUpgradeError {
                             info,
-                            error: ConnectionHandlerUpgrErr::Timeout,
+                            error: StreamUpgradeError::Timeout,
                         },
                     ));
                     continue;
@@ -273,23 +275,21 @@ where
                     ));
                     continue;
                 }
-                Poll::Ready(Some((
-                    info,
-                    Err(ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply(error))),
-                ))) => {
+                Poll::Ready(Some((info, Err(StreamUpgradeError::Apply(error))))) => {
                     handler.on_connection_event(ConnectionEvent::ListenUpgradeError(
                         ListenUpgradeError { info, error },
                     ));
                     continue;
                 }
-                Poll::Ready(Some((
-                    _,
-                    Err(ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(e))),
-                ))) => {
+                Poll::Ready(Some((_, Err(StreamUpgradeError::Io(e))))) => {
                     log::debug!("failed to upgrade inbound stream: {e}");
                     continue;
                 }
-                Poll::Ready(Some((_, Err(ConnectionHandlerUpgrErr::Timeout)))) => {
+                Poll::Ready(Some((_, Err(StreamUpgradeError::NegotiationFailed)))) => {
+                    log::debug!("no protocol could be agreed upon for inbound stream");
+                    continue;
+                }
+                Poll::Ready(Some((_, Err(StreamUpgradeError::Timeout)))) => {
                     log::debug!("inbound stream upgrade timed out");
                     continue;
                 }
@@ -488,11 +488,11 @@ impl<UserData, Upgrade> Unpin for SubstreamUpgrade<UserData, Upgrade> {}
 
 impl<UserData, Upgrade, UpgradeOutput, TUpgradeError> Future for SubstreamUpgrade<UserData, Upgrade>
 where
-    Upgrade: Future<Output = Result<UpgradeOutput, UpgradeError<TUpgradeError>>> + Unpin,
+    Upgrade: Future<Output = Result<UpgradeOutput, upgrade::UpgradeError<TUpgradeError>>> + Unpin,
 {
     type Output = (
         UserData,
-        Result<UpgradeOutput, ConnectionHandlerUpgrErr<TUpgradeError>>,
+        Result<UpgradeOutput, StreamUpgradeError<TUpgradeError>>,
     );
 
     fn poll(mut self: Pin<&mut Self>, cx: &mut Context) -> Poll<Self::Output> {
@@ -502,28 +502,34 @@ where
                     self.user_data
                         .take()
                         .expect("Future not to be polled again once ready."),
-                    Err(ConnectionHandlerUpgrErr::Timeout),
+                    Err(StreamUpgradeError::Timeout),
                 ))
             }
 
             Poll::Pending => {}
         }
 
-        match self.upgrade.poll_unpin(cx) {
-            Poll::Ready(Ok(upgrade)) => Poll::Ready((
-                self.user_data
-                    .take()
-                    .expect("Future not to be polled again once ready."),
-                Ok(upgrade),
-            )),
-            Poll::Ready(Err(err)) => Poll::Ready((
-                self.user_data
-                    .take()
-                    .expect("Future not to be polled again once ready."),
-                Err(ConnectionHandlerUpgrErr::Upgrade(err)),
-            )),
-            Poll::Pending => Poll::Pending,
-        }
+        let result = futures::ready!(self.upgrade.poll_unpin(cx));
+        let user_data = self
+            .user_data
+            .take()
+            .expect("Future not to be polled again once ready.");
+
+        Poll::Ready((
+            user_data,
+            result.map_err(|e| match e {
+                upgrade::UpgradeError::Select(NegotiationError::Failed) => {
+                    StreamUpgradeError::NegotiationFailed
+                }
+                upgrade::UpgradeError::Select(NegotiationError::ProtocolError(
+                    ProtocolError::IoError(e),
+                )) => StreamUpgradeError::Io(e),
+                upgrade::UpgradeError::Select(NegotiationError::ProtocolError(other)) => {
+                    StreamUpgradeError::Io(io::Error::new(io::ErrorKind::Other, other))
+                }
+                upgrade::UpgradeError::Apply(e) => StreamUpgradeError::Apply(e),
+            }),
+        ))
     }
 }
 
@@ -683,7 +689,7 @@ mod tests {
 
         assert!(matches!(
             connection.handler.error.unwrap(),
-            ConnectionHandlerUpgrErr::Timeout
+            StreamUpgradeError::Timeout
         ))
     }
 
@@ -786,7 +792,7 @@ mod tests {
 
     struct MockConnectionHandler {
         outbound_requested: bool,
-        error: Option<ConnectionHandlerUpgrErr<Void>>,
+        error: Option<StreamUpgradeError<Void>>,
         upgrade_timeout: Duration,
     }
 

swarm/src/dummy.rs

@@ -4,12 +4,12 @@ use crate::handler::{
     ConnectionEvent, DialUpgradeError, FullyNegotiatedInbound, FullyNegotiatedOutbound,
 };
 use crate::{
-    ConnectionDenied, ConnectionHandlerEvent, ConnectionHandlerUpgrErr, KeepAlive,
-    SubstreamProtocol, THandler, THandlerInEvent, THandlerOutEvent,
+    ConnectionDenied, ConnectionHandlerEvent, KeepAlive, StreamUpgradeError, SubstreamProtocol,
+    THandler, THandlerInEvent, THandlerOutEvent,
 };
 use libp2p_core::upgrade::DeniedUpgrade;
 use libp2p_core::Endpoint;
-use libp2p_core::{Multiaddr, UpgradeError};
+use libp2p_core::Multiaddr;
 use libp2p_identity::PeerId;
 use std::task::{Context, Poll};
 use void::Void;
@@ -132,9 +132,9 @@ impl crate::handler::ConnectionHandler for ConnectionHandler {
                 protocol, ..
             }) => void::unreachable(protocol),
             ConnectionEvent::DialUpgradeError(DialUpgradeError { info: _, error }) => match error {
-                ConnectionHandlerUpgrErr::Timeout => unreachable!(),
-                ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply(e)) => void::unreachable(e),
-                ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(_)) => {
+                StreamUpgradeError::Timeout => unreachable!(),
+                StreamUpgradeError::Apply(e) => void::unreachable(e),
+                StreamUpgradeError::NegotiationFailed | StreamUpgradeError::Io(_) => {
                     unreachable!("Denied upgrade does not support any protocols")
                 }
             },

swarm/src/handler.rs

@@ -49,8 +49,8 @@ mod select;
 pub use crate::upgrade::{InboundUpgradeSend, OutboundUpgradeSend, SendWrapper, UpgradeInfoSend};
 
 use instant::Instant;
-use libp2p_core::{upgrade::UpgradeError, Multiaddr};
-use std::{cmp::Ordering, error, fmt, task::Context, task::Poll, time::Duration};
+use libp2p_core::Multiaddr;
+use std::{cmp::Ordering, error, fmt, io, task::Context, task::Poll, time::Duration};
 
 pub use map_in::MapInEvent;
 pub use map_out::MapOutEvent;
@@ -270,7 +270,7 @@ pub struct AddressChange<'a> {
 /// that upgrading an outbound substream to the given protocol has failed.
 pub struct DialUpgradeError<OOI, OP: OutboundUpgradeSend> {
     pub info: OOI,
-    pub error: ConnectionHandlerUpgrErr<OP::Error>,
+    pub error: StreamUpgradeError<OP::Error>,
 }
 
 /// [`ConnectionEvent`] variant that informs the handler
@@ -458,54 +458,67 @@ impl<TConnectionUpgrade, TOutboundOpenInfo, TCustom, TErr>
     }
 }
 
+#[deprecated(note = "Renamed to `StreamUpgradeError`")]
+pub type ConnectionHandlerUpgrErr<TUpgrErr> = StreamUpgradeError<TUpgrErr>;
+
 /// Error that can happen on an outbound substream opening attempt.
 #[derive(Debug)]
-pub enum ConnectionHandlerUpgrErr<TUpgrErr> {
+pub enum StreamUpgradeError<TUpgrErr> {
     /// The opening attempt timed out before the negotiation was fully completed.
     Timeout,
-    /// Error while upgrading the substream to the protocol we want.
-    Upgrade(UpgradeError<TUpgrErr>),
+    /// The upgrade produced an error.
+    Apply(TUpgrErr),
+    /// No protocol could be agreed upon.
+    NegotiationFailed,
+    /// An IO or otherwise unrecoverable error happened.
+    Io(io::Error),
 }
 
-impl<TUpgrErr> ConnectionHandlerUpgrErr<TUpgrErr> {
-    /// Map the inner [`UpgradeError`] type.
-    pub fn map_upgrade_err<F, E>(self, f: F) -> ConnectionHandlerUpgrErr<E>
+impl<TUpgrErr> StreamUpgradeError<TUpgrErr> {
+    /// Map the inner [`StreamUpgradeError`] type.
+    pub fn map_upgrade_err<F, E>(self, f: F) -> StreamUpgradeError<E>
     where
-        F: FnOnce(UpgradeError<TUpgrErr>) -> UpgradeError<E>,
+        F: FnOnce(TUpgrErr) -> E,
     {
         match self {
-            ConnectionHandlerUpgrErr::Timeout => ConnectionHandlerUpgrErr::Timeout,
-            ConnectionHandlerUpgrErr::Upgrade(e) => ConnectionHandlerUpgrErr::Upgrade(f(e)),
+            StreamUpgradeError::Timeout => StreamUpgradeError::Timeout,
+            StreamUpgradeError::Apply(e) => StreamUpgradeError::Apply(f(e)),
+            StreamUpgradeError::NegotiationFailed => StreamUpgradeError::NegotiationFailed,
+            StreamUpgradeError::Io(e) => StreamUpgradeError::Io(e),
         }
     }
 }
 
-impl<TUpgrErr> fmt::Display for ConnectionHandlerUpgrErr<TUpgrErr>
+impl<TUpgrErr> fmt::Display for StreamUpgradeError<TUpgrErr>
 where
     TUpgrErr: error::Error + 'static,
 {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
-            ConnectionHandlerUpgrErr::Timeout => {
+            StreamUpgradeError::Timeout => {
                 write!(f, "Timeout error while opening a substream")
             }
-            ConnectionHandlerUpgrErr::Upgrade(err) => {
-                write!(f, "Upgrade: ")?;
+            StreamUpgradeError::Apply(err) => {
+                write!(f, "Apply: ")?;
                 crate::print_error_chain(f, err)
             }
+            StreamUpgradeError::NegotiationFailed => {
+                write!(f, "no protocols could be agreed upon")
+            }
+            StreamUpgradeError::Io(e) => {
+                write!(f, "IO error: ")?;
+                crate::print_error_chain(f, e)
+            }
         }
     }
 }
 
-impl<TUpgrErr> error::Error for ConnectionHandlerUpgrErr<TUpgrErr>
+impl<TUpgrErr> error::Error for StreamUpgradeError<TUpgrErr>
 where
     TUpgrErr: error::Error + 'static,
 {
     fn source(&self) -> Option<&(dyn error::Error + 'static)> {
-        match self {
-            ConnectionHandlerUpgrErr::Timeout => None,
-            ConnectionHandlerUpgrErr::Upgrade(_) => None,
-        }
+        None
     }
 }
 

swarm/src/handler/one_shot.rs

@@ -19,8 +19,8 @@
 // DEALINGS IN THE SOFTWARE.
 
 use crate::handler::{
-    ConnectionEvent, ConnectionHandler, ConnectionHandlerEvent, ConnectionHandlerUpgrErr,
-    DialUpgradeError, FullyNegotiatedInbound, FullyNegotiatedOutbound, KeepAlive,
+    ConnectionEvent, ConnectionHandler, ConnectionHandlerEvent, DialUpgradeError,
+    FullyNegotiatedInbound, FullyNegotiatedOutbound, KeepAlive, StreamUpgradeError,
     SubstreamProtocol,
 };
 use crate::upgrade::{InboundUpgradeSend, OutboundUpgradeSend};
@@ -37,7 +37,7 @@ where
     /// The upgrade for inbound substreams.
     listen_protocol: SubstreamProtocol<TInbound, ()>,
     /// If `Some`, something bad happened and we should shut down the handler with an error.
-    pending_error: Option<ConnectionHandlerUpgrErr<<TOutbound as OutboundUpgradeSend>::Error>>,
+    pending_error: Option<StreamUpgradeError<<TOutbound as OutboundUpgradeSend>::Error>>,
     /// Queue of events to produce in `poll()`.
     events_out: SmallVec<[TEvent; 4]>,
     /// Queue of outbound substreams to open.
@@ -123,7 +123,7 @@ where
 {
     type InEvent = TOutbound;
     type OutEvent = TEvent;
-    type Error = ConnectionHandlerUpgrErr<<Self::OutboundProtocol as OutboundUpgradeSend>::Error>;
+    type Error = StreamUpgradeError<<Self::OutboundProtocol as OutboundUpgradeSend>::Error>;
     type InboundProtocol = TInbound;
     type OutboundProtocol = TOutbound;
     type OutboundOpenInfo = ();

swarm/src/handler/select.rs

@@ -19,14 +19,14 @@
 // DEALINGS IN THE SOFTWARE.
 
 use crate::handler::{
-    AddressChange, ConnectionEvent, ConnectionHandler, ConnectionHandlerEvent,
-    ConnectionHandlerUpgrErr, DialUpgradeError, FullyNegotiatedInbound, FullyNegotiatedOutbound,
-    InboundUpgradeSend, KeepAlive, ListenUpgradeError, OutboundUpgradeSend, SubstreamProtocol,
+    AddressChange, ConnectionEvent, ConnectionHandler, ConnectionHandlerEvent, DialUpgradeError,
+    FullyNegotiatedInbound, FullyNegotiatedOutbound, InboundUpgradeSend, KeepAlive,
+    ListenUpgradeError, OutboundUpgradeSend, StreamUpgradeError, SubstreamProtocol,
 };
 use crate::upgrade::SendWrapper;
 use either::Either;
 use futures::future;
-use libp2p_core::upgrade::{SelectUpgrade, UpgradeError};
+use libp2p_core::upgrade::SelectUpgrade;
 use std::{cmp, task::Context, task::Poll};
 
 /// Implementation of [`ConnectionHandler`] that combines two protocols into one.
@@ -110,47 +110,32 @@ where
         match self {
             DialUpgradeError {
                 info: Either::Left(info),
-                error: ConnectionHandlerUpgrErr::Timeout,
+                error: StreamUpgradeError::Apply(Either::Left(err)),
             } => Either::Left(DialUpgradeError {
                 info,
-                error: ConnectionHandlerUpgrErr::Timeout,
+                error: StreamUpgradeError::Apply(err),
+            }),
+            DialUpgradeError {
+                info: Either::Right(info),
+                error: StreamUpgradeError::Apply(Either::Right(err)),
+            } => Either::Right(DialUpgradeError {
+                info,
+                error: StreamUpgradeError::Apply(err),
             }),
             DialUpgradeError {
                 info: Either::Left(info),
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(err)),
+                error: e,
             } => Either::Left(DialUpgradeError {
                 info,
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(err)),
-            }),
-            DialUpgradeError {
-                info: Either::Left(info),
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply(Either::Left(err))),
-            } => Either::Left(DialUpgradeError {
-                info,
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply(err)),
+                error: e.map_upgrade_err(|_| panic!("already handled above")),
             }),
             DialUpgradeError {
                 info: Either::Right(info),
-                error: ConnectionHandlerUpgrErr::Timeout,
+                error: e,
             } => Either::Right(DialUpgradeError {
                 info,
-                error: ConnectionHandlerUpgrErr::Timeout,
+                error: e.map_upgrade_err(|_| panic!("already handled above")),
             }),
-            DialUpgradeError {
-                info: Either::Right(info),
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(err)),
-            } => Either::Right(DialUpgradeError {
-                info,
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(err)),
-            }),
-            DialUpgradeError {
-                info: Either::Right(info),
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply(Either::Right(err))),
-            } => Either::Right(DialUpgradeError {
-                info,
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply(err)),
-            }),
-            _ => panic!("Wrong API usage; the upgrade error doesn't match the outbound open info"),
         }
     }
 }

swarm/src/lib.rs

@@ -122,8 +122,8 @@ pub use connection::pool::{ConnectionCounters, ConnectionLimits};
 pub use connection::{ConnectionError, ConnectionId};
 pub use executor::Executor;
 pub use handler::{
-    ConnectionHandler, ConnectionHandlerEvent, ConnectionHandlerSelect, ConnectionHandlerUpgrErr,
-    KeepAlive, OneShotHandler, OneShotHandlerConfig, SubstreamProtocol,
+    ConnectionHandler, ConnectionHandlerEvent, ConnectionHandlerSelect, KeepAlive, OneShotHandler,
+    OneShotHandlerConfig, StreamUpgradeError, SubstreamProtocol,
 };
 #[cfg(feature = "macros")]
 pub use libp2p_swarm_derive::NetworkBehaviour;