From cca7c6e5f08e9393318b8b0ae69aaeaab77fde32 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Sun, 22 Jul 2018 18:17:15 +0200 Subject: [PATCH 01/33] Rewrite ConnectionReuse --- core/src/connection_reuse.rs | 712 +++++++++++++++++++++++++---------- 1 file changed, 505 insertions(+), 207 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 67bf9469342..9b0d37f8906 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -1,4 +1,4 @@ -// Copyright 2017 Parity Technologies (UK) Ltd. +// Copyright 2018 Parity Technologies (UK) Ltd. // // Permission is hereby granted, free of charge, to any person obtaining a // copy of this software and associated documentation files (the "Software"), @@ -40,14 +40,16 @@ //! `MuxedTransport` trait. use fnv::FnvHashMap; -use futures::future::{self, Either, FutureResult}; -use futures::{Async, Future, Poll, Stream}; +use futures::future::{self, FutureResult}; +use futures::{Async, Future, Poll, Stream, stream, task}; use futures::stream::FuturesUnordered; -use futures::sync::mpsc; use multiaddr::Multiaddr; use muxing::StreamMuxer; use parking_lot::Mutex; -use std::io::{self, Error as IoError}; +use std::collections::hash_map::Entry; +use std::io::{Error as IoError, ErrorKind as IoErrorKind, Read, Write}; +use std::mem; +use std::ops::{Deref, DerefMut}; use std::sync::Arc; use tokio_io::{AsyncRead, AsyncWrite}; use transport::{MuxedTransport, Transport, UpgradedNode}; @@ -62,54 +64,80 @@ use upgrade::ConnectionUpgrade; pub struct ConnectionReuse where T: Transport, - T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade, C::Output: StreamMuxer, { - // Underlying transport and connection upgrade for when we need to dial or listen. - inner: UpgradedNode, - - // Struct shared between most of the `ConnectionReuse` infrastructure. - shared: Arc>>, + /// Struct shared between most of the `ConnectionReuse` infrastructure. + shared: Arc>>, } -struct Shared +/// Struct shared between most of the `ConnectionReuse` infrastructure. +struct Shared where - M: StreamMuxer, + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, { - // List of active muxers. - active_connections: FnvHashMap, + /// Underlying transport and connection upgrade, used when we need to dial or listen. + transport: UpgradedNode, + + /// All the connections that were opened, whether successful and/or active or not. + // TODO: this will grow forever + connections: FnvHashMap>, - // List of pending inbound substreams from dialed nodes. - // Only add to this list elements received through `add_to_next_rx`. - next_incoming: Vec<(M, M::InboundSubstream, Multiaddr)>, + /// Tasks to notify when one or more new elements were added to `connections`. + notify_on_new_connec: Vec, - // New elements are not directly added to `next_incoming`. Instead they are sent to this - // channel. This is done so that we can wake up tasks whenever a new element is added. - add_to_next_rx: mpsc::UnboundedReceiver<(M, M::InboundSubstream, Multiaddr)>, + /// Next `connection_id` to use when opening a connection. + next_connection_id: u64, +} - // Other side of `add_to_next_rx`. - add_to_next_tx: mpsc::UnboundedSender<(M, M::InboundSubstream, Multiaddr)>, +enum PeerState where M: StreamMuxer { + /// Connection is active and can be used to open substreams. + Active { + /// The muxer to open new substreams. + muxer: M, + /// Next incoming substream. + next_incoming: M::InboundSubstream, + /// Future of the address of the client. + client_addr: Multiaddr, + /// Unique identifier for this connection in the `ConnectionReuse`. + connection_id: u64, + /// Number of open substreams. + num_substreams: u64, + }, + + /// Connection is pending. + // TODO: stronger Future type + Pending { + /// Future that produces the muxer. + future: Box>, + /// All the tasks to notify when `future` resolves. + notify: Vec, + }, + + /// An earlier connection attempt errored. + Errored(IoError), + + /// The `PeerState` is poisonned. Happens if a panic happened while executing some of the + /// functions. + Poisonned, } impl From> for ConnectionReuse where T: Transport, - T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade, C::Output: StreamMuxer, { #[inline] fn from(node: UpgradedNode) -> ConnectionReuse { - let (tx, rx) = mpsc::unbounded(); - ConnectionReuse { - inner: node, shared: Arc::new(Mutex::new(Shared { - active_connections: Default::default(), - next_incoming: Vec::new(), - add_to_next_rx: rx, - add_to_next_tx: tx, + transport: node, + connections: Default::default(), + notify_on_new_connec: Vec::new(), + next_connection_id: 0, })), } } @@ -122,21 +150,22 @@ where C: ConnectionUpgrade + Clone + 'static, // TODO: 'static :( C::Output: StreamMuxer + Clone, C::MultiaddrFuture: Future, - C::NamesIter: Clone, // TODO: not elegant + C::NamesIter: Clone, + UpgradedNode: Clone, { - type Output = ::Substream; + type Output = ConnectionReuseSubstream; type MultiaddrFuture = future::FutureResult; type Listener = Box>; type ListenerUpgrade = FutureResult<(Self::Output, Self::MultiaddrFuture), IoError>; - type Dial = Box>; + type Dial = ConnectionReuseDial; fn listen_on(self, addr: Multiaddr) -> Result<(Self::Listener, Multiaddr), (Self, Multiaddr)> { - let (listener, new_addr) = match self.inner.listen_on(addr.clone()) { + let transport = self.shared.lock().transport.clone(); + let (listener, new_addr) = match transport.listen_on(addr.clone()) { Ok((l, a)) => (l, a), - Err((inner, addr)) => { + Err((_, addr)) => { return Err(( ConnectionReuse { - inner: inner, shared: self.shared, }, addr, @@ -145,88 +174,50 @@ where }; let listener = listener - .fuse() .map(|upgr| { upgr.and_then(|(out, addr)| { + trace!("Waiting for remote's address as listener"); addr.map(move |addr| (out, addr)) }) - }); + }) + .fuse(); let listener = ConnectionReuseListener { shared: self.shared.clone(), listener: listener, current_upgrades: FuturesUnordered::new(), - connections: Vec::new(), }; Ok((Box::new(listener) as Box<_>, new_addr)) } + #[inline] fn dial(self, addr: Multiaddr) -> Result { - // If we already have an active connection, use it! - let substream = if let Some(muxer) = self.shared - .lock() - .active_connections - .get(&addr) - .map(|muxer| muxer.clone()) - { - let a = addr.clone(); - Either::A(muxer.outbound().map(move |s| s.map(move |s| (s, future::ok(a))))) - } else { - Either::B(future::ok(None)) + let mut shared = self.shared.lock(); + + // If an earlier attempt to dial this multiaddress failed, we clear the error. Otherwise + // the returned `Future` will immediately produce the error. + let must_clear = match shared.connections.get(&addr) { + Some(&PeerState::Errored(ref err)) => { + trace!("Clearing existing connection to {} which errored earlier: {:?}", addr, err); + true + }, + _ => false, }; + if must_clear { + shared.connections.remove(&addr); + } - let shared = self.shared.clone(); - let inner = self.inner; - let future = substream.and_then(move |outbound| { - if let Some(o) = outbound { - debug!("Using existing multiplexed connection to {}", addr); - return Either::A(future::ok(o)); - } - // The previous stream muxer did not yield a new substream => start new dial - debug!("No existing connection to {}; dialing", addr); - match inner.dial(addr.clone()) { - Ok(dial) => { - let future = dial - .and_then(move |(muxer, addr_fut)| { - trace!("Waiting for remote's address"); - addr_fut.map(move |addr| (muxer, addr)) - }) - .and_then(move |(muxer, addr)| { - muxer.clone().outbound().and_then(move |substream| { - if let Some(s) = substream { - // Replace the active connection because we are the most recent. - let mut lock = shared.lock(); - lock.active_connections.insert(addr.clone(), muxer.clone()); - // TODO: doesn't need locking ; the sender could be extracted - let _ = lock.add_to_next_tx.unbounded_send(( - muxer.clone(), - muxer.inbound(), - addr.clone(), - )); - Ok((s, future::ok(addr))) - } else { - error!("failed to dial to {}", addr); - shared.lock().active_connections.remove(&addr); - Err(io::Error::new(io::ErrorKind::Other, "dial failed")) - } - }) - }); - Either::B(Either::A(future)) - } - Err(_) => { - let e = io::Error::new(io::ErrorKind::Other, "transport rejected dial"); - Either::B(Either::B(future::err(e))) - } - } - }); - - Ok(Box::new(future) as Box<_>) + Ok(ConnectionReuseDial { + outbound: None, + shared: self.shared.clone(), + addr, + }) } #[inline] fn nat_traversal(&self, server: &Multiaddr, observed: &Multiaddr) -> Option { - self.inner.transport().nat_traversal(server, observed) + self.shared.lock().transport.transport().nat_traversal(server, observed) } } @@ -237,11 +228,12 @@ where C: ConnectionUpgrade + Clone + 'static, // TODO: 'static :( C::Output: StreamMuxer + Clone, C::MultiaddrFuture: Future, - C::NamesIter: Clone, // TODO: not elegant + C::NamesIter: Clone, + UpgradedNode: Clone, { - type Incoming = ConnectionReuseIncoming; + type Incoming = ConnectionReuseIncoming; type IncomingUpgrade = - future::FutureResult<(::Substream, Self::MultiaddrFuture), IoError>; + future::FutureResult<(ConnectionReuseSubstream, Self::MultiaddrFuture), IoError>; #[inline] fn next_incoming(self) -> Self::Incoming { @@ -251,27 +243,206 @@ where } } +/// Implementation of `Future` for dialing a node. +pub struct ConnectionReuseDial +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, +{ + /// The future that will construct the substream, the connection id the muxer comes from, and + /// the `Future` of the client's multiaddr. + /// If `None`, we need to grab a new outbound substream from the muxer. + outbound: Option>, + + // Shared between the whole connection reuse mechanism. + shared: Arc>>, + + // The address we're trying to dial. + addr: Multiaddr, +} + +struct ConnectionReuseDialOut +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, +{ + /// The pending outbound substream. + stream: ::OutboundSubstream, + /// Id of the connection that was used to create the substream. + connection_id: u64, + /// Address of the remote. + client_addr: Multiaddr, +} + +impl Future for ConnectionReuseDial +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer + Clone + 'static, + UpgradedNode: Transport + Clone, + as Transport>::Dial: 'static, + as Transport>::MultiaddrFuture: 'static, +{ + type Item = (ConnectionReuseSubstream, FutureResult); + type Error = IoError; + + fn poll(&mut self) -> Poll { + loop { + let should_kill_existing_muxer; + if let Some(mut outbound) = self.outbound.take() { + match outbound.stream.poll() { + Ok(Async::Ready(Some(inner))) => { + trace!("Opened new outgoing substream to {}", self.addr); + let substream = ConnectionReuseSubstream { + connection_id: outbound.connection_id, + shared: self.shared.clone(), + inner, + addr: outbound.client_addr.clone(), + }; + return Ok(Async::Ready((substream, future::ok(outbound.client_addr)))); + }, + Ok(Async::NotReady) => { + self.outbound = Some(outbound); + return Ok(Async::NotReady); + }, + Ok(Async::Ready(None)) => { + // The muxer can no longer produce outgoing substreams. + // Let's reopen a connection. + trace!("Closing existing connection to {} ; can't produce outgoing substreams", self.addr); + should_kill_existing_muxer = true; + }, + Err(err) => { + // If we get an error while opening a substream, we decide to ignore it + // and open a new muxer. + // If opening the muxer produces an error, *then* we will return it. + debug!("Error while opening outgoing substream to {}: {:?}", self.addr, err); + should_kill_existing_muxer = true; + }, + } + } else { + should_kill_existing_muxer = false; + } + + // If we reach this point, that means we have to fill `self.outbound`. + // If `should_kill_existing_muxer`, do not use any existing connection but create a + // new one instead. + let mut shared = self.shared.lock(); + let shared = &mut *shared; // Avoids borrow errors + + // TODO: could be optimized + if should_kill_existing_muxer { + shared.connections.remove(&self.addr); + } + let connec = match shared.connections.entry(self.addr.clone()) { + Entry::Occupied(e) => e.into_mut(), + Entry::Vacant(e) => { + // Build the connection. + let state = match shared.transport.clone().dial(self.addr.clone()) { + Ok(future) => { + trace!("Opened new connection to {:?}", self.addr); + let future = future.and_then(|(out, addr)| addr.map(move |a| (out, a))); + let future = Box::new(future); + PeerState::Pending { future, notify: Vec::new() } + }, + Err(_) => { + trace!("Failed to open connection to {:?}, multiaddr not supported", self.addr); + let err = IoError::new(IoErrorKind::ConnectionRefused, "multiaddr not supported"); + PeerState::Errored(err) + }, + }; + + for task in shared.notify_on_new_connec.drain(..) { + task.notify(); + } + + e.insert(state) + }, + }; + + match mem::replace(&mut *connec, PeerState::Poisonned) { + PeerState::Active { muxer, next_incoming, connection_id, num_substreams, client_addr } => { + let first_outbound = muxer.clone().outbound(); + *connec = PeerState::Active { muxer, next_incoming, connection_id, num_substreams, client_addr: client_addr.clone() }; + trace!("Using existing connection to {} to open outbound substream", self.addr); + self.outbound = Some(ConnectionReuseDialOut { + stream: first_outbound, + connection_id, + client_addr, + }); + }, + PeerState::Pending { mut future, mut notify } => { + match future.poll() { + Ok(Async::Ready((muxer, client_addr))) => { + trace!("Successful new connection to {} ({})", self.addr, client_addr); + for task in notify { + task.notify(); + } + let next_incoming = muxer.clone().inbound(); + let first_outbound = muxer.clone().outbound(); + let connection_id = shared.next_connection_id; + shared.next_connection_id += 1; + *connec = PeerState::Active { muxer, next_incoming, connection_id, num_substreams: 1, client_addr: client_addr.clone() }; + self.outbound = Some(ConnectionReuseDialOut { + stream: first_outbound, + connection_id, + client_addr, + }); + }, + Ok(Async::NotReady) => { + notify.push(task::current()); + *connec = PeerState::Pending { future, notify }; + return Ok(Async::NotReady); + }, + Err(err) => { + trace!("Failed new connection to {}: {:?}", self.addr, err); + let io_err = IoError::new(err.kind(), err.to_string()); + *connec = PeerState::Errored(err); + return Err(io_err); + }, + } + }, + PeerState::Errored(err) => { + trace!("Existing new connection to {} errored earlier: {:?}", self.addr, err); + let io_err = IoError::new(err.kind(), err.to_string()); + *connec = PeerState::Errored(err); + return Err(io_err); + }, + PeerState::Poisonned => { + panic!("Poisonned peer state"); + }, + } + } + } +} + /// Implementation of `Stream` for the connections incoming from listening on a specific address. -pub struct ConnectionReuseListener +pub struct ConnectionReuseListener where - M: StreamMuxer, + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, { - // The main listener. `S` is from the underlying transport. - listener: S, - current_upgrades: FuturesUnordered, - connections: Vec<(M, ::InboundSubstream, Multiaddr)>, + /// The main listener. + listener: stream::Fuse, + /// Opened connections that need to be upgraded. + current_upgrades: FuturesUnordered>>, - // Shared between the whole connection reuse mechanism. - shared: Arc>>, + /// Shared between the whole connection reuse mechanism. + shared: Arc>>, } -impl Stream for ConnectionReuseListener +impl Stream for ConnectionReuseListener where - S: Stream, - F: Future, - M: StreamMuxer + Clone + 'static, // TODO: 'static :( + T: Transport, + T::Output: AsyncRead + AsyncWrite, + C: ConnectionUpgrade, + C::Output: StreamMuxer + Clone, + L: Stream, + Lu: Future + 'static, { - type Item = FutureResult<(M::Substream, FutureResult), IoError>; + type Item = FutureResult<(ConnectionReuseSubstream, FutureResult), IoError>; type Error = IoError; fn poll(&mut self) -> Poll, Self::Error> { @@ -281,145 +452,272 @@ where loop { match self.listener.poll() { Ok(Async::Ready(Some(upgrade))) => { - self.current_upgrades.push(upgrade); + trace!("New incoming connection"); + self.current_upgrades.push(Box::new(upgrade)); } Ok(Async::NotReady) => break, Ok(Async::Ready(None)) => { - debug!("listener has been closed"); - if self.connections.is_empty() && self.current_upgrades.is_empty() { + debug!("Listener has been closed"); + if self.current_upgrades.is_empty() { return Ok(Async::Ready(None)); + } else { + break; } - break } Err(err) => { - debug!("error while polling listener: {:?}", err); - if self.connections.is_empty() && self.current_upgrades.is_empty() { + debug!("Error while polling listener: {:?}", err); + if self.current_upgrades.is_empty() { return Err(err); + } else { + break; } - break } - } + }; } + // Process the connections being upgraded. loop { match self.current_upgrades.poll() { Ok(Async::Ready(Some((muxer, client_addr)))) => { + // Successfully upgraded a new incoming connection. + trace!("New multiplexed connection from {}", client_addr); + let mut shared = self.shared.lock(); let next_incoming = muxer.clone().inbound(); - self.connections - .push((muxer.clone(), next_incoming, client_addr.clone())); - } - Err(err) => { - debug!("error while upgrading listener connection: {:?}", err); - return Ok(Async::Ready(Some(future::err(err)))); + let connection_id = shared.next_connection_id; + shared.next_connection_id += 1; + let state = PeerState::Active { muxer, next_incoming, connection_id, num_substreams: 1, client_addr: client_addr.clone() }; + shared.connections.insert(client_addr, state); + for to_notify in shared.notify_on_new_connec.drain(..) { + to_notify.notify(); + } } - _ => break, - } - } - - // Check whether any incoming substream is ready. - for n in (0..self.connections.len()).rev() { - let (muxer, mut next_incoming, client_addr) = self.connections.swap_remove(n); - match next_incoming.poll() { Ok(Async::Ready(None)) => { - // stream muxer gave us a `None` => connection should be considered closed - debug!("no more inbound substreams on {}", client_addr); - self.shared.lock().active_connections.remove(&client_addr); - } - Ok(Async::Ready(Some(incoming))) => { - // We overwrite any current active connection to that multiaddr because we - // are the freshest possible connection. - self.shared - .lock() - .active_connections - .insert(client_addr.clone(), muxer.clone()); - // A new substream is ready. - let mut new_next = muxer.clone().inbound(); - self.connections - .push((muxer, new_next, client_addr.clone())); - return Ok(Async::Ready(Some( - future::ok((incoming, future::ok(client_addr))), - ))); + // No upgrade remaining ; if the listener is closed, close everything. + if self.listener.is_done() { + return Ok(Async::Ready(None)); + } else { + break; + } } Ok(Async::NotReady) => { - self.connections.push((muxer, next_incoming, client_addr)); - } + break; + }, Err(err) => { - debug!("error while upgrading the multiplexed incoming connection: {:?}", err); - // Insert the rest of the pending connections, but not the current one. + // Insert the rest of the pending upgrades, but not the current one. + debug!("Error while upgrading listener connection: {:?}", err); return Ok(Async::Ready(Some(future::err(err)))); } } } - // Nothing is ready, return `NotReady`. + // TODO: the listener should also poll the incoming substreams on the active connections + // that it opened, so that the muxed transport doesn't have to do it + Ok(Async::NotReady) } } /// Implementation of `Future` that yields the next incoming substream from a dialed connection. -pub struct ConnectionReuseIncoming +pub struct ConnectionReuseIncoming where - M: StreamMuxer, + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, { // Shared between the whole connection reuse system. - shared: Arc>>, + shared: Arc>>, } -impl Future for ConnectionReuseIncoming +impl Future for ConnectionReuseIncoming where - M: Clone + StreamMuxer, + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer + Clone, { - type Item = future::FutureResult<(M::Substream, future::FutureResult), IoError>; + type Item = future::FutureResult<(ConnectionReuseSubstream, future::FutureResult), IoError>; type Error = IoError; fn poll(&mut self) -> Poll { - let mut lock = self.shared.lock(); - - // Try to get any new muxer from `add_to_next_rx`. - // We push the new muxers to a channel instead of adding them to `next_incoming`, so that - // tasks are notified when something is pushed. - loop { - match lock.add_to_next_rx.poll() { - Ok(Async::Ready(Some(elem))) => { - lock.next_incoming.push(elem); - } - Ok(Async::NotReady) => break, - Ok(Async::Ready(None)) | Err(_) => unreachable!( - "the sender and receiver are both in the same struct, therefore \ - the link can never break" - ), + let mut shared = self.shared.lock(); + + // Keys of the elements in `shared.connections` to remove afterwards. + let mut to_remove = Vec::new(); + // Substream to return, if any found. + let mut ret_value = None; + + for (addr, state) in shared.connections.iter_mut() { + match *state { + PeerState::Active { ref mut next_incoming, ref muxer, ref mut num_substreams, connection_id, ref client_addr } => { + match next_incoming.poll() { + Ok(Async::Ready(Some(inner))) => { + trace!("New incoming substream from {}", client_addr); + let next = muxer.clone().inbound(); + *next_incoming = next; + *num_substreams += 1; + let substream = ConnectionReuseSubstream { + inner, + shared: self.shared.clone(), + connection_id, + addr: client_addr.clone(), + }; + ret_value = Some(Ok((substream, future::ok(client_addr.clone())))); + break; + }, + Ok(Async::Ready(None)) => { + // The muxer isn't capable of opening any inbound stream anymore, so + // we close the connection entirely. + trace!("Removing existing connection to {} as it cannot open inbound anymore", addr); + to_remove.push(addr.clone()); + }, + Ok(Async::NotReady) => (), + Err(err) => { + // If an error happens while opening an inbound stream, we close the + // connection entirely. + trace!("Error while opening inbound substream to {}: {:?}", addr, err); + to_remove.push(addr.clone()); + ret_value = Some(Err(err)); + break; + }, + } + }, + PeerState::Pending { ref mut notify, .. } => { + // TODO: this will add a new element at each iteration + notify.push(task::current()); + }, + PeerState::Errored(_) => {}, + PeerState::Poisonned => { + panic!("Poisonned peer state"); + }, } } - // Check whether any incoming substream is ready. - for n in (0..lock.next_incoming.len()).rev() { - let (muxer, mut future, addr) = lock.next_incoming.swap_remove(n); - match future.poll() { - Ok(Async::Ready(None)) => { - debug!("no inbound substream for {}", addr); - lock.active_connections.remove(&addr); - } - Ok(Async::Ready(Some(value))) => { - // A substream is ready ; push back the muxer for the next time this function - // is called, then return. - debug!("New incoming substream"); - let next = muxer.clone().inbound(); - lock.next_incoming.push((muxer, next, addr.clone())); - return Ok(Async::Ready(future::ok((value, future::ok(addr))))); - } - Ok(Async::NotReady) => { - lock.next_incoming.push((muxer, future, addr)); - } - Err(err) => { - // In case of error, we just not push back the element, which drops it. - debug!("ConnectionReuse incoming: one of the \ - multiplexed substreams produced an error: {:?}", - err); + for to_remove in to_remove { + shared.connections.remove(&to_remove); + } + + match ret_value { + Some(Ok(val)) => Ok(Async::Ready(future::ok(val))), + Some(Err(err)) => Err(err), + None => { + // TODO: will add an element to the list every time + shared.notify_on_new_connec.push(task::current()); + Ok(Async::NotReady) + }, + } + } +} + +/// Wraps around the `Substream`. +pub struct ConnectionReuseSubstream +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, +{ + inner: ::Substream, + shared: Arc>>, + /// Id this connection was created from. + connection_id: u64, + /// Address of the remote. + addr: Multiaddr, +} + +impl Deref for ConnectionReuseSubstream +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, +{ + type Target = ::Substream; + + #[inline] + fn deref(&self) -> &Self::Target { + &self.inner + } +} + +impl DerefMut for ConnectionReuseSubstream +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, +{ + #[inline] + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.inner + } +} + +impl Read for ConnectionReuseSubstream +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, +{ + #[inline] + fn read(&mut self, buf: &mut [u8]) -> Result { + self.inner.read(buf) + } +} + +impl AsyncRead for ConnectionReuseSubstream +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, +{ +} + +impl Write for ConnectionReuseSubstream +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, +{ + #[inline] + fn write(&mut self, buf: &[u8]) -> Result { + self.inner.write(buf) + } + + #[inline] + fn flush(&mut self) -> Result<(), IoError> { + self.inner.flush() + } +} + +impl AsyncWrite for ConnectionReuseSubstream +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, +{ + #[inline] + fn shutdown(&mut self) -> Poll<(), IoError> { + self.inner.shutdown() + } +} + +impl Drop for ConnectionReuseSubstream +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, +{ + fn drop(&mut self) { + // Remove one substream from all the connections whose connection_id matches ours. + let mut shared = self.shared.lock(); + shared.connections.retain(|_, connec| { + if let PeerState::Active { connection_id, ref mut num_substreams, .. } = connec { + if *connection_id == self.connection_id { + *num_substreams -= 1; + if *num_substreams == 0 { + trace!("All substreams to {} closed ; closing main connection", self.addr); + return false; + } } } - } - // Nothing is ready. - Ok(Async::NotReady) + true + }); } } From 623230da59a78044c11255097de03cfb33f9c9c8 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 23 Jul 2018 12:05:26 +0200 Subject: [PATCH 02/33] Listen to incoming substreams on the listener --- core/src/connection_reuse.rs | 228 +++++++++++++++++++++-------------- 1 file changed, 137 insertions(+), 91 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 9b0d37f8906..4a6834ac623 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -90,6 +90,9 @@ where /// Next `connection_id` to use when opening a connection. next_connection_id: u64, + + /// Next `listener_id` for the next listener we create. + next_listener_id: u64, } enum PeerState where M: StreamMuxer { @@ -105,6 +108,9 @@ enum PeerState where M: StreamMuxer { connection_id: u64, /// Number of open substreams. num_substreams: u64, + /// Id of the listener that created this connection, or `None` if it was opened by a + /// dialer. + listener_id: Option, }, /// Connection is pending. @@ -138,6 +144,7 @@ where connections: Default::default(), notify_on_new_connec: Vec::new(), next_connection_id: 0, + next_listener_id: 0, })), } } @@ -160,13 +167,14 @@ where type Dial = ConnectionReuseDial; fn listen_on(self, addr: Multiaddr) -> Result<(Self::Listener, Multiaddr), (Self, Multiaddr)> { - let transport = self.shared.lock().transport.clone(); - let (listener, new_addr) = match transport.listen_on(addr.clone()) { + let mut shared = self.shared.lock(); + + let (listener, new_addr) = match shared.transport.clone().listen_on(addr.clone()) { Ok((l, a)) => (l, a), Err((_, addr)) => { return Err(( ConnectionReuse { - shared: self.shared, + shared: self.shared.clone(), }, addr, )); @@ -182,9 +190,13 @@ where }) .fuse(); + let listener_id = shared.next_listener_id; + shared.next_listener_id += 1; + let listener = ConnectionReuseListener { shared: self.shared.clone(), - listener: listener, + listener, + listener_id, current_upgrades: FuturesUnordered::new(), }; @@ -362,9 +374,9 @@ where }; match mem::replace(&mut *connec, PeerState::Poisonned) { - PeerState::Active { muxer, next_incoming, connection_id, num_substreams, client_addr } => { + PeerState::Active { muxer, next_incoming, connection_id, listener_id, num_substreams, client_addr } => { let first_outbound = muxer.clone().outbound(); - *connec = PeerState::Active { muxer, next_incoming, connection_id, num_substreams, client_addr: client_addr.clone() }; + *connec = PeerState::Active { muxer, next_incoming, connection_id, listener_id, num_substreams, client_addr: client_addr.clone() }; trace!("Using existing connection to {} to open outbound substream", self.addr); self.outbound = Some(ConnectionReuseDialOut { stream: first_outbound, @@ -383,7 +395,7 @@ where let first_outbound = muxer.clone().outbound(); let connection_id = shared.next_connection_id; shared.next_connection_id += 1; - *connec = PeerState::Active { muxer, next_incoming, connection_id, num_substreams: 1, client_addr: client_addr.clone() }; + *connec = PeerState::Active { muxer, next_incoming, connection_id, num_substreams: 1, listener_id: None, client_addr: client_addr.clone() }; self.outbound = Some(ConnectionReuseDialOut { stream: first_outbound, connection_id, @@ -426,6 +438,8 @@ where { /// The main listener. listener: stream::Fuse, + /// Identifier for this listener. Used to determine which connections were opened by it. + listener_id: u64, /// Opened connections that need to be upgraded. current_upgrades: FuturesUnordered>>, @@ -458,19 +472,11 @@ where Ok(Async::NotReady) => break, Ok(Async::Ready(None)) => { debug!("Listener has been closed"); - if self.current_upgrades.is_empty() { - return Ok(Async::Ready(None)); - } else { - break; - } + break; } Err(err) => { debug!("Error while polling listener: {:?}", err); - if self.current_upgrades.is_empty() { - return Err(err); - } else { - break; - } + return Err(err); } }; } @@ -485,21 +491,13 @@ where let next_incoming = muxer.clone().inbound(); let connection_id = shared.next_connection_id; shared.next_connection_id += 1; - let state = PeerState::Active { muxer, next_incoming, connection_id, num_substreams: 1, client_addr: client_addr.clone() }; + let state = PeerState::Active { muxer, next_incoming, connection_id, listener_id: Some(self.listener_id), num_substreams: 1, client_addr: client_addr.clone() }; shared.connections.insert(client_addr, state); for to_notify in shared.notify_on_new_connec.drain(..) { to_notify.notify(); } } - Ok(Async::Ready(None)) => { - // No upgrade remaining ; if the listener is closed, close everything. - if self.listener.is_done() { - return Ok(Async::Ready(None)); - } else { - break; - } - } - Ok(Async::NotReady) => { + Ok(Async::Ready(None)) | Ok(Async::NotReady) => { break; }, Err(err) => { @@ -510,10 +508,26 @@ where } } - // TODO: the listener should also poll the incoming substreams on the active connections - // that it opened, so that the muxed transport doesn't have to do it - - Ok(Async::NotReady) + // Poll all the incoming connections on all the connections we opened. + let mut shared = self.shared.lock(); + match poll_incoming(&self.shared, &mut shared, Some(self.listener_id)) { + Ok(Async::Ready(None)) => { + if self.listener.is_done() && self.current_upgrades.is_empty() { + Ok(Async::Ready(None)) + } else { + Ok(Async::NotReady) + } + }, + Ok(Async::Ready(Some(substream))) => { + Ok(Async::Ready(Some(substream))) + }, + Ok(Async::NotReady) => { + Ok(Async::NotReady) + } + Err(err) => { + Ok(Async::Ready(Some(future::err(err)))) + } + } } } @@ -537,76 +551,108 @@ where type Item = future::FutureResult<(ConnectionReuseSubstream, future::FutureResult), IoError>; type Error = IoError; + #[inline] fn poll(&mut self) -> Poll { let mut shared = self.shared.lock(); - - // Keys of the elements in `shared.connections` to remove afterwards. - let mut to_remove = Vec::new(); - // Substream to return, if any found. - let mut ret_value = None; - - for (addr, state) in shared.connections.iter_mut() { - match *state { - PeerState::Active { ref mut next_incoming, ref muxer, ref mut num_substreams, connection_id, ref client_addr } => { - match next_incoming.poll() { - Ok(Async::Ready(Some(inner))) => { - trace!("New incoming substream from {}", client_addr); - let next = muxer.clone().inbound(); - *next_incoming = next; - *num_substreams += 1; - let substream = ConnectionReuseSubstream { - inner, - shared: self.shared.clone(), - connection_id, - addr: client_addr.clone(), - }; - ret_value = Some(Ok((substream, future::ok(client_addr.clone())))); - break; - }, - Ok(Async::Ready(None)) => { - // The muxer isn't capable of opening any inbound stream anymore, so - // we close the connection entirely. - trace!("Removing existing connection to {} as it cannot open inbound anymore", addr); - to_remove.push(addr.clone()); - }, - Ok(Async::NotReady) => (), - Err(err) => { - // If an error happens while opening an inbound stream, we close the - // connection entirely. - trace!("Error while opening inbound substream to {}: {:?}", addr, err); - to_remove.push(addr.clone()); - ret_value = Some(Err(err)); - break; - }, - } - }, - PeerState::Pending { ref mut notify, .. } => { - // TODO: this will add a new element at each iteration - notify.push(task::current()); - }, - PeerState::Errored(_) => {}, - PeerState::Poisonned => { - panic!("Poisonned peer state"); - }, - } - } - - for to_remove in to_remove { - shared.connections.remove(&to_remove); - } - - match ret_value { - Some(Ok(val)) => Ok(Async::Ready(future::ok(val))), - Some(Err(err)) => Err(err), - None => { + match poll_incoming(&self.shared, &mut shared, None) { + Ok(Async::Ready(Some(substream))) => { + Ok(Async::Ready(substream)) + }, + Ok(Async::Ready(None)) | Ok(Async::NotReady) => { // TODO: will add an element to the list every time shared.notify_on_new_connec.push(task::current()); Ok(Async::NotReady) }, + Err(err) => Err(err) } } } +/// Polls the incoming substreams on all the incoming connections that match the `listener`. +/// +/// Returns `Ready(None)` if no connection is matching the `listener`. Returns `NotReady` if +/// one or more connections are matching the `listener` but they are not ready. +fn poll_incoming(shared_arc: &Arc>>, shared: &mut Shared, listener: Option) + -> Poll, FutureResult), IoError>>, IoError> +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer + Clone, +{ + // Keys of the elements in `shared.connections` to remove afterwards. + let mut to_remove = Vec::new(); + // Substream to return, if any found. + let mut ret_value = None; + let mut found_one = false; + + for (addr, state) in shared.connections.iter_mut() { + match *state { + PeerState::Active { ref mut next_incoming, ref muxer, ref mut num_substreams, connection_id, ref client_addr, listener_id } => { + if listener_id != listener { + continue; + } + found_one = true; + + match next_incoming.poll() { + Ok(Async::Ready(Some(inner))) => { + trace!("New incoming substream from {}", client_addr); + let next = muxer.clone().inbound(); + *next_incoming = next; + *num_substreams += 1; + let substream = ConnectionReuseSubstream { + inner, + shared: shared_arc.clone(), + connection_id, + addr: client_addr.clone(), + }; + ret_value = Some(Ok((substream, future::ok(client_addr.clone())))); + break; + }, + Ok(Async::Ready(None)) => { + // The muxer isn't capable of opening any inbound stream anymore, so + // we close the connection entirely. + trace!("Removing existing connection to {} as it cannot open inbound anymore", addr); + to_remove.push(addr.clone()); + }, + Ok(Async::NotReady) => (), + Err(err) => { + // If an error happens while opening an inbound stream, we close the + // connection entirely. + trace!("Error while opening inbound substream to {}: {:?}", addr, err); + to_remove.push(addr.clone()); + ret_value = Some(Err(err)); + break; + }, + } + }, + PeerState::Pending { ref mut notify, .. } => { + // TODO: this will add a new element at each iteration + notify.push(task::current()); + }, + PeerState::Errored(_) => {}, + PeerState::Poisonned => { + panic!("Poisonned peer state"); + }, + } + } + + for to_remove in to_remove { + shared.connections.remove(&to_remove); + } + + match ret_value { + Some(Ok(val)) => Ok(Async::Ready(Some(future::ok(val)))), + Some(Err(err)) => Err(err), + None => { + if found_one { + Ok(Async::NotReady) + } else { + Ok(Async::Ready(None)) + } + }, + } +} + /// Wraps around the `Substream`. pub struct ConnectionReuseSubstream where From 9977dfc2a5268bd46b55590af94cf5b3131f48d7 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 23 Jul 2018 15:47:57 +0200 Subject: [PATCH 03/33] Fix dialer not counting as active substream --- core/src/connection_reuse.rs | 58 +++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 17 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 4a6834ac623..f1328ad4487 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -374,12 +374,13 @@ where }; match mem::replace(&mut *connec, PeerState::Poisonned) { - PeerState::Active { muxer, next_incoming, connection_id, listener_id, num_substreams, client_addr } => { - let first_outbound = muxer.clone().outbound(); + PeerState::Active { muxer, next_incoming, connection_id, listener_id, mut num_substreams, client_addr } => { + let outbound = muxer.clone().outbound(); + num_substreams += 1; *connec = PeerState::Active { muxer, next_incoming, connection_id, listener_id, num_substreams, client_addr: client_addr.clone() }; trace!("Using existing connection to {} to open outbound substream", self.addr); self.outbound = Some(ConnectionReuseDialOut { - stream: first_outbound, + stream: outbound, connection_id, client_addr, }); @@ -429,6 +430,20 @@ where } } +impl Drop for ConnectionReuseDial +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, +{ + fn drop(&mut self) { + if let Some(outbound) = self.outbound.take() { + let mut shared = self.shared.lock(); + remove_one_substream(&mut *shared, outbound.connection_id, &outbound.client_addr); + } + } +} + /// Implementation of `Stream` for the connections incoming from listening on a specific address. pub struct ConnectionReuseListener where @@ -653,6 +668,28 @@ where } } +/// Removes one substream from an active connection. Closes the connection if necessary. +fn remove_one_substream(shared: &mut Shared, connec_id: u64, addr: &Multiaddr) +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer, +{ + shared.connections.retain(|_, connec| { + if let PeerState::Active { connection_id, ref mut num_substreams, .. } = connec { + if *connection_id == connec_id { + *num_substreams -= 1; + if *num_substreams == 0 { + trace!("All substreams to {} closed ; closing main connection", addr); + return false; + } + } + } + + true + }); +} + /// Wraps around the `Substream`. pub struct ConnectionReuseSubstream where @@ -750,20 +787,7 @@ where C::Output: StreamMuxer, { fn drop(&mut self) { - // Remove one substream from all the connections whose connection_id matches ours. let mut shared = self.shared.lock(); - shared.connections.retain(|_, connec| { - if let PeerState::Active { connection_id, ref mut num_substreams, .. } = connec { - if *connection_id == self.connection_id { - *num_substreams -= 1; - if *num_substreams == 0 { - trace!("All substreams to {} closed ; closing main connection", self.addr); - return false; - } - } - } - - true - }); + remove_one_substream(&mut *shared, self.connection_id, &self.addr); } } From 9dbf7dfdcf1660b0cadd0a95a79ddbf024f74073 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Thu, 26 Jul 2018 13:17:12 +0200 Subject: [PATCH 04/33] Fix leak --- core/src/connection_reuse.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index f1328ad4487..f5a5c7282e1 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -50,7 +50,7 @@ use std::collections::hash_map::Entry; use std::io::{Error as IoError, ErrorKind as IoErrorKind, Read, Write}; use std::mem; use std::ops::{Deref, DerefMut}; -use std::sync::Arc; +use std::sync::{Arc, atomic::AtomicUsize, atomic::Ordering}; use tokio_io::{AsyncRead, AsyncWrite}; use transport::{MuxedTransport, Transport, UpgradedNode}; use upgrade::ConnectionUpgrade; @@ -86,7 +86,7 @@ where connections: FnvHashMap>, /// Tasks to notify when one or more new elements were added to `connections`. - notify_on_new_connec: Vec, + notify_on_new_connec: FnvHashMap, /// Next `connection_id` to use when opening a connection. next_connection_id: u64, @@ -142,7 +142,7 @@ where shared: Arc::new(Mutex::new(Shared { transport: node, connections: Default::default(), - notify_on_new_connec: Vec::new(), + notify_on_new_connec: Default::default(), next_connection_id: 0, next_listener_id: 0, })), @@ -365,8 +365,8 @@ where }, }; - for task in shared.notify_on_new_connec.drain(..) { - task.notify(); + for task in shared.notify_on_new_connec.drain() { + task.1.notify(); } e.insert(state) @@ -508,8 +508,8 @@ where shared.next_connection_id += 1; let state = PeerState::Active { muxer, next_incoming, connection_id, listener_id: Some(self.listener_id), num_substreams: 1, client_addr: client_addr.clone() }; shared.connections.insert(client_addr, state); - for to_notify in shared.notify_on_new_connec.drain(..) { - to_notify.notify(); + for to_notify in shared.notify_on_new_connec.drain() { + to_notify.1.notify(); } } Ok(Async::Ready(None)) | Ok(Async::NotReady) => { @@ -575,7 +575,11 @@ where }, Ok(Async::Ready(None)) | Ok(Async::NotReady) => { // TODO: will add an element to the list every time - shared.notify_on_new_connec.push(task::current()); + static NEXT_TASK_ID: AtomicUsize = AtomicUsize::new(0); + task_local!{ + static TASK_ID: usize = NEXT_TASK_ID.fetch_add(1, Ordering::Relaxed) + } + shared.notify_on_new_connec.insert(TASK_ID.with(|&v| v), task::current()); Ok(Async::NotReady) }, Err(err) => Err(err) From a1216df7b4572499d687616a5503e72ff67b13e9 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Sat, 28 Jul 2018 14:33:27 +0200 Subject: [PATCH 05/33] Fix potentially using tons of memory --- core/src/connection_reuse.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index f5a5c7282e1..88b950676b9 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -119,7 +119,7 @@ enum PeerState where M: StreamMuxer { /// Future that produces the muxer. future: Box>, /// All the tasks to notify when `future` resolves. - notify: Vec, + notify: FnvHashMap, }, /// An earlier connection attempt errored. @@ -255,6 +255,12 @@ where } } +static NEXT_TASK_ID: AtomicUsize = AtomicUsize::new(0); +// `TASK_ID` is used internally to uniquely identify each task. +task_local!{ + static TASK_ID: usize = NEXT_TASK_ID.fetch_add(1, Ordering::Relaxed) +} + /// Implementation of `Future` for dialing a node. pub struct ConnectionReuseDial where @@ -356,7 +362,7 @@ where trace!("Opened new connection to {:?}", self.addr); let future = future.and_then(|(out, addr)| addr.map(move |a| (out, a))); let future = Box::new(future); - PeerState::Pending { future, notify: Vec::new() } + PeerState::Pending { future, notify: Default::default() } }, Err(_) => { trace!("Failed to open connection to {:?}, multiaddr not supported", self.addr); @@ -390,7 +396,7 @@ where Ok(Async::Ready((muxer, client_addr))) => { trace!("Successful new connection to {} ({})", self.addr, client_addr); for task in notify { - task.notify(); + task.1.notify(); } let next_incoming = muxer.clone().inbound(); let first_outbound = muxer.clone().outbound(); @@ -404,7 +410,7 @@ where }); }, Ok(Async::NotReady) => { - notify.push(task::current()); + notify.insert(TASK_ID.with(|&t| t), task::current()); *connec = PeerState::Pending { future, notify }; return Ok(Async::NotReady); }, @@ -575,10 +581,6 @@ where }, Ok(Async::Ready(None)) | Ok(Async::NotReady) => { // TODO: will add an element to the list every time - static NEXT_TASK_ID: AtomicUsize = AtomicUsize::new(0); - task_local!{ - static TASK_ID: usize = NEXT_TASK_ID.fetch_add(1, Ordering::Relaxed) - } shared.notify_on_new_connec.insert(TASK_ID.with(|&v| v), task::current()); Ok(Async::NotReady) }, @@ -645,8 +647,7 @@ where } }, PeerState::Pending { ref mut notify, .. } => { - // TODO: this will add a new element at each iteration - notify.push(task::current()); + notify.insert(TASK_ID.with(|&t| t), task::current()); }, PeerState::Errored(_) => {}, PeerState::Poisonned => { From 90325effb85ff05ed0cb19aa6ad9224367eb86a0 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Wed, 1 Aug 2018 15:35:04 +0200 Subject: [PATCH 06/33] Refactor to use Shared in a centralised manner --- core/src/connection_reuse.rs | 707 ++++++++++++++++++---------------- core/src/transport/upgrade.rs | 4 +- 2 files changed, 379 insertions(+), 332 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 88b950676b9..dcf911ae1cf 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -46,7 +46,6 @@ use futures::stream::FuturesUnordered; use multiaddr::Multiaddr; use muxing::StreamMuxer; use parking_lot::Mutex; -use std::collections::hash_map::Entry; use std::io::{Error as IoError, ErrorKind as IoErrorKind, Read, Write}; use std::mem; use std::ops::{Deref, DerefMut}; @@ -55,62 +54,37 @@ use tokio_io::{AsyncRead, AsyncWrite}; use transport::{MuxedTransport, Transport, UpgradedNode}; use upgrade::ConnectionUpgrade; +use std::clone::Clone; + /// Allows reusing the same muxed connection multiple times. /// /// Can be created from an `UpgradedNode` through the `From` trait. /// /// Implements the `Transport` trait. -#[derive(Clone)] pub struct ConnectionReuse where T: Transport, - C: ConnectionUpgrade, - C::Output: StreamMuxer, + C: ConnectionUpgrade + Clone, + C::Output: StreamMuxer + Clone, { /// Struct shared between most of the `ConnectionReuse` infrastructure. - shared: Arc>>, + shared: Arc>, } -/// Struct shared between most of the `ConnectionReuse` infrastructure. -struct Shared -where - T: Transport, - C: ConnectionUpgrade, - C::Output: StreamMuxer, -{ - /// Underlying transport and connection upgrade, used when we need to dial or listen. - transport: UpgradedNode, - - /// All the connections that were opened, whether successful and/or active or not. - // TODO: this will grow forever - connections: FnvHashMap>, - - /// Tasks to notify when one or more new elements were added to `connections`. - notify_on_new_connec: FnvHashMap, - - /// Next `connection_id` to use when opening a connection. - next_connection_id: u64, - - /// Next `listener_id` for the next listener we create. - next_listener_id: u64, -} - -enum PeerState where M: StreamMuxer { +enum PeerState where M: StreamMuxer + Clone { /// Connection is active and can be used to open substreams. Active { /// The muxer to open new substreams. muxer: M, - /// Next incoming substream. - next_incoming: M::InboundSubstream, /// Future of the address of the client. client_addr: Multiaddr, /// Unique identifier for this connection in the `ConnectionReuse`. - connection_id: u64, + connection_id: usize, /// Number of open substreams. - num_substreams: u64, + num_substreams: usize, /// Id of the listener that created this connection, or `None` if it was opened by a /// dialer. - listener_id: Option, + listener_id: Option, }, /// Connection is pending. @@ -120,32 +94,189 @@ enum PeerState where M: StreamMuxer { future: Box>, /// All the tasks to notify when `future` resolves. notify: FnvHashMap, + /// Unique identifier for this connection in the `ConnectionReuse`. + connection_id: usize, }, /// An earlier connection attempt errored. - Errored(IoError), + Errored(IoError) +} + + +/// Struct shared between most of the `ConnectionReuse` infrastructure. +// #[derive(Clone)] +struct Shared +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer + Clone, +{ + /// Underlying transport and connection upgrade, used when we need to dial or listen. + transport: UpgradedNode, - /// The `PeerState` is poisonned. Happens if a panic happened while executing some of the - /// functions. - Poisonned, + /// All the connections that were opened, whether successful and/or active or not. + // TODO: this will grow forever + connections: Mutex>>, + + /// Tasks to notify when one or more new elements were added to `connections`. + notify_on_new_connec: Mutex>, + + /// Next `connection_id` to use when opening a connection. + next_connection_id: AtomicUsize, + + /// Next `listener_id` for the next listener we create. + next_listener_id: AtomicUsize, } -impl From> for ConnectionReuse +impl Shared where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, +{ + /// notify all `Tasks` in `self.notify_on_new_connec` that a new connection was established + /// consume the tasks in that process + fn new_con_notify(&self) { + let mut notifiers = self.notify_on_new_connec.lock(); + for to_notify in notifiers.drain() { + to_notify.1.notify(); + }; + } + + /// generate a new connection id + #[inline] + pub fn gen_next_connection_id(&self) -> usize { + self.next_connection_id.fetch_add(1, Ordering::Relaxed) + } + + /// generate a new listener id + #[inline] + pub fn gen_next_listener_id(&self) -> usize { + self.next_listener_id.fetch_add(1, Ordering::Relaxed) + } + + /// Insert a new connection, returns Some if an entry was present, notify listeners + pub fn insert_connection(&self, addr: Multiaddr, state: PeerState) + -> Option> { + let r = self.replace_connection(addr, state); + self.new_con_notify(); + r + } + + /// Replace a new connection, returns Some if an entry was present, **do not** notify listeners + pub fn replace_connection(&self, addr: Multiaddr, state: PeerState) + -> Option> { + self.connections.lock().insert(addr, state) + } + + /// Removes one substream from an active connection. Closes the connection if necessary. + pub fn remove_substream(&self, connec_id: usize, addr: &Multiaddr) { + self.connections.lock().retain(|_, connec| { + if let PeerState::Active { connection_id, ref mut num_substreams, .. } = connec { + if *connection_id == connec_id { + *num_substreams -= 1; + if *num_substreams == 0 { + trace!("All substreams to {} closed ; closing main connection", addr); + return false; + } + } + } + true + }); + } + + /// Polls the incoming substreams on all the incoming connections that match the `listener`. + /// + /// Returns `Ready(None)` if no connection is matching the `listener`. Returns `NotReady` if + /// one or more connections are matching the `listener` but they are not ready. + fn poll_incoming(&self, listener: Option) + -> Poll::Substream, usize, Multiaddr)>, IoError> + { + // Keys of the elements in `connections` to remove afterwards. + let mut to_remove = Vec::new(); + // Substream to return, if any found. + let mut ret_value = None; + let mut found_one = false; + + for (addr, state) in self.connections.lock().iter_mut() { + let res = if let PeerState::Active { ref mut muxer, ref mut num_substreams, + connection_id, client_addr, listener_id } = state { + if *listener_id == listener { + continue; + } + found_one = true; + + match muxer.clone().inbound().poll() { + Ok(Async::Ready(Some(inner))) => { + trace!("New incoming substream from {}", client_addr); + *num_substreams += 1; + Ok((connection_id.clone(), inner, client_addr.clone())) + }, + Err(err) => { + // If an error happens while opening an inbound stream, we close the + // connection entirely. + trace!("Error while opening inbound substream to {}: {:?}", addr, err); + to_remove.push(addr.clone()); + Err(err) + }, + Ok(Async::Ready(None)) => { + // The muxer isn't capable of opening any inbound stream anymore, so + // we close the connection entirely. + trace!("Removing existing connection to {} as it cannot open inbound anymore", addr); + to_remove.push(addr.clone()); + continue + }, + Ok(Async::NotReady) => { continue } + } + } else { + if listener.is_none() { + if let PeerState::Pending { ref mut notify, .. } = state { + notify.insert(TASK_ID.with(|&t| t), task::current()); + } + } + continue + }; + + ret_value = Some(res); + break; + } + + for to_remove in to_remove { + self.connections.lock().remove(&to_remove); + } + + match ret_value { + Some(Ok((connection_id, inner, addr))) => + Ok(Async::Ready(Some((inner, connection_id, addr)))), + Some(Err(err)) => Err(err), + None => { + if found_one { + Ok(Async::NotReady) + } else { + Ok(Async::Ready(None)) + } + } + } + } +} + + +impl From> for ConnectionReuse +where + T: Transport, + C: ConnectionUpgrade + Clone, + C::Output: StreamMuxer + Clone, { #[inline] fn from(node: UpgradedNode) -> ConnectionReuse { ConnectionReuse { - shared: Arc::new(Mutex::new(Shared { + shared: Arc::new(Shared { transport: node, connections: Default::default(), notify_on_new_connec: Default::default(), - next_connection_id: 0, - next_listener_id: 0, - })), + next_connection_id: Default::default(), + next_listener_id: Default::default(), + }), } } } @@ -156,6 +287,7 @@ where T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade + Clone + 'static, // TODO: 'static :( C::Output: StreamMuxer + Clone, + ::Substream : Clone, C::MultiaddrFuture: Future, C::NamesIter: Clone, UpgradedNode: Clone, @@ -167,14 +299,13 @@ where type Dial = ConnectionReuseDial; fn listen_on(self, addr: Multiaddr) -> Result<(Self::Listener, Multiaddr), (Self, Multiaddr)> { - let mut shared = self.shared.lock(); - let (listener, new_addr) = match shared.transport.clone().listen_on(addr.clone()) { + let (listener, new_addr) = match self.shared.transport.clone().listen_on(addr.clone()) { Ok((l, a)) => (l, a), Err((_, addr)) => { return Err(( ConnectionReuse { - shared: self.shared.clone(), + shared: self.shared, }, addr, )); @@ -190,11 +321,10 @@ where }) .fuse(); - let listener_id = shared.next_listener_id; - shared.next_listener_id += 1; + let listener_id = self.shared.gen_next_listener_id(); let listener = ConnectionReuseListener { - shared: self.shared.clone(), + shared: self.shared, listener, listener_id, current_upgrades: FuturesUnordered::new(), @@ -205,11 +335,10 @@ where #[inline] fn dial(self, addr: Multiaddr) -> Result { - let mut shared = self.shared.lock(); // If an earlier attempt to dial this multiaddress failed, we clear the error. Otherwise // the returned `Future` will immediately produce the error. - let must_clear = match shared.connections.get(&addr) { + let must_clear = match self.shared.connections.lock().get(&addr) { Some(&PeerState::Errored(ref err)) => { trace!("Clearing existing connection to {} which errored earlier: {:?}", addr, err); true @@ -217,19 +346,19 @@ where _ => false, }; if must_clear { - shared.connections.remove(&addr); + self.shared.connections.lock().remove(&addr); } Ok(ConnectionReuseDial { outbound: None, - shared: self.shared.clone(), + shared: self.shared, addr, }) } #[inline] fn nat_traversal(&self, server: &Multiaddr, observed: &Multiaddr) -> Option { - self.shared.lock().transport.transport().nat_traversal(server, observed) + self.shared.transport.transport().nat_traversal(server, observed) } } @@ -239,6 +368,7 @@ where T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade + Clone + 'static, // TODO: 'static :( C::Output: StreamMuxer + Clone, + ::Substream : Clone, C::MultiaddrFuture: Future, C::NamesIter: Clone, UpgradedNode: Clone, @@ -250,7 +380,7 @@ where #[inline] fn next_incoming(self) -> Self::Incoming { ConnectionReuseIncoming { - shared: self.shared.clone(), + shared: self.shared, } } } @@ -266,7 +396,7 @@ pub struct ConnectionReuseDial where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { /// The future that will construct the substream, the connection id the muxer comes from, and /// the `Future` of the client's multiaddr. @@ -274,7 +404,7 @@ where outbound: Option>, // Shared between the whole connection reuse mechanism. - shared: Arc>>, + shared: Arc>, // The address we're trying to dial. addr: Multiaddr, @@ -284,12 +414,12 @@ struct ConnectionReuseDialOut where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { /// The pending outbound substream. stream: ::OutboundSubstream, /// Id of the connection that was used to create the substream. - connection_id: u64, + connection_id: usize, /// Address of the remote. client_addr: Multiaddr, } @@ -299,6 +429,7 @@ where T: Transport, C: ConnectionUpgrade, C::Output: StreamMuxer + Clone + 'static, + ::Substream : Clone, UpgradedNode: Transport + Clone, as Transport>::Dial: 'static, as Transport>::MultiaddrFuture: 'static, @@ -308,129 +439,139 @@ where fn poll(&mut self) -> Poll { loop { - let should_kill_existing_muxer; - if let Some(mut outbound) = self.outbound.take() { - match outbound.stream.poll() { - Ok(Async::Ready(Some(inner))) => { - trace!("Opened new outgoing substream to {}", self.addr); - let substream = ConnectionReuseSubstream { - connection_id: outbound.connection_id, - shared: self.shared.clone(), - inner, - addr: outbound.client_addr.clone(), - }; - return Ok(Async::Ready((substream, future::ok(outbound.client_addr)))); - }, - Ok(Async::NotReady) => { - self.outbound = Some(outbound); - return Ok(Async::NotReady); - }, - Ok(Async::Ready(None)) => { - // The muxer can no longer produce outgoing substreams. - // Let's reopen a connection. - trace!("Closing existing connection to {} ; can't produce outgoing substreams", self.addr); - should_kill_existing_muxer = true; - }, - Err(err) => { - // If we get an error while opening a substream, we decide to ignore it - // and open a new muxer. - // If opening the muxer produces an error, *then* we will return it. - debug!("Error while opening outgoing substream to {}: {:?}", self.addr, err); - should_kill_existing_muxer = true; - }, - } - } else { - should_kill_existing_muxer = false; - } - - // If we reach this point, that means we have to fill `self.outbound`. - // If `should_kill_existing_muxer`, do not use any existing connection but create a - // new one instead. - let mut shared = self.shared.lock(); - let shared = &mut *shared; // Avoids borrow errors - - // TODO: could be optimized - if should_kill_existing_muxer { - shared.connections.remove(&self.addr); - } - let connec = match shared.connections.entry(self.addr.clone()) { - Entry::Occupied(e) => e.into_mut(), - Entry::Vacant(e) => { - // Build the connection. - let state = match shared.transport.clone().dial(self.addr.clone()) { - Ok(future) => { - trace!("Opened new connection to {:?}", self.addr); - let future = future.and_then(|(out, addr)| addr.map(move |a| (out, a))); - let future = Box::new(future); - PeerState::Pending { future, notify: Default::default() } + let should_kill_existing_muxer = match self.outbound.take() { + Some(mut outbound) => { + match outbound.stream.poll() { + Ok(Async::Ready(Some(inner))) => { + trace!("Opened new outgoing substream to {}", self.addr); + let shared = self.shared.clone(); + return Ok(Async::Ready((ConnectionReuseSubstream { + connection_id: outbound.connection_id, + inner, + shared, + addr: outbound.client_addr.clone(), + }, future::ok(outbound.client_addr)))); }, - Err(_) => { - trace!("Failed to open connection to {:?}, multiaddr not supported", self.addr); - let err = IoError::new(IoErrorKind::ConnectionRefused, "multiaddr not supported"); - PeerState::Errored(err) + Ok(Async::NotReady) => { + self.outbound = Some(outbound); + return Ok(Async::NotReady); + }, + Ok(Async::Ready(None)) => { + // The muxer can no longer produce outgoing substreams. + // Let's reopen a connection. + trace!("Closing existing connection to {} ; can't produce outgoing substreams", self.addr); + true + }, + Err(err) => { + // If we get an error while opening a substream, we decide to ignore it + // and open a new muxer. + // If opening the muxer produces an error, *then* we will return it. + debug!("Error while opening outgoing substream to {}: {:?}", self.addr, err); + true }, - }; - - for task in shared.notify_on_new_connec.drain() { - task.1.notify(); } - - e.insert(state) }, + _ => false }; - match mem::replace(&mut *connec, PeerState::Poisonned) { - PeerState::Active { muxer, next_incoming, connection_id, listener_id, mut num_substreams, client_addr } => { - let outbound = muxer.clone().outbound(); - num_substreams += 1; - *connec = PeerState::Active { muxer, next_incoming, connection_id, listener_id, num_substreams, client_addr: client_addr.clone() }; - trace!("Using existing connection to {} to open outbound substream", self.addr); - self.outbound = Some(ConnectionReuseDialOut { - stream: outbound, - connection_id, - client_addr, - }); - }, - PeerState::Pending { mut future, mut notify } => { - match future.poll() { - Ok(Async::Ready((muxer, client_addr))) => { - trace!("Successful new connection to {} ({})", self.addr, client_addr); - for task in notify { - task.1.notify(); - } - let next_incoming = muxer.clone().inbound(); - let first_outbound = muxer.clone().outbound(); - let connection_id = shared.next_connection_id; - shared.next_connection_id += 1; - *connec = PeerState::Active { muxer, next_incoming, connection_id, num_substreams: 1, listener_id: None, client_addr: client_addr.clone() }; + + let is_empty = { + let mut conns = self.shared.connections.lock(); + if should_kill_existing_muxer { + conns.remove(&self.addr); + true + + } else { + let item = conns.get(&self.addr); + + // error'ed, quit early + if let Some(PeerState::Errored(err)) = item { + trace!("Existing new connection to {} errored earlier: {:?}", self.addr, err); + let io_err = IoError::new(err.kind(), err.to_string()); + return Err(io_err) + }; + + item.is_none() + } + }; + + if is_empty { + // dial new and return early + let state = match self.shared.transport.clone().dial(self.addr.clone()) { + Ok(future) => { + trace!("Opened new connection to {:?}", self.addr); + let future = future.and_then(|(out, addr)| addr.map(move |a| (out, a))); + let future = Box::new(future); + let connection_id = self.shared.gen_next_connection_id(); + PeerState::Pending { future, notify: Default::default(), connection_id } + }, + Err(_) => { + trace!("Failed to open connection to {:?}, multiaddr not supported", self.addr); + let err = IoError::new(IoErrorKind::ConnectionRefused, "multiaddr not supported"); + PeerState::Errored(err) + }, + }; + + self.shared.insert_connection(self.addr.clone(), state); + return Ok(Async::NotReady); + } + + let mut conn = self.shared.connections.lock(); + if let Some(state) = conn.get_mut(&self.addr) { + + let replace_with = { + match state { + PeerState::Active { muxer, connection_id, client_addr, ref mut num_substreams, .. } => { + trace!("Using existing connection to {} to open outbound substream", self.addr); self.outbound = Some(ConnectionReuseDialOut { - stream: first_outbound, - connection_id, - client_addr, + stream: muxer.clone().outbound(), + connection_id: connection_id.clone(), + client_addr: client_addr.clone(), }); + + *num_substreams += 1; + None }, - Ok(Async::NotReady) => { - notify.insert(TASK_ID.with(|&t| t), task::current()); - *connec = PeerState::Pending { future, notify }; - return Ok(Async::NotReady); - }, - Err(err) => { - trace!("Failed new connection to {}: {:?}", self.addr, err); - let io_err = IoError::new(err.kind(), err.to_string()); - *connec = PeerState::Errored(err); - return Err(io_err); + PeerState::Pending { ref mut future, ref mut notify, connection_id } => { + match future.poll() { + Ok(Async::Ready((muxer, client_addr))) => { + trace!("Successful new connection to {} ({})", self.addr, client_addr); + for task in notify { + task.1.notify(); + } + let first_outbound = muxer.clone().outbound(); + self.outbound = Some(ConnectionReuseDialOut { + stream: first_outbound, + connection_id: connection_id.clone(), + client_addr: client_addr.clone(), + }); + + Some(PeerState::Active { muxer, connection_id: connection_id.clone(), + num_substreams: 1, listener_id: None, + client_addr }) + }, + Ok(Async::NotReady) => { + notify.insert(TASK_ID.with(|&t| t), task::current()); + return Ok(Async::NotReady); + }, + Err(err) => { + trace!("Failed new connection to {}: {:?}", self.addr, err); + Some(PeerState::Errored(err)) + }, + } }, + _ => { + // Because of a race someone might has changed the Peerstate between + // this get_mut and the previous lock. But no harm for us, we can just + // loop on and will find it soon enough. + continue + } } - }, - PeerState::Errored(err) => { - trace!("Existing new connection to {} errored earlier: {:?}", self.addr, err); - let io_err = IoError::new(err.kind(), err.to_string()); - *connec = PeerState::Errored(err); - return Err(io_err); - }, - PeerState::Poisonned => { - panic!("Poisonned peer state"); - }, + }; + + if let Some(new_state) = replace_with { + mem::replace(&mut *state, new_state); + } } } } @@ -440,12 +581,12 @@ impl Drop for ConnectionReuseDial where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { fn drop(&mut self) { + let shared = &self.shared; if let Some(outbound) = self.outbound.take() { - let mut shared = self.shared.lock(); - remove_one_substream(&mut *shared, outbound.connection_id, &outbound.client_addr); + shared.remove_substream(outbound.connection_id, &outbound.client_addr); } } } @@ -455,17 +596,17 @@ pub struct ConnectionReuseListener where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { /// The main listener. listener: stream::Fuse, /// Identifier for this listener. Used to determine which connections were opened by it. - listener_id: u64, + listener_id: usize, /// Opened connections that need to be upgraded. current_upgrades: FuturesUnordered>>, /// Shared between the whole connection reuse mechanism. - shared: Arc>>, + shared: Arc>, } impl Stream for ConnectionReuseListener @@ -474,10 +615,12 @@ where T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade, C::Output: StreamMuxer + Clone, + ::Substream : Clone, L: Stream, Lu: Future + 'static, { - type Item = FutureResult<(ConnectionReuseSubstream, FutureResult), IoError>; + type Item = FutureResult<(ConnectionReuseSubstream, + FutureResult), IoError>; type Error = IoError; fn poll(&mut self) -> Poll, Self::Error> { @@ -504,20 +647,8 @@ where // Process the connections being upgraded. loop { - match self.current_upgrades.poll() { - Ok(Async::Ready(Some((muxer, client_addr)))) => { - // Successfully upgraded a new incoming connection. - trace!("New multiplexed connection from {}", client_addr); - let mut shared = self.shared.lock(); - let next_incoming = muxer.clone().inbound(); - let connection_id = shared.next_connection_id; - shared.next_connection_id += 1; - let state = PeerState::Active { muxer, next_incoming, connection_id, listener_id: Some(self.listener_id), num_substreams: 1, client_addr: client_addr.clone() }; - shared.connections.insert(client_addr, state); - for to_notify in shared.notify_on_new_connec.drain() { - to_notify.1.notify(); - } - } + let (muxer, client_addr) = match self.current_upgrades.poll() { + Ok(Async::Ready(Some((muxer, client_addr)))) => (muxer, client_addr), Ok(Async::Ready(None)) | Ok(Async::NotReady) => { break; }, @@ -526,12 +657,24 @@ where debug!("Error while upgrading listener connection: {:?}", err); return Ok(Async::Ready(Some(future::err(err)))); } - } + }; + + + // Successfully upgraded a new incoming connection. + trace!("New multiplexed connection from {}", client_addr); + let connection_id = self.shared.gen_next_connection_id(); + + + self.shared.insert_connection(client_addr.clone(), + PeerState::Active { muxer, connection_id, + listener_id: Some(self.listener_id), + num_substreams: 1, + client_addr: client_addr }); } // Poll all the incoming connections on all the connections we opened. - let mut shared = self.shared.lock(); - match poll_incoming(&self.shared, &mut shared, Some(self.listener_id)) { + match self.shared.poll_incoming(Some(self.listener_id)) { + Ok(Async::NotReady) => Ok(Async::NotReady), Ok(Async::Ready(None)) => { if self.listener.is_done() && self.current_upgrades.is_empty() { Ok(Async::Ready(None)) @@ -539,14 +682,16 @@ where Ok(Async::NotReady) } }, - Ok(Async::Ready(Some(substream))) => { - Ok(Async::Ready(Some(substream))) - }, - Ok(Async::NotReady) => { - Ok(Async::NotReady) - } + Ok(Async::Ready(Some((inner, connection_id, addr)))) => + Ok(Async::Ready(Some(future::ok(( + ConnectionReuseSubstream { + inner: inner.clone(), + shared: self.shared.clone(), + connection_id: connection_id.clone(), + addr: addr.clone(), + }, future::ok(addr.clone())))))), Err(err) => { - Ok(Async::Ready(Some(future::err(err)))) + Ok(Async::Ready(Some(future::err(IoError::new(err.kind(), err.to_string()))))) } } } @@ -557,31 +702,38 @@ pub struct ConnectionReuseIncoming where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { // Shared between the whole connection reuse system. - shared: Arc>>, + shared: Arc>, } impl Future for ConnectionReuseIncoming where + T: Transport, + T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade, C::Output: StreamMuxer + Clone, + ::Substream : Clone, { - type Item = future::FutureResult<(ConnectionReuseSubstream, future::FutureResult), IoError>; + type Item = future::FutureResult<(ConnectionReuseSubstream, + future::FutureResult), IoError>; type Error = IoError; #[inline] fn poll(&mut self) -> Poll { - let mut shared = self.shared.lock(); - match poll_incoming(&self.shared, &mut shared, None) { - Ok(Async::Ready(Some(substream))) => { - Ok(Async::Ready(substream)) - }, + match self.shared.poll_incoming(None) { + Ok(Async::Ready(Some((inner, connection_id, addr)))) => + Ok(Async::Ready(future::ok(( + ConnectionReuseSubstream { + inner, + shared: self.shared.clone(), + connection_id, + addr: addr.clone(), + }, future::ok(addr))))), Ok(Async::Ready(None)) | Ok(Async::NotReady) => { - // TODO: will add an element to the list every time - shared.notify_on_new_connec.insert(TASK_ID.with(|&v| v), task::current()); + self.shared.notify_on_new_connec.lock().insert(TASK_ID.with(|&v| v), task::current()); Ok(Async::NotReady) }, Err(err) => Err(err) @@ -589,123 +741,19 @@ where } } -/// Polls the incoming substreams on all the incoming connections that match the `listener`. -/// -/// Returns `Ready(None)` if no connection is matching the `listener`. Returns `NotReady` if -/// one or more connections are matching the `listener` but they are not ready. -fn poll_incoming(shared_arc: &Arc>>, shared: &mut Shared, listener: Option) - -> Poll, FutureResult), IoError>>, IoError> -where - T: Transport, - C: ConnectionUpgrade, - C::Output: StreamMuxer + Clone, -{ - // Keys of the elements in `shared.connections` to remove afterwards. - let mut to_remove = Vec::new(); - // Substream to return, if any found. - let mut ret_value = None; - let mut found_one = false; - - for (addr, state) in shared.connections.iter_mut() { - match *state { - PeerState::Active { ref mut next_incoming, ref muxer, ref mut num_substreams, connection_id, ref client_addr, listener_id } => { - if listener_id != listener { - continue; - } - found_one = true; - match next_incoming.poll() { - Ok(Async::Ready(Some(inner))) => { - trace!("New incoming substream from {}", client_addr); - let next = muxer.clone().inbound(); - *next_incoming = next; - *num_substreams += 1; - let substream = ConnectionReuseSubstream { - inner, - shared: shared_arc.clone(), - connection_id, - addr: client_addr.clone(), - }; - ret_value = Some(Ok((substream, future::ok(client_addr.clone())))); - break; - }, - Ok(Async::Ready(None)) => { - // The muxer isn't capable of opening any inbound stream anymore, so - // we close the connection entirely. - trace!("Removing existing connection to {} as it cannot open inbound anymore", addr); - to_remove.push(addr.clone()); - }, - Ok(Async::NotReady) => (), - Err(err) => { - // If an error happens while opening an inbound stream, we close the - // connection entirely. - trace!("Error while opening inbound substream to {}: {:?}", addr, err); - to_remove.push(addr.clone()); - ret_value = Some(Err(err)); - break; - }, - } - }, - PeerState::Pending { ref mut notify, .. } => { - notify.insert(TASK_ID.with(|&t| t), task::current()); - }, - PeerState::Errored(_) => {}, - PeerState::Poisonned => { - panic!("Poisonned peer state"); - }, - } - } - - for to_remove in to_remove { - shared.connections.remove(&to_remove); - } - - match ret_value { - Some(Ok(val)) => Ok(Async::Ready(Some(future::ok(val)))), - Some(Err(err)) => Err(err), - None => { - if found_one { - Ok(Async::NotReady) - } else { - Ok(Async::Ready(None)) - } - }, - } -} - -/// Removes one substream from an active connection. Closes the connection if necessary. -fn remove_one_substream(shared: &mut Shared, connec_id: u64, addr: &Multiaddr) -where - T: Transport, - C: ConnectionUpgrade, - C::Output: StreamMuxer, -{ - shared.connections.retain(|_, connec| { - if let PeerState::Active { connection_id, ref mut num_substreams, .. } = connec { - if *connection_id == connec_id { - *num_substreams -= 1; - if *num_substreams == 0 { - trace!("All substreams to {} closed ; closing main connection", addr); - return false; - } - } - } - - true - }); -} /// Wraps around the `Substream`. pub struct ConnectionReuseSubstream where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { inner: ::Substream, - shared: Arc>>, + shared: Arc>, /// Id this connection was created from. - connection_id: u64, + connection_id: usize, /// Address of the remote. addr: Multiaddr, } @@ -714,7 +762,7 @@ impl Deref for ConnectionReuseSubstream where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { type Target = ::Substream; @@ -728,7 +776,7 @@ impl DerefMut for ConnectionReuseSubstream where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { #[inline] fn deref_mut(&mut self) -> &mut Self::Target { @@ -740,7 +788,7 @@ impl Read for ConnectionReuseSubstream where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { #[inline] fn read(&mut self, buf: &mut [u8]) -> Result { @@ -752,7 +800,7 @@ impl AsyncRead for ConnectionReuseSubstream where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { } @@ -760,7 +808,7 @@ impl Write for ConnectionReuseSubstream where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { #[inline] fn write(&mut self, buf: &[u8]) -> Result { @@ -777,7 +825,7 @@ impl AsyncWrite for ConnectionReuseSubstream where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { #[inline] fn shutdown(&mut self) -> Poll<(), IoError> { @@ -789,10 +837,9 @@ impl Drop for ConnectionReuseSubstream where T: Transport, C: ConnectionUpgrade, - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { fn drop(&mut self) { - let mut shared = self.shared.lock(); - remove_one_substream(&mut *shared, self.connection_id, &self.addr); + self.shared.remove_substream(self.connection_id, &self.addr); } } diff --git a/core/src/transport/upgrade.rs b/core/src/transport/upgrade.rs index 8c21897bfb6..dc3f7349f3f 100644 --- a/core/src/transport/upgrade.rs +++ b/core/src/transport/upgrade.rs @@ -50,14 +50,14 @@ impl<'a, T, C> UpgradedNode where T: Transport + 'a, T::Output: AsyncRead + AsyncWrite, - C: ConnectionUpgrade + 'a, + C: ConnectionUpgrade + 'a + Clone, { /// Turns this upgraded node into a `ConnectionReuse`. If the `Output` implements the /// `StreamMuxer` trait, the returned object will implement `Transport` and `MuxedTransport`. #[inline] pub fn into_connection_reuse(self) -> ConnectionReuse where - C::Output: StreamMuxer, + C::Output: StreamMuxer + Clone, { From::from(self) } From 6fd62039a6ccae269d5daa03f3517342db685384 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Wed, 1 Aug 2018 15:50:45 +0200 Subject: [PATCH 07/33] Fix code style, minor changes for clarity --- core/src/connection_reuse.rs | 375 +++++++++++++++++++++-------------- 1 file changed, 227 insertions(+), 148 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index dcf911ae1cf..b0fb41c8a87 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -41,15 +41,15 @@ use fnv::FnvHashMap; use futures::future::{self, FutureResult}; -use futures::{Async, Future, Poll, Stream, stream, task}; use futures::stream::FuturesUnordered; +use futures::{stream, task, Async, Future, Poll, Stream}; use multiaddr::Multiaddr; use muxing::StreamMuxer; use parking_lot::Mutex; use std::io::{Error as IoError, ErrorKind as IoErrorKind, Read, Write}; use std::mem; use std::ops::{Deref, DerefMut}; -use std::sync::{Arc, atomic::AtomicUsize, atomic::Ordering}; +use std::sync::{atomic::AtomicUsize, atomic::Ordering, Arc}; use tokio_io::{AsyncRead, AsyncWrite}; use transport::{MuxedTransport, Transport, UpgradedNode}; use upgrade::ConnectionUpgrade; @@ -71,7 +71,10 @@ where shared: Arc>, } -enum PeerState where M: StreamMuxer + Clone { +enum PeerState +where + M: StreamMuxer + Clone, +{ /// Connection is active and can be used to open substreams. Active { /// The muxer to open new substreams. @@ -99,10 +102,9 @@ enum PeerState where M: StreamMuxer + Clone { }, /// An earlier connection attempt errored. - Errored(IoError) + Errored(IoError), } - /// Struct shared between most of the `ConnectionReuse` infrastructure. // #[derive(Clone)] struct Shared @@ -140,7 +142,7 @@ where let mut notifiers = self.notify_on_new_connec.lock(); for to_notify in notifiers.drain() { to_notify.1.notify(); - }; + } } /// generate a new connection id @@ -156,27 +158,32 @@ where } /// Insert a new connection, returns Some if an entry was present, notify listeners - pub fn insert_connection(&self, addr: Multiaddr, state: PeerState) - -> Option> { - let r = self.replace_connection(addr, state); + pub fn insert_connection( + &self, + addr: Multiaddr, + state: PeerState, + ) -> Option> { + let r = self.connections.lock().insert(addr, state); self.new_con_notify(); r } - /// Replace a new connection, returns Some if an entry was present, **do not** notify listeners - pub fn replace_connection(&self, addr: Multiaddr, state: PeerState) - -> Option> { - self.connections.lock().insert(addr, state) - } - /// Removes one substream from an active connection. Closes the connection if necessary. pub fn remove_substream(&self, connec_id: usize, addr: &Multiaddr) { self.connections.lock().retain(|_, connec| { - if let PeerState::Active { connection_id, ref mut num_substreams, .. } = connec { + if let PeerState::Active { + connection_id, + ref mut num_substreams, + .. + } = connec + { if *connection_id == connec_id { *num_substreams -= 1; if *num_substreams == 0 { - trace!("All substreams to {} closed ; closing main connection", addr); + trace!( + "All substreams to {} closed ; closing main connection", + addr + ); return false; } } @@ -189,9 +196,10 @@ where /// /// Returns `Ready(None)` if no connection is matching the `listener`. Returns `NotReady` if /// one or more connections are matching the `listener` but they are not ready. - fn poll_incoming(&self, listener: Option) - -> Poll::Substream, usize, Multiaddr)>, IoError> - { + fn poll_incoming( + &self, + listener: Option, + ) -> Poll::Substream, usize, Multiaddr)>, IoError> { // Keys of the elements in `connections` to remove afterwards. let mut to_remove = Vec::new(); // Substream to return, if any found. @@ -199,42 +207,54 @@ where let mut found_one = false; for (addr, state) in self.connections.lock().iter_mut() { - let res = if let PeerState::Active { ref mut muxer, ref mut num_substreams, - connection_id, client_addr, listener_id } = state { - if *listener_id == listener { - continue; - } - found_one = true; + let res = { + if let PeerState::Active { + ref mut muxer, + ref mut num_substreams, + connection_id, + client_addr, + listener_id, + } = state + { + if *listener_id == listener { + continue; + } + found_one = true; - match muxer.clone().inbound().poll() { - Ok(Async::Ready(Some(inner))) => { - trace!("New incoming substream from {}", client_addr); - *num_substreams += 1; - Ok((connection_id.clone(), inner, client_addr.clone())) - }, - Err(err) => { - // If an error happens while opening an inbound stream, we close the - // connection entirely. - trace!("Error while opening inbound substream to {}: {:?}", addr, err); - to_remove.push(addr.clone()); - Err(err) - }, - Ok(Async::Ready(None)) => { - // The muxer isn't capable of opening any inbound stream anymore, so - // we close the connection entirely. - trace!("Removing existing connection to {} as it cannot open inbound anymore", addr); - to_remove.push(addr.clone()); - continue - }, - Ok(Async::NotReady) => { continue } - } - } else { - if listener.is_none() { - if let PeerState::Pending { ref mut notify, .. } = state { - notify.insert(TASK_ID.with(|&t| t), task::current()); + match muxer.clone().inbound().poll() { + Ok(Async::Ready(Some(inner))) => { + trace!("New incoming substream from {}", client_addr); + *num_substreams += 1; + Ok((inner, connection_id.clone(), client_addr.clone())) + } + Ok(Async::Ready(None)) => { + // The muxer isn't capable of opening any inbound stream anymore, so + // we close the connection entirely. + trace!("Removing existing connection to {} as it cannot open inbound anymore", addr); + to_remove.push(addr.clone()); + continue; + } + Ok(Async::NotReady) => continue, + Err(err) => { + // If an error happens while opening an inbound stream, we close the + // connection entirely. + trace!( + "Error while opening inbound substream to {}: {:?}", + addr, + err + ); + to_remove.push(addr.clone()); + Err(err) + } + } + } else { + if listener.is_none() { + if let PeerState::Pending { ref mut notify, .. } = state { + notify.insert(TASK_ID.with(|&t| t), task::current()); + } } + continue; } - continue }; ret_value = Some(res); @@ -246,8 +266,7 @@ where } match ret_value { - Some(Ok((connection_id, inner, addr))) => - Ok(Async::Ready(Some((inner, connection_id, addr)))), + Some(Ok(val)) => Ok(Async::Ready(Some(val))), Some(Err(err)) => Err(err), None => { if found_one { @@ -260,7 +279,6 @@ where } } - impl From> for ConnectionReuse where T: Transport, @@ -287,7 +305,7 @@ where T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade + Clone + 'static, // TODO: 'static :( C::Output: StreamMuxer + Clone, - ::Substream : Clone, + ::Substream: Clone, C::MultiaddrFuture: Future, C::NamesIter: Clone, UpgradedNode: Clone, @@ -299,7 +317,6 @@ where type Dial = ConnectionReuseDial; fn listen_on(self, addr: Multiaddr) -> Result<(Self::Listener, Multiaddr), (Self, Multiaddr)> { - let (listener, new_addr) = match self.shared.transport.clone().listen_on(addr.clone()) { Ok((l, a)) => (l, a), Err((_, addr)) => { @@ -335,14 +352,17 @@ where #[inline] fn dial(self, addr: Multiaddr) -> Result { - // If an earlier attempt to dial this multiaddress failed, we clear the error. Otherwise // the returned `Future` will immediately produce the error. let must_clear = match self.shared.connections.lock().get(&addr) { Some(&PeerState::Errored(ref err)) => { - trace!("Clearing existing connection to {} which errored earlier: {:?}", addr, err); + trace!( + "Clearing existing connection to {} which errored earlier: {:?}", + addr, + err + ); true - }, + } _ => false, }; if must_clear { @@ -358,7 +378,10 @@ where #[inline] fn nat_traversal(&self, server: &Multiaddr, observed: &Multiaddr) -> Option { - self.shared.transport.transport().nat_traversal(server, observed) + self.shared + .transport + .transport() + .nat_traversal(server, observed) } } @@ -368,14 +391,14 @@ where T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade + Clone + 'static, // TODO: 'static :( C::Output: StreamMuxer + Clone, - ::Substream : Clone, + ::Substream: Clone, C::MultiaddrFuture: Future, C::NamesIter: Clone, UpgradedNode: Clone, { type Incoming = ConnectionReuseIncoming; type IncomingUpgrade = - future::FutureResult<(ConnectionReuseSubstream, Self::MultiaddrFuture), IoError>; + future::FutureResult<(ConnectionReuseSubstream, Self::MultiaddrFuture), IoError>; #[inline] fn next_incoming(self) -> Self::Incoming { @@ -393,7 +416,7 @@ task_local!{ /// Implementation of `Future` for dialing a node. pub struct ConnectionReuseDial -where +where T: Transport, C: ConnectionUpgrade, C::Output: StreamMuxer + Clone, @@ -411,7 +434,7 @@ where } struct ConnectionReuseDialOut -where +where T: Transport, C: ConnectionUpgrade, C::Output: StreamMuxer + Clone, @@ -425,16 +448,19 @@ where } impl Future for ConnectionReuseDial -where +where T: Transport, C: ConnectionUpgrade, C::Output: StreamMuxer + Clone + 'static, - ::Substream : Clone, + ::Substream: Clone, UpgradedNode: Transport + Clone, as Transport>::Dial: 'static, as Transport>::MultiaddrFuture: 'static, { - type Item = (ConnectionReuseSubstream, FutureResult); + type Item = ( + ConnectionReuseSubstream, + FutureResult, + ); type Error = IoError; fn poll(&mut self) -> Poll { @@ -445,50 +471,58 @@ where Ok(Async::Ready(Some(inner))) => { trace!("Opened new outgoing substream to {}", self.addr); let shared = self.shared.clone(); - return Ok(Async::Ready((ConnectionReuseSubstream { - connection_id: outbound.connection_id, - inner, - shared, - addr: outbound.client_addr.clone(), - }, future::ok(outbound.client_addr)))); - }, + return Ok(Async::Ready(( + ConnectionReuseSubstream { + connection_id: outbound.connection_id, + inner, + shared, + addr: outbound.client_addr.clone(), + }, + future::ok(outbound.client_addr), + ))); + } Ok(Async::NotReady) => { self.outbound = Some(outbound); return Ok(Async::NotReady); - }, + } Ok(Async::Ready(None)) => { // The muxer can no longer produce outgoing substreams. // Let's reopen a connection. trace!("Closing existing connection to {} ; can't produce outgoing substreams", self.addr); true - }, + } Err(err) => { // If we get an error while opening a substream, we decide to ignore it // and open a new muxer. // If opening the muxer produces an error, *then* we will return it. - debug!("Error while opening outgoing substream to {}: {:?}", self.addr, err); + debug!( + "Error while opening outgoing substream to {}: {:?}", + self.addr, err + ); true - }, + } } - }, - _ => false + } + _ => false, }; - let is_empty = { let mut conns = self.shared.connections.lock(); if should_kill_existing_muxer { conns.remove(&self.addr); true - } else { let item = conns.get(&self.addr); // error'ed, quit early if let Some(PeerState::Errored(err)) = item { - trace!("Existing new connection to {} errored earlier: {:?}", self.addr, err); + trace!( + "Existing new connection to {} errored earlier: {:?}", + self.addr, + err + ); let io_err = IoError::new(err.kind(), err.to_string()); - return Err(io_err) + return Err(io_err); }; item.is_none() @@ -503,13 +537,21 @@ where let future = future.and_then(|(out, addr)| addr.map(move |a| (out, a))); let future = Box::new(future); let connection_id = self.shared.gen_next_connection_id(); - PeerState::Pending { future, notify: Default::default(), connection_id } - }, + PeerState::Pending { + future, + notify: Default::default(), + connection_id, + } + } Err(_) => { - trace!("Failed to open connection to {:?}, multiaddr not supported", self.addr); - let err = IoError::new(IoErrorKind::ConnectionRefused, "multiaddr not supported"); + trace!( + "Failed to open connection to {:?}, multiaddr not supported", + self.addr + ); + let err = + IoError::new(IoErrorKind::ConnectionRefused, "multiaddr not supported"); PeerState::Errored(err) - }, + } }; self.shared.insert_connection(self.addr.clone(), state); @@ -518,24 +560,41 @@ where let mut conn = self.shared.connections.lock(); if let Some(state) = conn.get_mut(&self.addr) { - - let replace_with = { + let replace_with = { match state { - PeerState::Active { muxer, connection_id, client_addr, ref mut num_substreams, .. } => { - trace!("Using existing connection to {} to open outbound substream", self.addr); + PeerState::Active { + muxer, + connection_id, + client_addr, + ref mut num_substreams, + .. + } => { + trace!( + "Using existing connection to {} to open outbound substream", + self.addr + ); self.outbound = Some(ConnectionReuseDialOut { stream: muxer.clone().outbound(), connection_id: connection_id.clone(), client_addr: client_addr.clone(), }); + // mutating the param in place, nothing to be done *num_substreams += 1; None - }, - PeerState::Pending { ref mut future, ref mut notify, connection_id } => { + } + PeerState::Pending { + ref mut future, + ref mut notify, + connection_id, + } => { match future.poll() { Ok(Async::Ready((muxer, client_addr))) => { - trace!("Successful new connection to {} ({})", self.addr, client_addr); + trace!( + "Successful new connection to {} ({})", + self.addr, + client_addr + ); for task in notify { task.1.notify(); } @@ -546,25 +605,30 @@ where client_addr: client_addr.clone(), }); - Some(PeerState::Active { muxer, connection_id: connection_id.clone(), - num_substreams: 1, listener_id: None, - client_addr }) - }, + // our connection was upgraded, replace it. + Some(PeerState::Active { + muxer, + connection_id: connection_id.clone(), + num_substreams: 1, + listener_id: None, + client_addr, + }) + } Ok(Async::NotReady) => { notify.insert(TASK_ID.with(|&t| t), task::current()); return Ok(Async::NotReady); - }, + } Err(err) => { trace!("Failed new connection to {}: {:?}", self.addr, err); Some(PeerState::Errored(err)) - }, + } } - }, + } _ => { // Because of a race someone might has changed the Peerstate between // this get_mut and the previous lock. But no harm for us, we can just // loop on and will find it soon enough. - continue + continue; } } }; @@ -578,15 +642,14 @@ where } impl Drop for ConnectionReuseDial -where +where T: Transport, C: ConnectionUpgrade, C::Output: StreamMuxer + Clone, { fn drop(&mut self) { - let shared = &self.shared; if let Some(outbound) = self.outbound.take() { - shared.remove_substream(outbound.connection_id, &outbound.client_addr); + self.shared.remove_substream(outbound.connection_id, &outbound.client_addr); } } } @@ -615,12 +678,17 @@ where T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade, C::Output: StreamMuxer + Clone, - ::Substream : Clone, + ::Substream: Clone, L: Stream, Lu: Future + 'static, { - type Item = FutureResult<(ConnectionReuseSubstream, - FutureResult), IoError>; + type Item = FutureResult< + ( + ConnectionReuseSubstream, + FutureResult, + ), + IoError, + >; type Error = IoError; fn poll(&mut self) -> Poll, Self::Error> { @@ -649,9 +717,7 @@ where loop { let (muxer, client_addr) = match self.current_upgrades.poll() { Ok(Async::Ready(Some((muxer, client_addr)))) => (muxer, client_addr), - Ok(Async::Ready(None)) | Ok(Async::NotReady) => { - break; - }, + Ok(Async::Ready(None)) | Ok(Async::NotReady) => break, Err(err) => { // Insert the rest of the pending upgrades, but not the current one. debug!("Error while upgrading listener connection: {:?}", err); @@ -659,17 +725,20 @@ where } }; - // Successfully upgraded a new incoming connection. trace!("New multiplexed connection from {}", client_addr); let connection_id = self.shared.gen_next_connection_id(); - - self.shared.insert_connection(client_addr.clone(), - PeerState::Active { muxer, connection_id, - listener_id: Some(self.listener_id), - num_substreams: 1, - client_addr: client_addr }); + self.shared.insert_connection( + client_addr.clone(), + PeerState::Active { + muxer, + connection_id, + listener_id: Some(self.listener_id), + num_substreams: 1, + client_addr: client_addr, + }, + ); } // Poll all the incoming connections on all the connections we opened. @@ -681,18 +750,22 @@ where } else { Ok(Async::NotReady) } - }, - Ok(Async::Ready(Some((inner, connection_id, addr)))) => + } + Ok(Async::Ready(Some((inner, connection_id, addr)))) => { Ok(Async::Ready(Some(future::ok(( ConnectionReuseSubstream { - inner: inner.clone(), - shared: self.shared.clone(), - connection_id: connection_id.clone(), - addr: addr.clone(), - }, future::ok(addr.clone())))))), - Err(err) => { - Ok(Async::Ready(Some(future::err(IoError::new(err.kind(), err.to_string()))))) + inner: inner.clone(), + shared: self.shared.clone(), + connection_id: connection_id.clone(), + addr: addr.clone(), + }, + future::ok(addr.clone()), + ))))) } + Err(err) => Ok(Async::Ready(Some(future::err(IoError::new( + err.kind(), + err.to_string(), + ))))), } } } @@ -710,39 +783,45 @@ where impl Future for ConnectionReuseIncoming where - T: Transport, T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade, C::Output: StreamMuxer + Clone, - ::Substream : Clone, + ::Substream: Clone, { - type Item = future::FutureResult<(ConnectionReuseSubstream, - future::FutureResult), IoError>; + type Item = future::FutureResult< + ( + ConnectionReuseSubstream, + future::FutureResult, + ), + IoError, + >; type Error = IoError; #[inline] fn poll(&mut self) -> Poll { match self.shared.poll_incoming(None) { - Ok(Async::Ready(Some((inner, connection_id, addr)))) => - Ok(Async::Ready(future::ok(( - ConnectionReuseSubstream { - inner, - shared: self.shared.clone(), - connection_id, - addr: addr.clone(), - }, future::ok(addr))))), + Ok(Async::Ready(Some((inner, connection_id, addr)))) => Ok(Async::Ready(future::ok(( + ConnectionReuseSubstream { + inner, + shared: self.shared.clone(), + connection_id, + addr: addr.clone(), + }, + future::ok(addr), + )))), Ok(Async::Ready(None)) | Ok(Async::NotReady) => { - self.shared.notify_on_new_connec.lock().insert(TASK_ID.with(|&v| v), task::current()); + self.shared + .notify_on_new_connec + .lock() + .insert(TASK_ID.with(|&v| v), task::current()); Ok(Async::NotReady) - }, - Err(err) => Err(err) + } + Err(err) => Err(err), } } } - - /// Wraps around the `Substream`. pub struct ConnectionReuseSubstream where From ed523df76ec3f657e8594e3e0efc0ce71df4ab07 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Wed, 1 Aug 2018 16:46:04 +0200 Subject: [PATCH 08/33] Explicit Counter --- core/src/connection_reuse.rs | 41 ++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index b0fb41c8a87..f30c2f14f80 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -71,6 +71,17 @@ where shared: Arc>, } +#[derive(Default)] +struct Counter { + inner: AtomicUsize +} + +impl Counter { + pub fn next(&self) -> usize { + self.inner.fetch_add(1, Ordering::Relaxed) + } +} + enum PeerState where M: StreamMuxer + Clone, @@ -123,11 +134,11 @@ where /// Tasks to notify when one or more new elements were added to `connections`. notify_on_new_connec: Mutex>, - /// Next `connection_id` to use when opening a connection. - next_connection_id: AtomicUsize, + /// Counter giving us the next connection_id + connection_counter: Counter, - /// Next `listener_id` for the next listener we create. - next_listener_id: AtomicUsize, + /// Counter giving us the next listener_id + listener_counter: Counter, } impl Shared @@ -145,18 +156,6 @@ where } } - /// generate a new connection id - #[inline] - pub fn gen_next_connection_id(&self) -> usize { - self.next_connection_id.fetch_add(1, Ordering::Relaxed) - } - - /// generate a new listener id - #[inline] - pub fn gen_next_listener_id(&self) -> usize { - self.next_listener_id.fetch_add(1, Ordering::Relaxed) - } - /// Insert a new connection, returns Some if an entry was present, notify listeners pub fn insert_connection( &self, @@ -292,8 +291,8 @@ where transport: node, connections: Default::default(), notify_on_new_connec: Default::default(), - next_connection_id: Default::default(), - next_listener_id: Default::default(), + connection_counter: Default::default(), + listener_counter: Default::default(), }), } } @@ -338,7 +337,7 @@ where }) .fuse(); - let listener_id = self.shared.gen_next_listener_id(); + let listener_id = self.shared.listener_counter.next(); let listener = ConnectionReuseListener { shared: self.shared, @@ -536,7 +535,7 @@ where trace!("Opened new connection to {:?}", self.addr); let future = future.and_then(|(out, addr)| addr.map(move |a| (out, a))); let future = Box::new(future); - let connection_id = self.shared.gen_next_connection_id(); + let connection_id = self.shared.connection_counter.next(); PeerState::Pending { future, notify: Default::default(), @@ -727,7 +726,7 @@ where // Successfully upgraded a new incoming connection. trace!("New multiplexed connection from {}", client_addr); - let connection_id = self.shared.gen_next_connection_id(); + let connection_id = self.shared.connection_counter.next(); self.shared.insert_connection( client_addr.clone(), From 48e79d37cc59a3c2549361ce49c745d725d02c62 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Wed, 1 Aug 2018 16:47:06 +0200 Subject: [PATCH 09/33] Fully lock poll_incoming to enforce consistency --- core/src/connection_reuse.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index f30c2f14f80..90538f4b191 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -204,8 +204,9 @@ where // Substream to return, if any found. let mut ret_value = None; let mut found_one = false; + let mut conn = self.connections.lock(); - for (addr, state) in self.connections.lock().iter_mut() { + for (addr, state) in conn.iter_mut() { let res = { if let PeerState::Active { ref mut muxer, @@ -261,7 +262,7 @@ where } for to_remove in to_remove { - self.connections.lock().remove(&to_remove); + conn.remove(&to_remove); } match ret_value { From 1156e023d2e77bf3a6641ba07225bf36695007a0 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Wed, 1 Aug 2018 18:20:17 +0200 Subject: [PATCH 10/33] also move the poll_outbound into the manager --- core/src/connection_reuse.rs | 314 ++++++++++++++++++----------------- 1 file changed, 165 insertions(+), 149 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 90538f4b191..891137ba98c 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -47,9 +47,9 @@ use multiaddr::Multiaddr; use muxing::StreamMuxer; use parking_lot::Mutex; use std::io::{Error as IoError, ErrorKind as IoErrorKind, Read, Write}; -use std::mem; use std::ops::{Deref, DerefMut}; use std::sync::{atomic::AtomicUsize, atomic::Ordering, Arc}; +use std::collections::hash_map::Entry; use tokio_io::{AsyncRead, AsyncWrite}; use transport::{MuxedTransport, Transport, UpgradedNode}; use upgrade::ConnectionUpgrade; @@ -90,8 +90,6 @@ where Active { /// The muxer to open new substreams. muxer: M, - /// Future of the address of the client. - client_addr: Multiaddr, /// Unique identifier for this connection in the `ConnectionReuse`. connection_id: usize, /// Number of open substreams. @@ -190,6 +188,134 @@ where true }); } +} + +impl Shared +where + T: Transport + Clone, + C: ConnectionUpgrade + Clone, + C::Output: StreamMuxer + Clone + 'static, + UpgradedNode: Transport + Clone, + as Transport>::Dial: 'static, + as Transport>::MultiaddrFuture: 'static, +{ + fn poll_outbound(&self, + addr: Multiaddr, + reset: bool) -> Poll<(::OutboundSubstream, usize), IoError> { + + let mut conns = self.connections.lock(); + + if reset { + conns.remove(&addr); + } + + match conns.entry(addr.clone()) { + Entry::Vacant(e) => { + // Empty: dial, keep in mind, which task wanted to be notified, + // then return with `NotReady` + e.insert(match self.transport.clone().dial(addr.clone()) { + Ok(future) => { + trace!("Opened new connection to {:?}", addr); + let future = future.and_then(|(out, addr)| addr.map(move |a| (out, a))); + let future = Box::new(future); + let connection_id = self.connection_counter.next(); + + // make sure we are woken up once this connects + let mut notify : FnvHashMap = Default::default(); + notify.insert(TASK_ID.with(|&t| t), task::current()); + + PeerState::Pending { + future, + notify, + connection_id, + } + } + Err(_) => { + trace!( + "Failed to open connection to {:?}, multiaddr not supported", + addr + ); + let err = + IoError::new(IoErrorKind::ConnectionRefused, "multiaddr not supported"); + PeerState::Errored(err) + } + }); + self.new_con_notify(); + + Ok(Async::NotReady) + } + Entry::Occupied(mut e) => { + let (replace_with, return_with) = match e.get_mut() { + PeerState::Errored(err) => { + // Error: return it + let io_err = IoError::new(err.kind(), err.to_string()); + return Err(io_err); + } + PeerState::Active { + muxer, + connection_id, + ref mut num_substreams, + .. + } => { + // perfect, let's reuse, update the stream and + // return the new ReuseDialOut + trace!( + "Using existing connection to {} to open outbound substream", + addr + ); + // mutating the param in place, nothing to be done + *num_substreams += 1; + return Ok(Async::Ready( (muxer.clone().outbound(), connection_id.clone()))); + } + PeerState::Pending { + ref mut future, + ref mut notify, + connection_id, + } => { + // was pending, let's check if that has changed + match future.poll() { + Ok(Async::NotReady) => { + // no it still isn't ready, keep the current task + // informed but return with NotReady + notify.insert(TASK_ID.with(|&t| t), task::current()); + return Ok(Async::NotReady); + } + Err(err) => { + // well, looks like connecting failed, replace the + // entry then return the Error + trace!("Failed new connection to {}: {:?}", addr, err); + let io_err = IoError::new(err.kind(), err.to_string()); + (PeerState::Errored(io_err), Err(err)) + } + Ok(Async::Ready((muxer, client_addr))) => { + trace!( + "Successful new connection to {} ({})", + addr, + client_addr + ); + for task in notify { + task.1.notify(); + } + let first_outbound = muxer.clone().outbound(); + + // our connection was upgraded, replace it. + (PeerState::Active { + muxer, + connection_id: connection_id.clone(), + num_substreams: 1, + listener_id: None + }, Ok(Async::Ready( (first_outbound, *connection_id)))) + } + } + } + }; + + e.insert(replace_with); + + return_with + } + } + } /// Polls the incoming substreams on all the incoming connections that match the `listener`. /// @@ -212,7 +338,6 @@ where ref mut muxer, ref mut num_substreams, connection_id, - client_addr, listener_id, } = state { @@ -223,9 +348,9 @@ where match muxer.clone().inbound().poll() { Ok(Async::Ready(Some(inner))) => { - trace!("New incoming substream from {}", client_addr); + trace!("New incoming substream from {}", addr); *num_substreams += 1; - Ok((inner, connection_id.clone(), client_addr.clone())) + Ok((inner, connection_id.clone(), addr.clone())) } Ok(Async::Ready(None)) => { // The muxer isn't capable of opening any inbound stream anymore, so @@ -301,11 +426,12 @@ where impl Transport for ConnectionReuse where - T: Transport + 'static, // TODO: 'static :( + T: Transport + Clone + 'static, T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade + Clone + 'static, // TODO: 'static :( C::Output: StreamMuxer + Clone, ::Substream: Clone, + ::OutboundSubstream: Clone, C::MultiaddrFuture: Future, C::NamesIter: Clone, UpgradedNode: Clone, @@ -387,11 +513,12 @@ where impl MuxedTransport for ConnectionReuse where - T: Transport + 'static, // TODO: 'static :( + T: Transport + Clone + 'static, // TODO: 'static :( T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade + Clone + 'static, // TODO: 'static :( C::Output: StreamMuxer + Clone, ::Substream: Clone, + ::OutboundSubstream: Clone, C::MultiaddrFuture: Future, C::NamesIter: Clone, UpgradedNode: Clone, @@ -449,10 +576,11 @@ where impl Future for ConnectionReuseDial where - T: Transport, - C: ConnectionUpgrade, + T: Transport + Clone, + C: ConnectionUpgrade + Clone, C::Output: StreamMuxer + Clone + 'static, ::Substream: Clone, + ::OutboundSubstream: Clone, UpgradedNode: Transport + Clone, as Transport>::Dial: 'static, as Transport>::MultiaddrFuture: 'static, @@ -481,10 +609,7 @@ where future::ok(outbound.client_addr), ))); } - Ok(Async::NotReady) => { - self.outbound = Some(outbound); - return Ok(Async::NotReady); - } + Ok(Async::NotReady) => return Ok(Async::NotReady), Ok(Async::Ready(None)) => { // The muxer can no longer produce outgoing substreams. // Let's reopen a connection. @@ -506,135 +631,19 @@ where _ => false, }; - let is_empty = { - let mut conns = self.shared.connections.lock(); - if should_kill_existing_muxer { - conns.remove(&self.addr); - true - } else { - let item = conns.get(&self.addr); - - // error'ed, quit early - if let Some(PeerState::Errored(err)) = item { - trace!( - "Existing new connection to {} errored earlier: {:?}", - self.addr, - err - ); - let io_err = IoError::new(err.kind(), err.to_string()); - return Err(io_err); - }; - - item.is_none() + match self.shared.poll_outbound(self.addr.clone(), should_kill_existing_muxer) { + Ok(Async::Ready((stream, connection_id))) => { + self.outbound = Some(ConnectionReuseDialOut { + stream: stream.clone(), + connection_id, + client_addr: self.addr.clone(), + }) } - }; - - if is_empty { - // dial new and return early - let state = match self.shared.transport.clone().dial(self.addr.clone()) { - Ok(future) => { - trace!("Opened new connection to {:?}", self.addr); - let future = future.and_then(|(out, addr)| addr.map(move |a| (out, a))); - let future = Box::new(future); - let connection_id = self.shared.connection_counter.next(); - PeerState::Pending { - future, - notify: Default::default(), - connection_id, - } - } - Err(_) => { - trace!( - "Failed to open connection to {:?}, multiaddr not supported", - self.addr - ); - let err = - IoError::new(IoErrorKind::ConnectionRefused, "multiaddr not supported"); - PeerState::Errored(err) - } - }; - - self.shared.insert_connection(self.addr.clone(), state); - return Ok(Async::NotReady); - } - - let mut conn = self.shared.connections.lock(); - if let Some(state) = conn.get_mut(&self.addr) { - let replace_with = { - match state { - PeerState::Active { - muxer, - connection_id, - client_addr, - ref mut num_substreams, - .. - } => { - trace!( - "Using existing connection to {} to open outbound substream", - self.addr - ); - self.outbound = Some(ConnectionReuseDialOut { - stream: muxer.clone().outbound(), - connection_id: connection_id.clone(), - client_addr: client_addr.clone(), - }); - - // mutating the param in place, nothing to be done - *num_substreams += 1; - None - } - PeerState::Pending { - ref mut future, - ref mut notify, - connection_id, - } => { - match future.poll() { - Ok(Async::Ready((muxer, client_addr))) => { - trace!( - "Successful new connection to {} ({})", - self.addr, - client_addr - ); - for task in notify { - task.1.notify(); - } - let first_outbound = muxer.clone().outbound(); - self.outbound = Some(ConnectionReuseDialOut { - stream: first_outbound, - connection_id: connection_id.clone(), - client_addr: client_addr.clone(), - }); - - // our connection was upgraded, replace it. - Some(PeerState::Active { - muxer, - connection_id: connection_id.clone(), - num_substreams: 1, - listener_id: None, - client_addr, - }) - } - Ok(Async::NotReady) => { - notify.insert(TASK_ID.with(|&t| t), task::current()); - return Ok(Async::NotReady); - } - Err(err) => { - trace!("Failed new connection to {}: {:?}", self.addr, err); - Some(PeerState::Errored(err)) - } - } - } - _ => { - // Because of a race someone might has changed the Peerstate between - // this get_mut and the previous lock. But no harm for us, we can just - // loop on and will find it soon enough. - continue; - } - } - }; - - if let Some(new_state) = replace_with { - mem::replace(&mut *state, new_state); + Ok(Async::NotReady) => { + return Ok(Async::NotReady) + } + Err(err) => { + return Err(err) } } } @@ -674,11 +683,15 @@ where impl Stream for ConnectionReuseListener where - T: Transport, + T: Transport + Clone, T::Output: AsyncRead + AsyncWrite, - C: ConnectionUpgrade, - C::Output: StreamMuxer + Clone, + C: ConnectionUpgrade + Clone, + C::Output: StreamMuxer + Clone + 'static, ::Substream: Clone, + ::OutboundSubstream: Clone, + UpgradedNode: Transport + Clone, + as Transport>::Dial: 'static, + as Transport>::MultiaddrFuture: 'static, L: Stream, Lu: Future + 'static, { @@ -736,7 +749,6 @@ where connection_id, listener_id: Some(self.listener_id), num_substreams: 1, - client_addr: client_addr, }, ); } @@ -783,11 +795,15 @@ where impl Future for ConnectionReuseIncoming where - T: Transport, + T: Transport + Clone, T::Output: AsyncRead + AsyncWrite, - C: ConnectionUpgrade, - C::Output: StreamMuxer + Clone, + C: ConnectionUpgrade + Clone, + C::Output: StreamMuxer + Clone + 'static, ::Substream: Clone, + ::OutboundSubstream: Clone, + UpgradedNode: Transport + Clone, + as Transport>::Dial: 'static, + as Transport>::MultiaddrFuture: 'static, { type Item = future::FutureResult< ( From f8b226905e05bacf93a3577ff9a0313b4e8ae8e7 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Wed, 1 Aug 2018 18:25:23 +0200 Subject: [PATCH 11/33] Rename Shared -> ConnectionsManager --- core/src/connection_reuse.rs | 66 ++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 891137ba98c..8d3eee45c78 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -68,7 +68,7 @@ where C::Output: StreamMuxer + Clone, { /// Struct shared between most of the `ConnectionReuse` infrastructure. - shared: Arc>, + manager: Arc>, } #[derive(Default)] @@ -116,7 +116,7 @@ where /// Struct shared between most of the `ConnectionReuse` infrastructure. // #[derive(Clone)] -struct Shared +struct ConnectionsManager where T: Transport, C: ConnectionUpgrade, @@ -139,7 +139,7 @@ where listener_counter: Counter, } -impl Shared +impl ConnectionsManager where T: Transport, C: ConnectionUpgrade, @@ -190,7 +190,7 @@ where } } -impl Shared +impl ConnectionsManager where T: Transport + Clone, C: ConnectionUpgrade + Clone, @@ -413,7 +413,7 @@ where #[inline] fn from(node: UpgradedNode) -> ConnectionReuse { ConnectionReuse { - shared: Arc::new(Shared { + manager: Arc::new(ConnectionsManager { transport: node, connections: Default::default(), notify_on_new_connec: Default::default(), @@ -443,12 +443,12 @@ where type Dial = ConnectionReuseDial; fn listen_on(self, addr: Multiaddr) -> Result<(Self::Listener, Multiaddr), (Self, Multiaddr)> { - let (listener, new_addr) = match self.shared.transport.clone().listen_on(addr.clone()) { + let (listener, new_addr) = match self.manager.transport.clone().listen_on(addr.clone()) { Ok((l, a)) => (l, a), Err((_, addr)) => { return Err(( ConnectionReuse { - shared: self.shared, + manager: self.manager, }, addr, )); @@ -464,10 +464,10 @@ where }) .fuse(); - let listener_id = self.shared.listener_counter.next(); + let listener_id = self.manager.listener_counter.next(); let listener = ConnectionReuseListener { - shared: self.shared, + manager: self.manager, listener, listener_id, current_upgrades: FuturesUnordered::new(), @@ -480,7 +480,7 @@ where fn dial(self, addr: Multiaddr) -> Result { // If an earlier attempt to dial this multiaddress failed, we clear the error. Otherwise // the returned `Future` will immediately produce the error. - let must_clear = match self.shared.connections.lock().get(&addr) { + let must_clear = match self.manager.connections.lock().get(&addr) { Some(&PeerState::Errored(ref err)) => { trace!( "Clearing existing connection to {} which errored earlier: {:?}", @@ -492,19 +492,19 @@ where _ => false, }; if must_clear { - self.shared.connections.lock().remove(&addr); + self.manager.connections.lock().remove(&addr); } Ok(ConnectionReuseDial { outbound: None, - shared: self.shared, + manager: self.manager, addr, }) } #[inline] fn nat_traversal(&self, server: &Multiaddr, observed: &Multiaddr) -> Option { - self.shared + self.manager .transport .transport() .nat_traversal(server, observed) @@ -530,7 +530,7 @@ where #[inline] fn next_incoming(self) -> Self::Incoming { ConnectionReuseIncoming { - shared: self.shared, + manager: self.manager, } } } @@ -553,8 +553,8 @@ where /// If `None`, we need to grab a new outbound substream from the muxer. outbound: Option>, - // Shared between the whole connection reuse mechanism. - shared: Arc>, + // ConnectionsManager between the whole connection reuse mechanism. + manager: Arc>, // The address we're trying to dial. addr: Multiaddr, @@ -598,12 +598,12 @@ where match outbound.stream.poll() { Ok(Async::Ready(Some(inner))) => { trace!("Opened new outgoing substream to {}", self.addr); - let shared = self.shared.clone(); + let manager = self.manager.clone(); return Ok(Async::Ready(( ConnectionReuseSubstream { connection_id: outbound.connection_id, inner, - shared, + manager, addr: outbound.client_addr.clone(), }, future::ok(outbound.client_addr), @@ -631,7 +631,7 @@ where _ => false, }; - match self.shared.poll_outbound(self.addr.clone(), should_kill_existing_muxer) { + match self.manager.poll_outbound(self.addr.clone(), should_kill_existing_muxer) { Ok(Async::Ready((stream, connection_id))) => { self.outbound = Some(ConnectionReuseDialOut { stream: stream.clone(), @@ -658,7 +658,7 @@ where { fn drop(&mut self) { if let Some(outbound) = self.outbound.take() { - self.shared.remove_substream(outbound.connection_id, &outbound.client_addr); + self.manager.remove_substream(outbound.connection_id, &outbound.client_addr); } } } @@ -677,8 +677,8 @@ where /// Opened connections that need to be upgraded. current_upgrades: FuturesUnordered>>, - /// Shared between the whole connection reuse mechanism. - shared: Arc>, + /// ConnectionsManager between the whole connection reuse mechanism. + manager: Arc>, } impl Stream for ConnectionReuseListener @@ -740,9 +740,9 @@ where // Successfully upgraded a new incoming connection. trace!("New multiplexed connection from {}", client_addr); - let connection_id = self.shared.connection_counter.next(); + let connection_id = self.manager.connection_counter.next(); - self.shared.insert_connection( + self.manager.insert_connection( client_addr.clone(), PeerState::Active { muxer, @@ -754,7 +754,7 @@ where } // Poll all the incoming connections on all the connections we opened. - match self.shared.poll_incoming(Some(self.listener_id)) { + match self.manager.poll_incoming(Some(self.listener_id)) { Ok(Async::NotReady) => Ok(Async::NotReady), Ok(Async::Ready(None)) => { if self.listener.is_done() && self.current_upgrades.is_empty() { @@ -767,7 +767,7 @@ where Ok(Async::Ready(Some(future::ok(( ConnectionReuseSubstream { inner: inner.clone(), - shared: self.shared.clone(), + manager: self.manager.clone(), connection_id: connection_id.clone(), addr: addr.clone(), }, @@ -789,8 +789,8 @@ where C: ConnectionUpgrade, C::Output: StreamMuxer + Clone, { - // Shared between the whole connection reuse system. - shared: Arc>, + // ConnectionsManager between the whole connection reuse system. + manager: Arc>, } impl Future for ConnectionReuseIncoming @@ -816,18 +816,18 @@ where #[inline] fn poll(&mut self) -> Poll { - match self.shared.poll_incoming(None) { + match self.manager.poll_incoming(None) { Ok(Async::Ready(Some((inner, connection_id, addr)))) => Ok(Async::Ready(future::ok(( ConnectionReuseSubstream { inner, - shared: self.shared.clone(), + manager: self.manager.clone(), connection_id, addr: addr.clone(), }, future::ok(addr), )))), Ok(Async::Ready(None)) | Ok(Async::NotReady) => { - self.shared + self.manager .notify_on_new_connec .lock() .insert(TASK_ID.with(|&v| v), task::current()); @@ -846,7 +846,7 @@ where C::Output: StreamMuxer + Clone, { inner: ::Substream, - shared: Arc>, + manager: Arc>, /// Id this connection was created from. connection_id: usize, /// Address of the remote. @@ -935,6 +935,6 @@ where C::Output: StreamMuxer + Clone, { fn drop(&mut self) { - self.shared.remove_substream(self.connection_id, &self.addr); + self.manager.remove_substream(self.connection_id, &self.addr); } } From 772ef621e931388d0fbc3cfc7d7b5cfeb10d42c3 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Wed, 1 Aug 2018 22:24:03 +0200 Subject: [PATCH 12/33] move the last external references into the manager --- core/src/connection_reuse.rs | 84 +++++++++++++++++++++++++----------- 1 file changed, 59 insertions(+), 25 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 8d3eee45c78..1f5a14d6f1e 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -188,17 +188,70 @@ where true }); } + + /// Clear the cached connection if the entry contains an error. Returns whether an error was + /// found and has been removed. + fn clear_error(&self, addr: Multiaddr) -> bool { + let mut conns = self.connections.lock(); + + if let Entry::Occupied(e) = conns.entry(addr) { + if let PeerState::Errored(ref err) = e.get() { + trace!("Clearing existing connection to {} which errored earlier: {:?}", e.key(), err); + } else { + return false// nothing to do, quit + } + + // clear the error + e.remove(); + true + } else { + false + } + } } impl ConnectionsManager where - T: Transport + Clone, + T: Transport, C: ConnectionUpgrade + Clone, C::Output: StreamMuxer + Clone + 'static, UpgradedNode: Transport + Clone, as Transport>::Dial: 'static, as Transport>::MultiaddrFuture: 'static, { + + /// Informs the Manager about a new inbound connection that has been + /// established, so it + fn new_inbound( + &self, + addr: Multiaddr, + listener_id: usize, + muxer: C::Output, + ) { + let mut conns = self.connections.lock(); + let new_state = PeerState::Active { + muxer, + connection_id: self.connection_counter.next(), + listener_id: Some(listener_id), + num_substreams: 1, + }; + match conns.insert(addr.clone(), new_state) { + None | Some(PeerState::Errored(_)) => { + // all good, moving on + }, + Some(PeerState::Pending { ref mut notify, .. }) => { + // we need to wake some up, that we have a connection now + for to_notify in notify.drain() { + to_notify.1.notify() + } + } + Some(PeerState::Active { .. }) => { + // FIXME: Now this is confusing. What exactly is supposed to happen here? + trace!("Overwriting active connection {:?}", addr); + } + } + } + fn poll_outbound(&self, addr: Multiaddr, reset: bool) -> Poll<(::OutboundSubstream, usize), IoError> { @@ -478,23 +531,10 @@ where #[inline] fn dial(self, addr: Multiaddr) -> Result { + // If an earlier attempt to dial this multiaddress failed, we clear the error. Otherwise // the returned `Future` will immediately produce the error. - let must_clear = match self.manager.connections.lock().get(&addr) { - Some(&PeerState::Errored(ref err)) => { - trace!( - "Clearing existing connection to {} which errored earlier: {:?}", - addr, - err - ); - true - } - _ => false, - }; - if must_clear { - self.manager.connections.lock().remove(&addr); - } - + self.manager.clear_error(addr.clone()); Ok(ConnectionReuseDial { outbound: None, manager: self.manager, @@ -740,17 +780,11 @@ where // Successfully upgraded a new incoming connection. trace!("New multiplexed connection from {}", client_addr); - let connection_id = self.manager.connection_counter.next(); - self.manager.insert_connection( + self.manager.new_inbound( client_addr.clone(), - PeerState::Active { - muxer, - connection_id, - listener_id: Some(self.listener_id), - num_substreams: 1, - }, - ); + self.listener_id, + muxer); } // Poll all the incoming connections on all the connections we opened. From f1095d8dec8cfceda08be487382f8e2680e4344b Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Wed, 1 Aug 2018 22:24:48 +0200 Subject: [PATCH 13/33] remove redundant connection_id and add docs --- core/src/connection_reuse.rs | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 1f5a14d6f1e..7734c60e0f4 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -106,8 +106,6 @@ where future: Box>, /// All the tasks to notify when `future` resolves. notify: FnvHashMap, - /// Unique identifier for this connection in the `ConnectionReuse`. - connection_id: usize, }, /// An earlier connection attempt errored. @@ -145,7 +143,7 @@ where C: ConnectionUpgrade, C::Output: StreamMuxer + Clone, { - /// notify all `Tasks` in `self.notify_on_new_connec` that a new connection was established + /// Notify all `Tasks` in `self.notify_on_new_connec` that a new connection was established /// consume the tasks in that process fn new_con_notify(&self) { let mut notifiers = self.notify_on_new_connec.lock(); @@ -154,19 +152,9 @@ where } } - /// Insert a new connection, returns Some if an entry was present, notify listeners - pub fn insert_connection( - &self, - addr: Multiaddr, - state: PeerState, - ) -> Option> { - let r = self.connections.lock().insert(addr, state); - self.new_con_notify(); - r - } - - /// Removes one substream from an active connection. Closes the connection if necessary. - pub fn remove_substream(&self, connec_id: usize, addr: &Multiaddr) { + /// Removes one substream from an active connection. + /// Closes the connection if no substream is left. + fn remove_substream(&self, connec_id: usize, addr: &Multiaddr) { self.connections.lock().retain(|_, connec| { if let PeerState::Active { connection_id, @@ -271,7 +259,6 @@ where trace!("Opened new connection to {:?}", addr); let future = future.and_then(|(out, addr)| addr.map(move |a| (out, a))); let future = Box::new(future); - let connection_id = self.connection_counter.next(); // make sure we are woken up once this connects let mut notify : FnvHashMap = Default::default(); @@ -279,8 +266,7 @@ where PeerState::Pending { future, - notify, - connection_id, + notify } } Err(_) => { @@ -322,8 +308,7 @@ where } PeerState::Pending { ref mut future, - ref mut notify, - connection_id, + ref mut notify } => { // was pending, let's check if that has changed match future.poll() { @@ -350,6 +335,7 @@ where task.1.notify(); } let first_outbound = muxer.clone().outbound(); + let connection_id = self.connection_counter.next(); // our connection was upgraded, replace it. (PeerState::Active { @@ -357,7 +343,7 @@ where connection_id: connection_id.clone(), num_substreams: 1, listener_id: None - }, Ok(Async::Ready( (first_outbound, *connection_id)))) + }, Ok(Async::Ready( (first_outbound, connection_id)))) } } } @@ -372,8 +358,9 @@ where /// Polls the incoming substreams on all the incoming connections that match the `listener`. /// - /// Returns `Ready(None)` if no connection is matching the `listener`. Returns `NotReady` if - /// one or more connections are matching the `listener` but they are not ready. + /// Returns `Ready>` for the first matching connection. + /// Return `Ready(None)` if no connection is matching the `listener`. Returns `NotReady` if + /// one or more connections are matching the `listener` but none is ready yet. fn poll_incoming( &self, listener: Option, From 4bfd01beb94a71417a422a3b119e5e07b46424c8 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Wed, 1 Aug 2018 23:49:02 +0200 Subject: [PATCH 14/33] Clean up substream tracking. - remove connection_id and its counter - add unique 'stream_id' and a counter - track specific stream_ids rathern than total - remove now unnecessary wrapper objects --- core/src/connection_reuse.rs | 131 ++++++++++++++--------------------- 1 file changed, 53 insertions(+), 78 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 7734c60e0f4..67108a102cb 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -82,6 +82,8 @@ impl Counter { } } +type StreamId = usize; + enum PeerState where M: StreamMuxer + Clone, @@ -91,9 +93,9 @@ where /// The muxer to open new substreams. muxer: M, /// Unique identifier for this connection in the `ConnectionReuse`. - connection_id: usize, + // connection_id: usize, /// Number of open substreams. - num_substreams: usize, + substreams: Vec, /// Id of the listener that created this connection, or `None` if it was opened by a /// dialer. listener_id: Option, @@ -130,8 +132,8 @@ where /// Tasks to notify when one or more new elements were added to `connections`. notify_on_new_connec: Mutex>, - /// Counter giving us the next connection_id - connection_counter: Counter, + /// Counter giving us the next stream_id + streams_counter: Counter, /// Counter giving us the next listener_id listener_counter: Counter, @@ -152,29 +154,22 @@ where } } - /// Removes one substream from an active connection. - /// Closes the connection if no substream is left. - fn remove_substream(&self, connec_id: usize, addr: &Multiaddr) { - self.connections.lock().retain(|_, connec| { - if let PeerState::Active { - connection_id, - ref mut num_substreams, - .. - } = connec - { - if *connection_id == connec_id { - *num_substreams -= 1; - if *num_substreams == 0 { - trace!( - "All substreams to {} closed ; closing main connection", - addr - ); - return false; - } - } - } - true - }); + /// Removes one substream from an active connection. Drops the connection if no substream + /// is left. Returns `true` if the connection has been dropped. + fn remove_substream(&self, addr: &Multiaddr, stream_id: &StreamId) -> bool { + let mut conns = self.connections.lock(); + let mut drop = false; + + if let Some(PeerState::Active { ref mut substreams, .. }) = conns.get_mut(addr) { + substreams.retain(|e| e != stream_id); + drop = substreams.is_empty(); + } + + if drop { + conns.remove(addr); + } + + drop } /// Clear the cached connection if the entry contains an error. Returns whether an error was @@ -219,9 +214,8 @@ where let mut conns = self.connections.lock(); let new_state = PeerState::Active { muxer, - connection_id: self.connection_counter.next(), listener_id: Some(listener_id), - num_substreams: 1, + substreams: vec![], }; match conns.insert(addr.clone(), new_state) { None | Some(PeerState::Errored(_)) => { @@ -242,7 +236,7 @@ where fn poll_outbound(&self, addr: Multiaddr, - reset: bool) -> Poll<(::OutboundSubstream, usize), IoError> { + reset: bool) -> Poll<(::OutboundSubstream, StreamId), IoError> { let mut conns = self.connections.lock(); @@ -292,8 +286,7 @@ where } PeerState::Active { muxer, - connection_id, - ref mut num_substreams, + ref mut substreams, .. } => { // perfect, let's reuse, update the stream and @@ -302,9 +295,10 @@ where "Using existing connection to {} to open outbound substream", addr ); + let stream_id = self.streams_counter.next(); + substreams.push(stream_id); // mutating the param in place, nothing to be done - *num_substreams += 1; - return Ok(Async::Ready( (muxer.clone().outbound(), connection_id.clone()))); + return Ok(Async::Ready((muxer.clone().outbound(), stream_id))); } PeerState::Pending { ref mut future, @@ -335,15 +329,14 @@ where task.1.notify(); } let first_outbound = muxer.clone().outbound(); - let connection_id = self.connection_counter.next(); + let stream_id = self.streams_counter.next(); // our connection was upgraded, replace it. (PeerState::Active { muxer, - connection_id: connection_id.clone(), - num_substreams: 1, + substreams: vec![stream_id], listener_id: None - }, Ok(Async::Ready( (first_outbound, connection_id)))) + }, Ok(Async::Ready((first_outbound, stream_id)))) } } } @@ -364,7 +357,7 @@ where fn poll_incoming( &self, listener: Option, - ) -> Poll::Substream, usize, Multiaddr)>, IoError> { + ) -> Poll::Substream, StreamId, Multiaddr)>, IoError> { // Keys of the elements in `connections` to remove afterwards. let mut to_remove = Vec::new(); // Substream to return, if any found. @@ -376,8 +369,7 @@ where let res = { if let PeerState::Active { ref mut muxer, - ref mut num_substreams, - connection_id, + ref mut substreams, listener_id, } = state { @@ -389,8 +381,9 @@ where match muxer.clone().inbound().poll() { Ok(Async::Ready(Some(inner))) => { trace!("New incoming substream from {}", addr); - *num_substreams += 1; - Ok((inner, connection_id.clone(), addr.clone())) + let stream_id = self.streams_counter.next(); + substreams.push(stream_id); + Ok((inner, stream_id, addr.clone())) } Ok(Async::Ready(None)) => { // The muxer isn't capable of opening any inbound stream anymore, so @@ -457,7 +450,7 @@ where transport: node, connections: Default::default(), notify_on_new_connec: Default::default(), - connection_counter: Default::default(), + streams_counter: Default::default(), listener_counter: Default::default(), }), } @@ -578,7 +571,7 @@ where /// The future that will construct the substream, the connection id the muxer comes from, and /// the `Future` of the client's multiaddr. /// If `None`, we need to grab a new outbound substream from the muxer. - outbound: Option>, + outbound: Option<(::OutboundSubstream, StreamId)>, // ConnectionsManager between the whole connection reuse mechanism. manager: Arc>, @@ -587,20 +580,6 @@ where addr: Multiaddr, } -struct ConnectionReuseDialOut -where - T: Transport, - C: ConnectionUpgrade, - C::Output: StreamMuxer + Clone, -{ - /// The pending outbound substream. - stream: ::OutboundSubstream, - /// Id of the connection that was used to create the substream. - connection_id: usize, - /// Address of the remote. - client_addr: Multiaddr, -} - impl Future for ConnectionReuseDial where T: Transport + Clone, @@ -621,19 +600,19 @@ where fn poll(&mut self) -> Poll { loop { let should_kill_existing_muxer = match self.outbound.take() { - Some(mut outbound) => { - match outbound.stream.poll() { + Some((mut stream, stream_id)) => { + match stream.poll() { Ok(Async::Ready(Some(inner))) => { trace!("Opened new outgoing substream to {}", self.addr); let manager = self.manager.clone(); return Ok(Async::Ready(( ConnectionReuseSubstream { - connection_id: outbound.connection_id, inner, manager, - addr: outbound.client_addr.clone(), + stream_id: stream_id, + addr: self.addr.clone(), }, - future::ok(outbound.client_addr), + future::ok(self.addr.clone()), ))); } Ok(Async::NotReady) => return Ok(Async::NotReady), @@ -659,12 +638,8 @@ where }; match self.manager.poll_outbound(self.addr.clone(), should_kill_existing_muxer) { - Ok(Async::Ready((stream, connection_id))) => { - self.outbound = Some(ConnectionReuseDialOut { - stream: stream.clone(), - connection_id, - client_addr: self.addr.clone(), - }) + Ok(Async::Ready(val)) => { + self.outbound = Some(val); } Ok(Async::NotReady) => { return Ok(Async::NotReady) @@ -685,7 +660,7 @@ where { fn drop(&mut self) { if let Some(outbound) = self.outbound.take() { - self.manager.remove_substream(outbound.connection_id, &outbound.client_addr); + self.manager.remove_substream(&self.addr, &outbound.1); } } } @@ -784,12 +759,12 @@ where Ok(Async::NotReady) } } - Ok(Async::Ready(Some((inner, connection_id, addr)))) => { + Ok(Async::Ready(Some((inner, stream_id, addr)))) => { Ok(Async::Ready(Some(future::ok(( ConnectionReuseSubstream { inner: inner.clone(), manager: self.manager.clone(), - connection_id: connection_id.clone(), + stream_id, addr: addr.clone(), }, future::ok(addr.clone()), @@ -838,11 +813,11 @@ where #[inline] fn poll(&mut self) -> Poll { match self.manager.poll_incoming(None) { - Ok(Async::Ready(Some((inner, connection_id, addr)))) => Ok(Async::Ready(future::ok(( + Ok(Async::Ready(Some((inner, stream_id, addr)))) => Ok(Async::Ready(future::ok(( ConnectionReuseSubstream { inner, manager: self.manager.clone(), - connection_id, + stream_id, addr: addr.clone(), }, future::ok(addr), @@ -869,9 +844,9 @@ where inner: ::Substream, manager: Arc>, /// Id this connection was created from. - connection_id: usize, - /// Address of the remote. addr: Multiaddr, + /// The Id of this particular stream + stream_id: StreamId } impl Deref for ConnectionReuseSubstream @@ -956,6 +931,6 @@ where C::Output: StreamMuxer + Clone, { fn drop(&mut self) { - self.manager.remove_substream(self.connection_id, &self.addr); + self.manager.remove_substream(&self.addr, &self.stream_id); } } From 8cf6b6d76c430f642664124b9c31dce9b530a656 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 2 Aug 2018 00:01:59 +0200 Subject: [PATCH 15/33] improved and internalized notification system --- core/src/connection_reuse.rs | 46 +++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 67108a102cb..9caa43b4fd0 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -107,7 +107,7 @@ where /// Future that produces the muxer. future: Box>, /// All the tasks to notify when `future` resolves. - notify: FnvHashMap, + notify: Vec<(usize, task::Task)>, }, /// An earlier connection attempt errored. @@ -130,7 +130,7 @@ where connections: Mutex>>, /// Tasks to notify when one or more new elements were added to `connections`. - notify_on_new_connec: Mutex>, + notify_on_new_connec: Mutex>, /// Counter giving us the next stream_id streams_counter: Counter, @@ -149,8 +149,16 @@ where /// consume the tasks in that process fn new_con_notify(&self) { let mut notifiers = self.notify_on_new_connec.lock(); - for to_notify in notifiers.drain() { - to_notify.1.notify(); + for (_, task) in notifiers.drain(..) { + task.notify(); + } + } + + /// Register `task` under id if not yet registered + fn register_notifier(&self, id: usize, task: task::Task) { + let mut notifiers = self.notify_on_new_connec.lock(); + if notifiers.iter().all(|(t, _)| { *t != id }) { + notifiers.push((id, task)) } } @@ -223,8 +231,8 @@ where }, Some(PeerState::Pending { ref mut notify, .. }) => { // we need to wake some up, that we have a connection now - for to_notify in notify.drain() { - to_notify.1.notify() + for (_, task) in notify.drain(..) { + task.notify() } } Some(PeerState::Active { .. }) => { @@ -254,13 +262,10 @@ where let future = future.and_then(|(out, addr)| addr.map(move |a| (out, a))); let future = Box::new(future); - // make sure we are woken up once this connects - let mut notify : FnvHashMap = Default::default(); - notify.insert(TASK_ID.with(|&t| t), task::current()); - PeerState::Pending { future, - notify + // make sure we are woken up once this connects + notify: vec![(TASK_ID.with(|&t| t), task::current())] } } Err(_) => { @@ -309,7 +314,10 @@ where Ok(Async::NotReady) => { // no it still isn't ready, keep the current task // informed but return with NotReady - notify.insert(TASK_ID.with(|&t| t), task::current()); + let t_id = TASK_ID.with(|&t| t); + if notify.iter().all(|(id, _)| { *id != t_id }) { + notify.push((t_id, task::current())); + } return Ok(Async::NotReady); } Err(err) => { @@ -325,8 +333,8 @@ where addr, client_addr ); - for task in notify { - task.1.notify(); + for (_, task) in notify.drain(..) { + task.notify(); } let first_outbound = muxer.clone().outbound(); let stream_id = self.streams_counter.next(); @@ -408,7 +416,10 @@ where } else { if listener.is_none() { if let PeerState::Pending { ref mut notify, .. } = state { - notify.insert(TASK_ID.with(|&t| t), task::current()); + let t_id = TASK_ID.with(|&t| t); + if notify.iter().all(|(id, _)| { *id != t_id }) { + notify.push((t_id, task::current())); + } } } continue; @@ -823,10 +834,7 @@ where future::ok(addr), )))), Ok(Async::Ready(None)) | Ok(Async::NotReady) => { - self.manager - .notify_on_new_connec - .lock() - .insert(TASK_ID.with(|&v| v), task::current()); + self.manager.register_notifier(TASK_ID.with(|&v| v), task::current()); Ok(Async::NotReady) } Err(err) => Err(err), From d9157ebb477b4c395e5ff72cea17a43179ddfae6 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 2 Aug 2018 00:13:42 +0200 Subject: [PATCH 16/33] Docs & comments --- core/src/connection_reuse.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 9caa43b4fd0..8700672bf19 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -189,7 +189,7 @@ where if let PeerState::Errored(ref err) = e.get() { trace!("Clearing existing connection to {} which errored earlier: {:?}", e.key(), err); } else { - return false// nothing to do, quit + return false // nothing to do, quit } // clear the error @@ -238,10 +238,14 @@ where Some(PeerState::Active { .. }) => { // FIXME: Now this is confusing. What exactly is supposed to happen here? trace!("Overwriting active connection {:?}", addr); + // do something with open substreams? } } } + /// Polls for the outbound stream of `addr`. Clears the cached value first if `reset` is true. + /// Dials, if no connection is in the internal cache. Returns `Ok(Async::NotReady)` as long as + /// the connection isn't establed and ready yet. fn poll_outbound(&self, addr: Multiaddr, reset: bool) -> Poll<(::OutboundSubstream, StreamId), IoError> { @@ -250,6 +254,7 @@ where if reset { conns.remove(&addr); + // do something with open substreams? } match conns.entry(addr.clone()) { @@ -432,6 +437,7 @@ where for to_remove in to_remove { conn.remove(&to_remove); + // do something with open substreams? } match ret_value { @@ -742,7 +748,7 @@ where // Process the connections being upgraded. loop { let (muxer, client_addr) = match self.current_upgrades.poll() { - Ok(Async::Ready(Some((muxer, client_addr)))) => (muxer, client_addr), + Ok(Async::Ready(Some(val))) => val, Ok(Async::Ready(None)) | Ok(Async::NotReady) => break, Err(err) => { // Insert the rest of the pending upgrades, but not the current one. @@ -834,6 +840,7 @@ where future::ok(addr), )))), Ok(Async::Ready(None)) | Ok(Async::NotReady) => { + // wake us up, when there is a new connection self.manager.register_notifier(TASK_ID.with(|&v| v), task::current()); Ok(Async::NotReady) } From a6153334f5b13cca5d4574a42a183258acff9a4e Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 2 Aug 2018 14:20:12 +0200 Subject: [PATCH 17/33] Remove unnecessary constraints and clones, make ConnectionReuse clonable again --- core/src/connection_reuse.rs | 25 ++++++++----------------- core/tests/multiplex.rs | 2 +- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 8700672bf19..7b9486018bd 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -60,7 +60,8 @@ use std::clone::Clone; /// /// Can be created from an `UpgradedNode` through the `From` trait. /// -/// Implements the `Transport` trait. +/// Implements the `Transport` trait +#[derive(Clone)] pub struct ConnectionReuse where T: Transport, @@ -476,12 +477,10 @@ where impl Transport for ConnectionReuse where - T: Transport + Clone + 'static, + T: Transport + 'static, T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade + Clone + 'static, // TODO: 'static :( C::Output: StreamMuxer + Clone, - ::Substream: Clone, - ::OutboundSubstream: Clone, C::MultiaddrFuture: Future, C::NamesIter: Clone, UpgradedNode: Clone, @@ -550,12 +549,10 @@ where impl MuxedTransport for ConnectionReuse where - T: Transport + Clone + 'static, // TODO: 'static :( + T: Transport + 'static, // TODO: 'static :( T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade + Clone + 'static, // TODO: 'static :( C::Output: StreamMuxer + Clone, - ::Substream: Clone, - ::OutboundSubstream: Clone, C::MultiaddrFuture: Future, C::NamesIter: Clone, UpgradedNode: Clone, @@ -599,11 +596,9 @@ where impl Future for ConnectionReuseDial where - T: Transport + Clone, + T: Transport, C: ConnectionUpgrade + Clone, C::Output: StreamMuxer + Clone + 'static, - ::Substream: Clone, - ::OutboundSubstream: Clone, UpgradedNode: Transport + Clone, as Transport>::Dial: 'static, as Transport>::MultiaddrFuture: 'static, @@ -702,12 +697,10 @@ where impl Stream for ConnectionReuseListener where - T: Transport + Clone, + T: Transport, T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade + Clone, C::Output: StreamMuxer + Clone + 'static, - ::Substream: Clone, - ::OutboundSubstream: Clone, UpgradedNode: Transport + Clone, as Transport>::Dial: 'static, as Transport>::MultiaddrFuture: 'static, @@ -779,7 +772,7 @@ where Ok(Async::Ready(Some((inner, stream_id, addr)))) => { Ok(Async::Ready(Some(future::ok(( ConnectionReuseSubstream { - inner: inner.clone(), + inner, manager: self.manager.clone(), stream_id, addr: addr.clone(), @@ -808,12 +801,10 @@ where impl Future for ConnectionReuseIncoming where - T: Transport + Clone, + T: Transport, T::Output: AsyncRead + AsyncWrite, C: ConnectionUpgrade + Clone, C::Output: StreamMuxer + Clone + 'static, - ::Substream: Clone, - ::OutboundSubstream: Clone, UpgradedNode: Transport + Clone, as Transport>::Dial: 'static, as Transport>::MultiaddrFuture: 'static, diff --git a/core/tests/multiplex.rs b/core/tests/multiplex.rs index 4c0c9210551..39a05ff8f54 100644 --- a/core/tests/multiplex.rs +++ b/core/tests/multiplex.rs @@ -50,7 +50,7 @@ impl Clone for OnlyOnce { ) } } -impl Transport for OnlyOnce { +impl Transport for OnlyOnce { type Output = T::Output; type MultiaddrFuture = T::MultiaddrFuture; type Listener = T::Listener; From b87d589500832341bc019c1473aed291c514583d Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 2 Aug 2018 15:07:15 +0200 Subject: [PATCH 18/33] Fixing tests --- core/src/connection_reuse.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 7b9486018bd..26cc2e2f616 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -387,7 +387,7 @@ where listener_id, } = state { - if *listener_id == listener { + if *listener_id != listener { continue; } found_one = true; @@ -522,7 +522,7 @@ where current_upgrades: FuturesUnordered::new(), }; - Ok((Box::new(listener) as Box<_>, new_addr)) + Ok((Box::new(listener), new_addr)) } #[inline] From 725fc00d2b9a99a2ee7da671f6f65d8370f6abdd Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 2 Aug 2018 16:53:30 +0200 Subject: [PATCH 19/33] extended debugging --- core/Cargo.toml | 1 + core/src/connection_reuse.rs | 10 +++++++++- core/tests/multiplex.rs | 9 ++++++++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/core/Cargo.toml b/core/Cargo.toml index c9391ec190d..029a293cd65 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -29,3 +29,4 @@ tokio = "0.1" tokio-codec = "0.1" tokio-current-thread = "0.1" tokio-timer = "0.2" +env_logger = "0.5.11" \ No newline at end of file diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 26cc2e2f616..64cae4d6756 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -232,6 +232,7 @@ where }, Some(PeerState::Pending { ref mut notify, .. }) => { // we need to wake some up, that we have a connection now + trace!("Found incoming for pending connection to {}", addr); for (_, task) in notify.drain(..) { task.notify() } @@ -508,7 +509,10 @@ where .map(|upgr| { upgr.and_then(|(out, addr)| { trace!("Waiting for remote's address as listener"); - addr.map(move |addr| (out, addr)) + addr.map(move |addr| { + trace!("Address found:{:?}", addr); + (out, addr) + }) }) }) .fuse(); @@ -763,13 +767,17 @@ where match self.manager.poll_incoming(Some(self.listener_id)) { Ok(Async::NotReady) => Ok(Async::NotReady), Ok(Async::Ready(None)) => { + trace!("Ready but empty"); if self.listener.is_done() && self.current_upgrades.is_empty() { + trace!("Ready, empty, we are, too. Closing;"); Ok(Async::Ready(None)) } else { + trace!("Ready and empty, but we're still going strong. We'll wait!"); Ok(Async::NotReady) } } Ok(Async::Ready(Some((inner, stream_id, addr)))) => { + trace!("Stream {:} for {:?} ready.", stream_id, addr); Ok(Async::Ready(Some(future::ok(( ConnectionReuseSubstream { inner, diff --git a/core/tests/multiplex.rs b/core/tests/multiplex.rs index 39a05ff8f54..23b111483ba 100644 --- a/core/tests/multiplex.rs +++ b/core/tests/multiplex.rs @@ -25,6 +25,8 @@ extern crate libp2p_core; extern crate libp2p_tcp_transport; extern crate tokio_current_thread; extern crate tokio_io; +extern crate env_logger; + use bytes::BytesMut; use futures::future::Future; @@ -68,11 +70,14 @@ impl Transport for OnlyOnce { } } + + #[test] fn client_to_server_outbound() { // A client opens a connection to a server, then an outgoing substream, then sends a message // on that substream. + let _ = env_logger::try_init(); let (tx, rx) = transport::connector(); let bg_thread = thread::spawn(move || { @@ -83,7 +88,7 @@ fn client_to_server_outbound() { .unwrap_or_else(|_| panic!()).0 .into_future() .map_err(|(err, _)| err) - .and_then(|(client, _)| client.unwrap()) + .and_then(|(client, _)| { client.expect("Client could not be created") }) .map(|client| client.0) .map(|client| Framed::<_, BytesMut>::new(client)) .and_then(|client| { @@ -119,6 +124,7 @@ fn connection_reused_for_dialing() { // A client dials the same multiaddress twice in a row. We check that it uses two substreams // instead of opening two different connections. + let _ = env_logger::try_init(); let (tx, rx) = transport::connector(); let bg_thread = thread::spawn(move || { @@ -189,6 +195,7 @@ fn use_opened_listen_to_dial() { // substream on that same connection, that the client has to accept. The client then sends a // message on that new substream. + let _ = env_logger::try_init(); let (tx, rx) = transport::connector(); let bg_thread = thread::spawn(move || { From f8fdcfe9c53f6312dd5fcdca75a4d37d06fea86c Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 2 Aug 2018 17:44:58 +0200 Subject: [PATCH 20/33] Docs and Code style --- core/src/connection_reuse.rs | 114 ++++++++++++++++++----------------- 1 file changed, 58 insertions(+), 56 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 64cae4d6756..eb0654b8905 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -46,10 +46,10 @@ use futures::{stream, task, Async, Future, Poll, Stream}; use multiaddr::Multiaddr; use muxing::StreamMuxer; use parking_lot::Mutex; +use std::collections::hash_map::Entry; use std::io::{Error as IoError, ErrorKind as IoErrorKind, Read, Write}; use std::ops::{Deref, DerefMut}; use std::sync::{atomic::AtomicUsize, atomic::Ordering, Arc}; -use std::collections::hash_map::Entry; use tokio_io::{AsyncRead, AsyncWrite}; use transport::{MuxedTransport, Transport, UpgradedNode}; use upgrade::ConnectionUpgrade; @@ -72,17 +72,21 @@ where manager: Arc>, } +/// Keeps an internal counter, for every new number issued +/// will increase its internal state. #[derive(Default)] struct Counter { - inner: AtomicUsize + inner: AtomicUsize, } impl Counter { + /// Returns the next counter, increasing the internal state pub fn next(&self) -> usize { self.inner.fetch_add(1, Ordering::Relaxed) } } +/// Alias for improved readability type StreamId = usize; enum PeerState @@ -93,8 +97,6 @@ where Active { /// The muxer to open new substreams. muxer: M, - /// Unique identifier for this connection in the `ConnectionReuse`. - // connection_id: usize, /// Number of open substreams. substreams: Vec, /// Id of the listener that created this connection, or `None` if it was opened by a @@ -116,6 +118,9 @@ where } /// Struct shared between most of the `ConnectionReuse` infrastructure. +/// Knows about all connections and their current state, allows one to poll for +/// incoming and outbound connections, while managing all state-transitions that +/// might occur automatically. // #[derive(Clone)] struct ConnectionsManager where @@ -158,7 +163,7 @@ where /// Register `task` under id if not yet registered fn register_notifier(&self, id: usize, task: task::Task) { let mut notifiers = self.notify_on_new_connec.lock(); - if notifiers.iter().all(|(t, _)| { *t != id }) { + if notifiers.iter().all(|(t, _)| *t != id) { notifiers.push((id, task)) } } @@ -169,7 +174,10 @@ where let mut conns = self.connections.lock(); let mut drop = false; - if let Some(PeerState::Active { ref mut substreams, .. }) = conns.get_mut(addr) { + if let Some(PeerState::Active { + ref mut substreams, .. + }) = conns.get_mut(addr) + { substreams.retain(|e| e != stream_id); drop = substreams.is_empty(); } @@ -187,12 +195,16 @@ where let mut conns = self.connections.lock(); if let Entry::Occupied(e) = conns.entry(addr) { - if let PeerState::Errored(ref err) = e.get() { - trace!("Clearing existing connection to {} which errored earlier: {:?}", e.key(), err); + if let PeerState::Errored(ref err) = e.get() { + trace!( + "Clearing existing connection to {} which errored earlier: {:?}", + e.key(), + err + ); } else { - return false // nothing to do, quit + return false; // nothing to do, quit } - + // clear the error e.remove(); true @@ -211,25 +223,19 @@ where as Transport>::Dial: 'static, as Transport>::MultiaddrFuture: 'static, { - /// Informs the Manager about a new inbound connection that has been - /// established, so it - fn new_inbound( - &self, - addr: Multiaddr, - listener_id: usize, - muxer: C::Output, - ) { + /// established, so it + fn new_inbound(&self, addr: Multiaddr, listener_id: usize, muxer: C::Output) { let mut conns = self.connections.lock(); let new_state = PeerState::Active { - muxer, - listener_id: Some(listener_id), - substreams: vec![], - }; + muxer, + listener_id: Some(listener_id), + substreams: vec![], + }; match conns.insert(addr.clone(), new_state) { None | Some(PeerState::Errored(_)) => { // all good, moving on - }, + } Some(PeerState::Pending { ref mut notify, .. }) => { // we need to wake some up, that we have a connection now trace!("Found incoming for pending connection to {}", addr); @@ -248,10 +254,11 @@ where /// Polls for the outbound stream of `addr`. Clears the cached value first if `reset` is true. /// Dials, if no connection is in the internal cache. Returns `Ok(Async::NotReady)` as long as /// the connection isn't establed and ready yet. - fn poll_outbound(&self, + fn poll_outbound( + &self, addr: Multiaddr, - reset: bool) -> Poll<(::OutboundSubstream, StreamId), IoError> { - + reset: bool, + ) -> Poll<(::OutboundSubstream, StreamId), IoError> { let mut conns = self.connections.lock(); if reset { @@ -272,7 +279,7 @@ where PeerState::Pending { future, // make sure we are woken up once this connects - notify: vec![(TASK_ID.with(|&t| t), task::current())] + notify: vec![(TASK_ID.with(|&t| t), task::current())], } } Err(_) => { @@ -301,8 +308,8 @@ where ref mut substreams, .. } => { - // perfect, let's reuse, update the stream and - // return the new ReuseDialOut + // perfect, let's reuse, update the stream_id and + // return a new outbound trace!( "Using existing connection to {} to open outbound substream", addr @@ -314,7 +321,7 @@ where } PeerState::Pending { ref mut future, - ref mut notify + ref mut notify, } => { // was pending, let's check if that has changed match future.poll() { @@ -322,7 +329,7 @@ where // no it still isn't ready, keep the current task // informed but return with NotReady let t_id = TASK_ID.with(|&t| t); - if notify.iter().all(|(id, _)| { *id != t_id }) { + if notify.iter().all(|(id, _)| *id != t_id) { notify.push((t_id, task::current())); } return Ok(Async::NotReady); @@ -335,11 +342,7 @@ where (PeerState::Errored(io_err), Err(err)) } Ok(Async::Ready((muxer, client_addr))) => { - trace!( - "Successful new connection to {} ({})", - addr, - client_addr - ); + trace!("Successful new connection to {} ({})", addr, client_addr); for (_, task) in notify.drain(..) { task.notify(); } @@ -347,11 +350,14 @@ where let stream_id = self.streams_counter.next(); // our connection was upgraded, replace it. - (PeerState::Active { - muxer, - substreams: vec![stream_id], - listener_id: None - }, Ok(Async::Ready((first_outbound, stream_id)))) + ( + PeerState::Active { + muxer, + substreams: vec![stream_id], + listener_id: None, + }, + Ok(Async::Ready((first_outbound, stream_id))), + ) } } } @@ -424,7 +430,7 @@ where if listener.is_none() { if let PeerState::Pending { ref mut notify, .. } = state { let t_id = TASK_ID.with(|&t| t); - if notify.iter().all(|(id, _)| { *id != t_id }) { + if notify.iter().all(|(id, _)| *id != t_id) { notify.push((t_id, task::current())); } } @@ -531,7 +537,6 @@ where #[inline] fn dial(self, addr: Multiaddr) -> Result { - // If an earlier attempt to dial this multiaddress failed, we clear the error. Otherwise // the returned `Future` will immediately produce the error. self.manager.clear_error(addr.clone()); @@ -653,16 +658,14 @@ where _ => false, }; - match self.manager.poll_outbound(self.addr.clone(), should_kill_existing_muxer) { + match self.manager + .poll_outbound(self.addr.clone(), should_kill_existing_muxer) + { Ok(Async::Ready(val)) => { self.outbound = Some(val); } - Ok(Async::NotReady) => { - return Ok(Async::NotReady) - } - Err(err) => { - return Err(err) - } + Ok(Async::NotReady) => return Ok(Async::NotReady), + Err(err) => return Err(err), } } } @@ -757,10 +760,8 @@ where // Successfully upgraded a new incoming connection. trace!("New multiplexed connection from {}", client_addr); - self.manager.new_inbound( - client_addr.clone(), - self.listener_id, - muxer); + self.manager + .new_inbound(client_addr.clone(), self.listener_id, muxer); } // Poll all the incoming connections on all the connections we opened. @@ -840,7 +841,8 @@ where )))), Ok(Async::Ready(None)) | Ok(Async::NotReady) => { // wake us up, when there is a new connection - self.manager.register_notifier(TASK_ID.with(|&v| v), task::current()); + self.manager + .register_notifier(TASK_ID.with(|&v| v), task::current()); Ok(Async::NotReady) } Err(err) => Err(err), @@ -860,7 +862,7 @@ where /// Id this connection was created from. addr: Multiaddr, /// The Id of this particular stream - stream_id: StreamId + stream_id: StreamId, } impl Deref for ConnectionReuseSubstream From a8985ea94f755a45abf54234ae804d65e531cd06 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Fri, 3 Aug 2018 11:50:12 +0200 Subject: [PATCH 21/33] Also immediately check whether the incoming Pending connection is ready; more traces for debugging; `iter().any()` is a more effecient approach rather than `iter().all()` --- core/src/connection_reuse.rs | 218 ++++++++++++++++++----------------- 1 file changed, 110 insertions(+), 108 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index eb0654b8905..c4b459a0285 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -155,6 +155,7 @@ where /// consume the tasks in that process fn new_con_notify(&self) { let mut notifiers = self.notify_on_new_connec.lock(); + trace!("Notify {} listeners about a new connection", notifiers.len()); for (_, task) in notifiers.drain(..) { task.notify(); } @@ -163,7 +164,8 @@ where /// Register `task` under id if not yet registered fn register_notifier(&self, id: usize, task: task::Task) { let mut notifiers = self.notify_on_new_connec.lock(); - if notifiers.iter().all(|(t, _)| *t != id) { + if !notifiers.iter().any(|(t, _)| *t == id) { + trace!("Notify registered for task: {}", id); notifiers.push((id, task)) } } @@ -266,108 +268,104 @@ where // do something with open substreams? } - match conns.entry(addr.clone()) { - Entry::Vacant(e) => { - // Empty: dial, keep in mind, which task wanted to be notified, - // then return with `NotReady` - e.insert(match self.transport.clone().dial(addr.clone()) { - Ok(future) => { - trace!("Opened new connection to {:?}", addr); - let future = future.and_then(|(out, addr)| addr.map(move |a| (out, a))); - let future = Box::new(future); - - PeerState::Pending { - future, - // make sure we are woken up once this connects - notify: vec![(TASK_ID.with(|&t| t), task::current())], - } - } - Err(_) => { - trace!( - "Failed to open connection to {:?}, multiaddr not supported", - addr - ); - let err = - IoError::new(IoErrorKind::ConnectionRefused, "multiaddr not supported"); - PeerState::Errored(err) - } - }); - self.new_con_notify(); + let state = conns.entry(addr.clone()).or_insert({ + let state = match self.transport.clone().dial(addr.clone()) { + Ok(future) => { + trace!("Opening new connection to {:?}", addr); + let future = future.and_then(|(out, addr)| addr.map(move |a| (out, a))); + let future = Box::new(future); - Ok(Async::NotReady) + PeerState::Pending { + future, + // make sure we are woken up once this connects + notify: vec![(TASK_ID.with(|&t| t), task::current())], + } + } + Err(_) => { + trace!( + "Failed to open connection to {:?}, multiaddr not supported", + addr + ); + let err = + IoError::new(IoErrorKind::ConnectionRefused, "multiaddr not supported"); + PeerState::Errored(err) + } + }; + self.new_con_notify(); + // Althought we've just started in pending state, there are transports, which are + // immediately ready - namely 'memory' - so we need to poll our future right away + // rather than waiting for another poll on it. + state + }); + + let (replace_with, return_with) = match state { + PeerState::Errored(err) => { + // Error: return it + let io_err = IoError::new(err.kind(), err.to_string()); + return Err(io_err); } - Entry::Occupied(mut e) => { - let (replace_with, return_with) = match e.get_mut() { - PeerState::Errored(err) => { - // Error: return it - let io_err = IoError::new(err.kind(), err.to_string()); - return Err(io_err); + PeerState::Active { + muxer, + ref mut substreams, + .. + } => { + // perfect, let's reuse, update the stream_id and + // return a new outbound + trace!( + "Using existing connection to {} to open outbound substream", + addr + ); + let stream_id = self.streams_counter.next(); + substreams.push(stream_id); + // mutating the param in place, nothing to be done + return Ok(Async::Ready((muxer.clone().outbound(), stream_id))); + } + PeerState::Pending { + ref mut future, + ref mut notify, + } => { + // was pending, let's check if that has changed + match future.poll() { + Ok(Async::NotReady) => { + // no it still isn't ready, keep the current task + // informed but return with NotReady + let t_id = TASK_ID.with(|&t| t); + if !notify.iter().any(|(id, _)| *id == t_id) { + notify.push((t_id, task::current())); + } + return Ok(Async::NotReady); } - PeerState::Active { - muxer, - ref mut substreams, - .. - } => { - // perfect, let's reuse, update the stream_id and - // return a new outbound - trace!( - "Using existing connection to {} to open outbound substream", - addr - ); - let stream_id = self.streams_counter.next(); - substreams.push(stream_id); - // mutating the param in place, nothing to be done - return Ok(Async::Ready((muxer.clone().outbound(), stream_id))); + Err(err) => { + // well, looks like connecting failed, replace the + // entry then return the Error + trace!("Failed new connection to {}: {:?}", addr, err); + let io_err = IoError::new(err.kind(), err.to_string()); + (PeerState::Errored(io_err), Err(err)) } - PeerState::Pending { - ref mut future, - ref mut notify, - } => { - // was pending, let's check if that has changed - match future.poll() { - Ok(Async::NotReady) => { - // no it still isn't ready, keep the current task - // informed but return with NotReady - let t_id = TASK_ID.with(|&t| t); - if notify.iter().all(|(id, _)| *id != t_id) { - notify.push((t_id, task::current())); - } - return Ok(Async::NotReady); - } - Err(err) => { - // well, looks like connecting failed, replace the - // entry then return the Error - trace!("Failed new connection to {}: {:?}", addr, err); - let io_err = IoError::new(err.kind(), err.to_string()); - (PeerState::Errored(io_err), Err(err)) - } - Ok(Async::Ready((muxer, client_addr))) => { - trace!("Successful new connection to {} ({})", addr, client_addr); - for (_, task) in notify.drain(..) { - task.notify(); - } - let first_outbound = muxer.clone().outbound(); - let stream_id = self.streams_counter.next(); - - // our connection was upgraded, replace it. - ( - PeerState::Active { - muxer, - substreams: vec![stream_id], - listener_id: None, - }, - Ok(Async::Ready((first_outbound, stream_id))), - ) - } + Ok(Async::Ready((muxer, client_addr))) => { + trace!("Successful new connection to {} ({})", addr, client_addr); + for (_, task) in notify.drain(..) { + task.notify(); } - } - }; - - e.insert(replace_with); + let first_outbound = muxer.clone().outbound(); + let stream_id = self.streams_counter.next(); - return_with + // our connection was upgraded, replace it. + ( + PeerState::Active { + muxer, + substreams: vec![stream_id], + listener_id: None, + }, + Ok(Async::Ready((first_outbound, stream_id))), + ) + } + } } - } + }; + + *state = replace_with; + return_with } /// Polls the incoming substreams on all the incoming connections that match the `listener`. @@ -453,6 +451,7 @@ where Some(Err(err)) => Err(err), None => { if found_one { + self.register_notifier(TASK_ID.with(|&v| v), task::current()); Ok(Async::NotReady) } else { Ok(Async::Ready(None)) @@ -768,12 +767,13 @@ where match self.manager.poll_incoming(Some(self.listener_id)) { Ok(Async::NotReady) => Ok(Async::NotReady), Ok(Async::Ready(None)) => { - trace!("Ready but empty"); if self.listener.is_done() && self.current_upgrades.is_empty() { - trace!("Ready, empty, we are, too. Closing;"); + trace!("Ready but empty; we are, too; Closing!"); Ok(Async::Ready(None)) } else { - trace!("Ready and empty, but we're still going strong. We'll wait!"); + trace!("Ready but empty; but we're still going strong; We'll wait!"); + self.manager + .register_notifier(TASK_ID.with(|&v| v), task::current()); Ok(Async::NotReady) } } @@ -830,15 +830,17 @@ where #[inline] fn poll(&mut self) -> Poll { match self.manager.poll_incoming(None) { - Ok(Async::Ready(Some((inner, stream_id, addr)))) => Ok(Async::Ready(future::ok(( - ConnectionReuseSubstream { - inner, - manager: self.manager.clone(), - stream_id, - addr: addr.clone(), - }, - future::ok(addr), - )))), + Ok(Async::Ready(Some((inner, stream_id, addr)))) => { + trace!("New Incoming Substream found {:} for {}", stream_id, addr); + Ok(Async::Ready(future::ok(( + ConnectionReuseSubstream { + inner, + manager: self.manager.clone(), + stream_id, + addr: addr.clone(), + }, + future::ok(addr),)))) + }, Ok(Async::Ready(None)) | Ok(Async::NotReady) => { // wake us up, when there is a new connection self.manager From b6d18ba579f892433ea66aaf74f37b8411fd0647 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Fri, 3 Aug 2018 13:02:30 +0200 Subject: [PATCH 22/33] insert_with rather than insert --- core/src/connection_reuse.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index c4b459a0285..dd13396ea73 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -264,11 +264,12 @@ where let mut conns = self.connections.lock(); if reset { + trace!("Resetting connection to {:}", &addr); conns.remove(&addr); // do something with open substreams? } - let state = conns.entry(addr.clone()).or_insert({ + let state = conns.entry(addr.clone()).or_insert_with(|| { let state = match self.transport.clone().dial(addr.clone()) { Ok(future) => { trace!("Opening new connection to {:?}", addr); From a08bd9ae164ea89f855e74942beb3568e466ac27 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Fri, 3 Aug 2018 13:02:54 +0200 Subject: [PATCH 23/33] do not immediately drop the connection, it might still have pending incoming streams --- core/src/connection_reuse.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index dd13396ea73..362311d7955 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -170,25 +170,16 @@ where } } - /// Removes one substream from an active connection. Drops the connection if no substream - /// is left. Returns `true` if the connection has been dropped. - fn remove_substream(&self, addr: &Multiaddr, stream_id: &StreamId) -> bool { + /// Removes one substream from an active connection. + fn remove_substream(&self, addr: &Multiaddr, stream_id: &StreamId) { let mut conns = self.connections.lock(); - let mut drop = false; if let Some(PeerState::Active { ref mut substreams, .. }) = conns.get_mut(addr) { substreams.retain(|e| e != stream_id); - drop = substreams.is_empty(); } - - if drop { - conns.remove(addr); - } - - drop } /// Clear the cached connection if the entry contains an error. Returns whether an error was From 9f504f27a5a0cc7740559eefc53fc45749c6cf53 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Fri, 3 Aug 2018 19:10:42 +0200 Subject: [PATCH 24/33] Massive Rewrite to drop stream_ids, track closed-state via AtomicBool --- core/src/connection_reuse.rs | 327 ++++++++++++++++------------------- 1 file changed, 152 insertions(+), 175 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 362311d7955..28f7c83022c 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -49,11 +49,10 @@ use parking_lot::Mutex; use std::collections::hash_map::Entry; use std::io::{Error as IoError, ErrorKind as IoErrorKind, Read, Write}; use std::ops::{Deref, DerefMut}; -use std::sync::{atomic::AtomicUsize, atomic::Ordering, Arc}; +use std::sync::{atomic::AtomicUsize, atomic::AtomicBool, atomic::Ordering, Arc}; use tokio_io::{AsyncRead, AsyncWrite}; use transport::{MuxedTransport, Transport, UpgradedNode}; use upgrade::ConnectionUpgrade; - use std::clone::Clone; /// Allows reusing the same muxed connection multiple times. @@ -86,9 +85,6 @@ impl Counter { } } -/// Alias for improved readability -type StreamId = usize; - enum PeerState where M: StreamMuxer + Clone, @@ -97,11 +93,11 @@ where Active { /// The muxer to open new substreams. muxer: M, - /// Number of open substreams. - substreams: Vec, /// Id of the listener that created this connection, or `None` if it was opened by a /// dialer. listener_id: Option, + /// whether this connection was closed + closed: Arc }, /// Connection is pending. @@ -138,9 +134,6 @@ where /// Tasks to notify when one or more new elements were added to `connections`. notify_on_new_connec: Mutex>, - /// Counter giving us the next stream_id - streams_counter: Counter, - /// Counter giving us the next listener_id listener_counter: Counter, } @@ -170,15 +163,17 @@ where } } - /// Removes one substream from an active connection. - fn remove_substream(&self, addr: &Multiaddr, stream_id: &StreamId) { + /// Removes the current entry, clears and resets substreams if open. + fn reset(&self, addr: &Multiaddr) { let mut conns = self.connections.lock(); + self.reset_conn(conns.remove(&addr)); + } - if let Some(PeerState::Active { - ref mut substreams, .. - }) = conns.get_mut(addr) - { - substreams.retain(|e| e != stream_id); + /// resets the connections if the given PeerState is active and has + /// substreams + fn reset_conn(&self, state: Option>) { + if let Some(PeerState::Active { closed, .. }) = state { + closed.store(false, Ordering::Relaxed); } } @@ -207,6 +202,41 @@ where } } +fn poll_for_substream(mut outbound: M::OutboundSubstream, addr: &Multiaddr) + -> (Result, IoError>, Option>) +where + M: StreamMuxer + Clone +{ + match outbound.poll() { + Ok(Async::Ready(Some(inner))) => { + trace!("Opened new outgoing substream to {}", addr); + // all good, return the new stream + (Ok(Some(inner)), None) + } + Ok(Async::NotReady) => (Ok(None), None), + Ok(Async::Ready(None)) => { + // The muxer can no longer produce outgoing substreams. + // Let's reopen a connection. + trace!("Closing existing connection to {} ; can't produce outgoing substreams", addr); + + let err = IoError::new(IoErrorKind::ConnectionRefused, "No Streams left"); + let io_err = IoError::new(IoErrorKind::ConnectionRefused, "No Streams left"); + (Err(err), Some(PeerState::Errored(io_err))) + } + Err(err) => { + // If we get an error while opening a substream, we decide to ignore it + // and open a new muxer. + // If opening the muxer produces an error, *then* we will return it. + debug!( + "Error while opening outgoing substream to {}: {:?}", + addr, err + ); + let io_err = IoError::new(err.kind(), err.to_string()); + (Err(io_err), Some(PeerState::Errored(err))) + } + } +} + impl ConnectionsManager where T: Transport, @@ -223,24 +253,18 @@ where let new_state = PeerState::Active { muxer, listener_id: Some(listener_id), - substreams: vec![], + closed: Arc::new(AtomicBool::new(false)) }; - match conns.insert(addr.clone(), new_state) { - None | Some(PeerState::Errored(_)) => { - // all good, moving on - } - Some(PeerState::Pending { ref mut notify, .. }) => { - // we need to wake some up, that we have a connection now - trace!("Found incoming for pending connection to {}", addr); - for (_, task) in notify.drain(..) { - task.notify() - } - } - Some(PeerState::Active { .. }) => { - // FIXME: Now this is confusing. What exactly is supposed to happen here? - trace!("Overwriting active connection {:?}", addr); - // do something with open substreams? + let mut old_state = conns.insert(addr.clone(), new_state); + + if let Some(PeerState::Pending { ref mut notify, .. }) = old_state { + // we need to wake some up, that we have a connection now + trace!("Found incoming for pending connection to {}", addr); + for (_, task) in notify.drain(..) { + task.notify() } + } else { + self.reset_conn(old_state); } } @@ -250,16 +274,9 @@ where fn poll_outbound( &self, addr: Multiaddr, - reset: bool, - ) -> Poll<(::OutboundSubstream, StreamId), IoError> { + ) -> Poll<(::Substream, Arc), IoError> { let mut conns = self.connections.lock(); - if reset { - trace!("Resetting connection to {:}", &addr); - conns.remove(&addr); - // do something with open substreams? - } - let state = conns.entry(addr.clone()).or_insert_with(|| { let state = match self.transport.clone().dial(addr.clone()) { Ok(future) => { @@ -298,19 +315,28 @@ where } PeerState::Active { muxer, - ref mut substreams, + closed, .. } => { - // perfect, let's reuse, update the stream_id and - // return a new outbound - trace!( - "Using existing connection to {} to open outbound substream", - addr - ); - let stream_id = self.streams_counter.next(); - substreams.push(stream_id); - // mutating the param in place, nothing to be done - return Ok(Async::Ready((muxer.clone().outbound(), stream_id))); + if closed.load(Ordering::Relaxed) { + (PeerState::Errored(IoError::new(IoErrorKind::BrokenPipe, "Connection closed")), + Err(IoError::new(IoErrorKind::BrokenPipe, "Connection closed"))) + } else { + // perfect, let's reuse, update the stream_id and + // return a new outbound + trace!( + "Using existing connection to {} to open outbound substream", + addr + ); + match poll_for_substream(muxer.clone().outbound(), &addr) { + (Ok(Some(stream)), _) => { + return Ok(Async::Ready((stream, closed.clone()))); + } + (Err(res), Some(replace)) => (replace, Err(res)), + (Ok(None), _) => return Ok(Async::NotReady), + (Err(res), None ) => return Err(res), + } + } } PeerState::Pending { ref mut future, @@ -339,18 +365,22 @@ where for (_, task) in notify.drain(..) { task.notify(); } - let first_outbound = muxer.clone().outbound(); - let stream_id = self.streams_counter.next(); - - // our connection was upgraded, replace it. - ( - PeerState::Active { - muxer, - substreams: vec![stream_id], - listener_id: None, - }, - Ok(Async::Ready((first_outbound, stream_id))), - ) + match poll_for_substream(muxer.clone().outbound(), &addr) { + (Ok(Some(stream)), _) => { + let closed = Arc::new(AtomicBool::new(false)); + let state = PeerState::Active { + muxer, + closed: closed.clone(), + listener_id: None, + }; + + (state, Ok(Async::Ready((stream, closed)))) + } + (Err(res), Some(replace)) => (replace, Err(res)), + (Ok(None), _) => return Ok(Async::NotReady), + (Err(res), None ) => return Err(res), + + } } } } @@ -368,7 +398,7 @@ where fn poll_incoming( &self, listener: Option, - ) -> Poll::Substream, StreamId, Multiaddr)>, IoError> { + ) -> Poll::Substream, Arc, Multiaddr)>, IoError> { // Keys of the elements in `connections` to remove afterwards. let mut to_remove = Vec::new(); // Substream to return, if any found. @@ -380,21 +410,25 @@ where let res = { if let PeerState::Active { ref mut muxer, - ref mut substreams, listener_id, + closed, } = state { + if closed.load(Ordering::Relaxed) { + to_remove.push(addr.clone()); + continue + } + if *listener_id != listener { continue; } + found_one = true; match muxer.clone().inbound().poll() { - Ok(Async::Ready(Some(inner))) => { + Ok(Async::Ready(Some(stream))) => { trace!("New incoming substream from {}", addr); - let stream_id = self.streams_counter.next(); - substreams.push(stream_id); - Ok((inner, stream_id, addr.clone())) + Ok((stream, closed.clone(), addr.clone())) } Ok(Async::Ready(None)) => { // The muxer isn't capable of opening any inbound stream anymore, so @@ -434,8 +468,7 @@ where } for to_remove in to_remove { - conn.remove(&to_remove); - // do something with open substreams? + self.reset_conn(conn.remove(&to_remove)); } match ret_value { @@ -466,7 +499,6 @@ where transport: node, connections: Default::default(), notify_on_new_connec: Default::default(), - streams_counter: Default::default(), listener_counter: Default::default(), }), } @@ -532,7 +564,6 @@ where // the returned `Future` will immediately produce the error. self.manager.clear_error(addr.clone()); Ok(ConnectionReuseDial { - outbound: None, manager: self.manager, addr, }) @@ -582,11 +613,6 @@ where C: ConnectionUpgrade, C::Output: StreamMuxer + Clone, { - /// The future that will construct the substream, the connection id the muxer comes from, and - /// the `Future` of the client's multiaddr. - /// If `None`, we need to grab a new outbound substream from the muxer. - outbound: Option<(::OutboundSubstream, StreamId)>, - // ConnectionsManager between the whole connection reuse mechanism. manager: Arc>, @@ -610,67 +636,14 @@ where type Error = IoError; fn poll(&mut self) -> Poll { - loop { - let should_kill_existing_muxer = match self.outbound.take() { - Some((mut stream, stream_id)) => { - match stream.poll() { - Ok(Async::Ready(Some(inner))) => { - trace!("Opened new outgoing substream to {}", self.addr); - let manager = self.manager.clone(); - return Ok(Async::Ready(( - ConnectionReuseSubstream { - inner, - manager, - stream_id: stream_id, - addr: self.addr.clone(), - }, - future::ok(self.addr.clone()), - ))); - } - Ok(Async::NotReady) => return Ok(Async::NotReady), - Ok(Async::Ready(None)) => { - // The muxer can no longer produce outgoing substreams. - // Let's reopen a connection. - trace!("Closing existing connection to {} ; can't produce outgoing substreams", self.addr); - true - } - Err(err) => { - // If we get an error while opening a substream, we decide to ignore it - // and open a new muxer. - // If opening the muxer produces an error, *then* we will return it. - debug!( - "Error while opening outgoing substream to {}: {:?}", - self.addr, err - ); - true - } - } - } - _ => false, - }; - - match self.manager - .poll_outbound(self.addr.clone(), should_kill_existing_muxer) - { - Ok(Async::Ready(val)) => { - self.outbound = Some(val); - } - Ok(Async::NotReady) => return Ok(Async::NotReady), - Err(err) => return Err(err), + match self.manager.poll_outbound(self.addr.clone()) { + Ok(Async::Ready((inner, closed))) => { + Ok(Async::Ready( + (ConnectionReuseSubstream { inner, closed}, + future::ok(self.addr.clone())))) } - } - } -} - -impl Drop for ConnectionReuseDial -where - T: Transport, - C: ConnectionUpgrade, - C::Output: StreamMuxer + Clone, -{ - fn drop(&mut self) { - if let Some(outbound) = self.outbound.take() { - self.manager.remove_substream(&self.addr, &outbound.1); + Err(err) => Err(err), + Ok(Async::NotReady) => Ok(Async::NotReady) } } } @@ -769,16 +742,11 @@ where Ok(Async::NotReady) } } - Ok(Async::Ready(Some((inner, stream_id, addr)))) => { - trace!("Stream {:} for {:?} ready.", stream_id, addr); - Ok(Async::Ready(Some(future::ok(( - ConnectionReuseSubstream { - inner, - manager: self.manager.clone(), - stream_id, - addr: addr.clone(), - }, - future::ok(addr.clone()), + Ok(Async::Ready(Some((inner, closed, addr)))) => { + trace!("Stream for {:?} ready.", addr); + Ok(Async::Ready(Some(future::ok( + (ConnectionReuseSubstream {inner, closed}, + future::ok(addr.clone()), ))))) } Err(err) => Ok(Async::Ready(Some(future::err(IoError::new( @@ -822,15 +790,10 @@ where #[inline] fn poll(&mut self) -> Poll { match self.manager.poll_incoming(None) { - Ok(Async::Ready(Some((inner, stream_id, addr)))) => { - trace!("New Incoming Substream found {:} for {}", stream_id, addr); + Ok(Async::Ready(Some((inner, closed, addr)))) => { + trace!("New Incoming Substream for {}", addr); Ok(Async::Ready(future::ok(( - ConnectionReuseSubstream { - inner, - manager: self.manager.clone(), - stream_id, - addr: addr.clone(), - }, + ConnectionReuseSubstream {inner, closed}, future::ok(addr),)))) }, Ok(Async::Ready(None)) | Ok(Async::NotReady) => { @@ -852,11 +815,23 @@ where C::Output: StreamMuxer + Clone, { inner: ::Substream, - manager: Arc>, - /// Id this connection was created from. - addr: Multiaddr, - /// The Id of this particular stream - stream_id: StreamId, + closed: Arc +} + +impl ConnectionReuseSubstream +where + T: Transport, + C: ConnectionUpgrade, + C::Output: StreamMuxer + Clone, +{ + + /// Reset the connection this Substream is muxed out of + /// will result in it being removed from the cache on the + /// next poll + pub fn reset_connection(self) { + self.closed.store(true, Ordering::Relaxed); + } + } impl Deref for ConnectionReuseSubstream @@ -893,7 +868,11 @@ where { #[inline] fn read(&mut self, buf: &mut [u8]) -> Result { - self.inner.read(buf) + if self.closed.load(Ordering::Relaxed) { + return Err(IoError::new(IoErrorKind::BrokenPipe, "Connection has been closed")); + } + + self.inner.read(buf) } } @@ -913,11 +892,17 @@ where { #[inline] fn write(&mut self, buf: &[u8]) -> Result { + if self.closed.load(Ordering::Relaxed) { + return Err(IoError::new(IoErrorKind::BrokenPipe, "Connection has been closed")); + } self.inner.write(buf) } #[inline] fn flush(&mut self) -> Result<(), IoError> { + if self.closed.load(Ordering::Relaxed) { + return Err(IoError::new(IoErrorKind::BrokenPipe, "Connection has been closed")); + } self.inner.flush() } } @@ -930,17 +915,9 @@ where { #[inline] fn shutdown(&mut self) -> Poll<(), IoError> { + if self.closed.load(Ordering::Relaxed) { + return Err(IoError::new(IoErrorKind::BrokenPipe, "Connection has been closed")); + } self.inner.shutdown() } -} - -impl Drop for ConnectionReuseSubstream -where - T: Transport, - C: ConnectionUpgrade, - C::Output: StreamMuxer + Clone, -{ - fn drop(&mut self) { - self.manager.remove_substream(&self.addr, &self.stream_id); - } -} +} \ No newline at end of file From 567b9ed7b5ab486e41304c6e11417da5436df4e9 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 9 Aug 2018 14:18:51 +0200 Subject: [PATCH 25/33] Remove unused method --- core/src/connection_reuse.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 28f7c83022c..55b521f73fa 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -163,12 +163,6 @@ where } } - /// Removes the current entry, clears and resets substreams if open. - fn reset(&self, addr: &Multiaddr) { - let mut conns = self.connections.lock(); - self.reset_conn(conns.remove(&addr)); - } - /// resets the connections if the given PeerState is active and has /// substreams fn reset_conn(&self, state: Option>) { From 44ded6c9f1a0cadedce3b151a220430cd46df63d Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 9 Aug 2018 14:19:26 +0200 Subject: [PATCH 26/33] Reference by ... well... reference --- core/src/connection_reuse.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 55b521f73fa..25e90102471 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -242,7 +242,7 @@ where { /// Informs the Manager about a new inbound connection that has been /// established, so it - fn new_inbound(&self, addr: Multiaddr, listener_id: usize, muxer: C::Output) { + fn new_inbound(&self, addr: &Multiaddr, listener_id: usize, muxer: C::Output) { let mut conns = self.connections.lock(); let new_state = PeerState::Active { muxer, @@ -267,7 +267,7 @@ where /// the connection isn't establed and ready yet. fn poll_outbound( &self, - addr: Multiaddr, + addr: &Multiaddr, ) -> Poll<(::Substream, Arc), IoError> { let mut conns = self.connections.lock(); @@ -630,7 +630,7 @@ where type Error = IoError; fn poll(&mut self) -> Poll { - match self.manager.poll_outbound(self.addr.clone()) { + match self.manager.poll_outbound(&self.addr) { Ok(Async::Ready((inner, closed))) => { Ok(Async::Ready( (ConnectionReuseSubstream { inner, closed}, @@ -718,8 +718,7 @@ where // Successfully upgraded a new incoming connection. trace!("New multiplexed connection from {}", client_addr); - self.manager - .new_inbound(client_addr.clone(), self.listener_id, muxer); + self.manager.new_inbound(&client_addr, self.listener_id, muxer); } // Poll all the incoming connections on all the connections we opened. From a89d5217cad502c183ad15343ca860b0f7422d61 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 9 Aug 2018 14:35:31 +0200 Subject: [PATCH 27/33] refactor to use match to improve readibility --- core/src/connection_reuse.rs | 44 +++++++++++++----------------------- 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 25e90102471..5a4e9507b21 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -53,7 +53,6 @@ use std::sync::{atomic::AtomicUsize, atomic::AtomicBool, atomic::Ordering, Arc}; use tokio_io::{AsyncRead, AsyncWrite}; use transport::{MuxedTransport, Transport, UpgradedNode}; use upgrade::ConnectionUpgrade; -use std::clone::Clone; /// Allows reusing the same muxed connection multiple times. /// @@ -198,7 +197,7 @@ where fn poll_for_substream(mut outbound: M::OutboundSubstream, addr: &Multiaddr) -> (Result, IoError>, Option>) -where +where M: StreamMuxer + Clone { match outbound.poll() { @@ -401,22 +400,13 @@ where let mut conn = self.connections.lock(); for (addr, state) in conn.iter_mut() { - let res = { - if let PeerState::Active { - ref mut muxer, - listener_id, - closed, - } = state - { - if closed.load(Ordering::Relaxed) { - to_remove.push(addr.clone()); - continue - } - - if *listener_id != listener { - continue; - } - + let res = match state { + PeerState::Active { closed, .. } if closed.load(Ordering::Relaxed) => { + to_remove.push(addr.clone()); + continue + } + PeerState::Active { listener_id, .. } if *listener_id != listener => continue, + PeerState::Active { ref mut muxer, closed, .. } => { found_one = true; match muxer.clone().inbound().poll() { @@ -444,17 +434,15 @@ where Err(err) } } - } else { - if listener.is_none() { - if let PeerState::Pending { ref mut notify, .. } = state { - let t_id = TASK_ID.with(|&t| t); - if notify.iter().all(|(id, _)| *id != t_id) { - notify.push((t_id, task::current())); - } - } + } + PeerState::Pending { ref mut notify, .. } if listener.is_none() => { + let t_id = TASK_ID.with(|&t| t); + if notify.iter().all(|(id, _)| *id != t_id) { + notify.push((t_id, task::current())); } - continue; + continue } + _ => continue }; ret_value = Some(res); @@ -865,7 +853,7 @@ where return Err(IoError::new(IoErrorKind::BrokenPipe, "Connection has been closed")); } - self.inner.read(buf) + self.inner.read(buf) } } From f17f6cd1bb418f5d915c6cdfc65598cf84939a0f Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 9 Aug 2018 14:42:58 +0200 Subject: [PATCH 28/33] Simplify by using --- core/src/connection_reuse.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 5a4e9507b21..4f0a10f8d98 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -618,15 +618,8 @@ where type Error = IoError; fn poll(&mut self) -> Poll { - match self.manager.poll_outbound(&self.addr) { - Ok(Async::Ready((inner, closed))) => { - Ok(Async::Ready( - (ConnectionReuseSubstream { inner, closed}, - future::ok(self.addr.clone())))) - } - Err(err) => Err(err), - Ok(Async::NotReady) => Ok(Async::NotReady) - } + let (inner, closed) = try_ready!(self.manager.poll_outbound(&self.addr)); + Ok(Async::Ready((ConnectionReuseSubstream { inner, closed}, future::ok(self.addr.clone())))) } } From 38695c3ae47cafa6efa88264d0c3fa31e4b29253 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 9 Aug 2018 14:54:07 +0200 Subject: [PATCH 29/33] Replace vector of tasks with custom-hasher HashMap of tasks --- core/src/connection_reuse.rs | 63 ++++++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 24 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 4f0a10f8d98..7a1b1ab5f75 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -46,7 +46,7 @@ use futures::{stream, task, Async, Future, Poll, Stream}; use multiaddr::Multiaddr; use muxing::StreamMuxer; use parking_lot::Mutex; -use std::collections::hash_map::Entry; +use std::collections::{HashMap, hash_map::Entry}; use std::io::{Error as IoError, ErrorKind as IoErrorKind, Read, Write}; use std::ops::{Deref, DerefMut}; use std::sync::{atomic::AtomicUsize, atomic::AtomicBool, atomic::Ordering, Arc}; @@ -54,6 +54,29 @@ use tokio_io::{AsyncRead, AsyncWrite}; use transport::{MuxedTransport, Transport, UpgradedNode}; use upgrade::ConnectionUpgrade; +use std::hash::{BuildHasherDefault, Hasher}; + +#[derive(Default)] +struct UsizeHasher { + state: usize +} + +impl Hasher for UsizeHasher { + fn finish(&self) -> u64 { + self.state as u64 + } + fn write(&mut self, _i: &[u8]) { + unreachable!() + } + fn write_usize(&mut self, i: usize) { + self.state = i; + } +} + +type UsizeHasherBuilder = BuildHasherDefault; + + + /// Allows reusing the same muxed connection multiple times. /// /// Can be created from an `UpgradedNode` through the `From` trait. @@ -105,7 +128,7 @@ where /// Future that produces the muxer. future: Box>, /// All the tasks to notify when `future` resolves. - notify: Vec<(usize, task::Task)>, + notify: HashMap, }, /// An earlier connection attempt errored. @@ -131,7 +154,7 @@ where connections: Mutex>>, /// Tasks to notify when one or more new elements were added to `connections`. - notify_on_new_connec: Mutex>, + notify_on_new_connec: Mutex>, /// Counter giving us the next listener_id listener_counter: Counter, @@ -148,7 +171,7 @@ where fn new_con_notify(&self) { let mut notifiers = self.notify_on_new_connec.lock(); trace!("Notify {} listeners about a new connection", notifiers.len()); - for (_, task) in notifiers.drain(..) { + for (_, task) in notifiers.drain() { task.notify(); } } @@ -156,10 +179,7 @@ where /// Register `task` under id if not yet registered fn register_notifier(&self, id: usize, task: task::Task) { let mut notifiers = self.notify_on_new_connec.lock(); - if !notifiers.iter().any(|(t, _)| *t == id) { - trace!("Notify registered for task: {}", id); - notifiers.push((id, task)) - } + notifiers.insert(id, task); } /// resets the connections if the given PeerState is active and has @@ -253,7 +273,7 @@ where if let Some(PeerState::Pending { ref mut notify, .. }) = old_state { // we need to wake some up, that we have a connection now trace!("Found incoming for pending connection to {}", addr); - for (_, task) in notify.drain(..) { + for (_, task) in notify.drain() { task.notify() } } else { @@ -277,11 +297,11 @@ where let future = future.and_then(|(out, addr)| addr.map(move |a| (out, a))); let future = Box::new(future); - PeerState::Pending { - future, - // make sure we are woken up once this connects - notify: vec![(TASK_ID.with(|&t| t), task::current())], - } + let mut notify = HashMap::::default(); + // make sure we are woken up once this connects + notify.insert(TASK_ID.with(|&t| t), task::current()); + + PeerState::Pending { future, notify } } Err(_) => { trace!( @@ -340,10 +360,7 @@ where Ok(Async::NotReady) => { // no it still isn't ready, keep the current task // informed but return with NotReady - let t_id = TASK_ID.with(|&t| t); - if !notify.iter().any(|(id, _)| *id == t_id) { - notify.push((t_id, task::current())); - } + notify.insert(TASK_ID.with(|&t| t), task::current()); return Ok(Async::NotReady); } Err(err) => { @@ -355,7 +372,7 @@ where } Ok(Async::Ready((muxer, client_addr))) => { trace!("Successful new connection to {} ({})", addr, client_addr); - for (_, task) in notify.drain(..) { + for (_, task) in notify.drain() { task.notify(); } match poll_for_substream(muxer.clone().outbound(), &addr) { @@ -436,10 +453,7 @@ where } } PeerState::Pending { ref mut notify, .. } if listener.is_none() => { - let t_id = TASK_ID.with(|&t| t); - if notify.iter().all(|(id, _)| *id != t_id) { - notify.push((t_id, task::current())); - } + notify.insert(TASK_ID.with(|&t| t), task::current()); continue } _ => continue @@ -480,7 +494,8 @@ where manager: Arc::new(ConnectionsManager { transport: node, connections: Default::default(), - notify_on_new_connec: Default::default(), + notify_on_new_connec: Mutex::new( + HashMap::::default()), listener_counter: Default::default(), }), } From 482922012e3d38ba573dcceb52c35ec56a376b14 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 9 Aug 2018 15:04:24 +0200 Subject: [PATCH 30/33] Switch from Relaxed to Release&Acquire-Ordering --- core/src/connection_reuse.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 7a1b1ab5f75..bd4fe8d7812 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -186,7 +186,7 @@ where /// substreams fn reset_conn(&self, state: Option>) { if let Some(PeerState::Active { closed, .. }) = state { - closed.store(false, Ordering::Relaxed); + closed.store(false, Ordering::Release); } } @@ -331,7 +331,7 @@ where closed, .. } => { - if closed.load(Ordering::Relaxed) { + if closed.load(Ordering::Acquire) { (PeerState::Errored(IoError::new(IoErrorKind::BrokenPipe, "Connection closed")), Err(IoError::new(IoErrorKind::BrokenPipe, "Connection closed"))) } else { @@ -418,7 +418,7 @@ where for (addr, state) in conn.iter_mut() { let res = match state { - PeerState::Active { closed, .. } if closed.load(Ordering::Relaxed) => { + PeerState::Active { closed, .. } if closed.load(Ordering::Acquire) => { to_remove.push(addr.clone()); continue } @@ -818,7 +818,7 @@ where /// will result in it being removed from the cache on the /// next poll pub fn reset_connection(self) { - self.closed.store(true, Ordering::Relaxed); + self.closed.store(true, Ordering::Release); } } @@ -857,7 +857,7 @@ where { #[inline] fn read(&mut self, buf: &mut [u8]) -> Result { - if self.closed.load(Ordering::Relaxed) { + if self.closed.load(Ordering::Acquire) { return Err(IoError::new(IoErrorKind::BrokenPipe, "Connection has been closed")); } @@ -881,7 +881,7 @@ where { #[inline] fn write(&mut self, buf: &[u8]) -> Result { - if self.closed.load(Ordering::Relaxed) { + if self.closed.load(Ordering::Acquire) { return Err(IoError::new(IoErrorKind::BrokenPipe, "Connection has been closed")); } self.inner.write(buf) @@ -889,7 +889,7 @@ where #[inline] fn flush(&mut self) -> Result<(), IoError> { - if self.closed.load(Ordering::Relaxed) { + if self.closed.load(Ordering::Acquire) { return Err(IoError::new(IoErrorKind::BrokenPipe, "Connection has been closed")); } self.inner.flush() @@ -904,7 +904,7 @@ where { #[inline] fn shutdown(&mut self) -> Poll<(), IoError> { - if self.closed.load(Ordering::Relaxed) { + if self.closed.load(Ordering::Acquire) { return Err(IoError::new(IoErrorKind::BrokenPipe, "Connection has been closed")); } self.inner.shutdown() From d7636af4967fb5972367ed6dd61ce51b0b06fa59 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 9 Aug 2018 15:10:26 +0200 Subject: [PATCH 31/33] move `reset_con` into PeerState --- core/src/connection_reuse.rs | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index bd4fe8d7812..2b55355762a 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -135,6 +135,19 @@ where Errored(IoError), } +impl PeerState +where + M: StreamMuxer + Clone +{ + /// resets the connections if the given PeerState is active and has + /// substreams. Consume the PeerState while doing so. + fn reset_conn(self) { + if let PeerState::Active { closed, .. } = self { + closed.store(false, Ordering::Release); + } + } +} + /// Struct shared between most of the `ConnectionReuse` infrastructure. /// Knows about all connections and their current state, allows one to poll for /// incoming and outbound connections, while managing all state-transitions that @@ -182,14 +195,6 @@ where notifiers.insert(id, task); } - /// resets the connections if the given PeerState is active and has - /// substreams - fn reset_conn(&self, state: Option>) { - if let Some(PeerState::Active { closed, .. }) = state { - closed.store(false, Ordering::Release); - } - } - /// Clear the cached connection if the entry contains an error. Returns whether an error was /// found and has been removed. fn clear_error(&self, addr: Multiaddr) -> bool { @@ -277,7 +282,7 @@ where task.notify() } } else { - self.reset_conn(old_state); + old_state.map(|s| s.reset_conn()); } } @@ -464,7 +469,7 @@ where } for to_remove in to_remove { - self.reset_conn(conn.remove(&to_remove)); + conn.remove(&to_remove).map(|s| s.reset_conn()); } match ret_value { From 375d9d754869ecfc2990138c923961eb0c94dbd0 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Mon, 13 Aug 2018 12:53:37 +0200 Subject: [PATCH 32/33] addressing grumbles: clean up clear_error, remove comment --- core/src/connection_reuse.rs | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 2b55355762a..4677a086fb0 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -46,7 +46,7 @@ use futures::{stream, task, Async, Future, Poll, Stream}; use multiaddr::Multiaddr; use muxing::StreamMuxer; use parking_lot::Mutex; -use std::collections::{HashMap, hash_map::Entry}; +use std::collections::HashMap; use std::io::{Error as IoError, ErrorKind as IoErrorKind, Read, Write}; use std::ops::{Deref, DerefMut}; use std::sync::{atomic::AtomicUsize, atomic::AtomicBool, atomic::Ordering, Arc}; @@ -152,7 +152,6 @@ where /// Knows about all connections and their current state, allows one to poll for /// incoming and outbound connections, while managing all state-transitions that /// might occur automatically. -// #[derive(Clone)] struct ConnectionsManager where T: Transport, @@ -197,26 +196,22 @@ where /// Clear the cached connection if the entry contains an error. Returns whether an error was /// found and has been removed. - fn clear_error(&self, addr: Multiaddr) -> bool { + fn clear_error(&self, addr: &Multiaddr) -> bool { let mut conns = self.connections.lock(); - if let Entry::Occupied(e) = conns.entry(addr) { - if let PeerState::Errored(ref err) = e.get() { - trace!( - "Clearing existing connection to {} which errored earlier: {:?}", - e.key(), - err - ); - } else { - return false; // nothing to do, quit - } - - // clear the error - e.remove(); - true + if let Some(PeerState::Errored(ref err)) = conns.get(addr) { + trace!( + "Clearing existing connection to {} which errored earlier: {:?}", + addr, + err + ); } else { - false + return false; } + + // only reaching the point if the entry was an error + conns.remove(addr); + return true; } } @@ -564,7 +559,7 @@ where fn dial(self, addr: Multiaddr) -> Result { // If an earlier attempt to dial this multiaddress failed, we clear the error. Otherwise // the returned `Future` will immediately produce the error. - self.manager.clear_error(addr.clone()); + self.manager.clear_error(&addr); Ok(ConnectionReuseDial { manager: self.manager, addr, From ac9eacc05721b1d950c3fd6a5a78de6825182e15 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Mon, 13 Aug 2018 13:57:19 +0200 Subject: [PATCH 33/33] Simplify poll_for_substream, add docs for it --- core/src/connection_reuse.rs | 40 +++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/core/src/connection_reuse.rs b/core/src/connection_reuse.rs index 4677a086fb0..040c46ba9d9 100644 --- a/core/src/connection_reuse.rs +++ b/core/src/connection_reuse.rs @@ -215,26 +215,29 @@ where } } -fn poll_for_substream(mut outbound: M::OutboundSubstream, addr: &Multiaddr) - -> (Result, IoError>, Option>) +/// Polls the given outbound of addr for a new substream. Returns `Ok(Some(stream))` if available, +/// `Ok(None)` if the stream is not ready yet add an `Err` if the stream had failed. +fn poll_for_substream(mut outbound: O, addr: &Multiaddr) + -> Result, IoError> where - M: StreamMuxer + Clone + + S: AsyncRead + AsyncWrite, + O: Future, Error = IoError> { match outbound.poll() { Ok(Async::Ready(Some(inner))) => { trace!("Opened new outgoing substream to {}", addr); // all good, return the new stream - (Ok(Some(inner)), None) + Ok(Some(inner)) } - Ok(Async::NotReady) => (Ok(None), None), + Ok(Async::NotReady) => Ok(None), Ok(Async::Ready(None)) => { // The muxer can no longer produce outgoing substreams. // Let's reopen a connection. trace!("Closing existing connection to {} ; can't produce outgoing substreams", addr); let err = IoError::new(IoErrorKind::ConnectionRefused, "No Streams left"); - let io_err = IoError::new(IoErrorKind::ConnectionRefused, "No Streams left"); - (Err(err), Some(PeerState::Errored(io_err))) + Err(err) } Err(err) => { // If we get an error while opening a substream, we decide to ignore it @@ -244,8 +247,7 @@ where "Error while opening outgoing substream to {}: {:?}", addr, err ); - let io_err = IoError::new(err.kind(), err.to_string()); - (Err(io_err), Some(PeerState::Errored(err))) + Err(err) } } } @@ -342,12 +344,14 @@ where addr ); match poll_for_substream(muxer.clone().outbound(), &addr) { - (Ok(Some(stream)), _) => { + Ok(None) => return Ok(Async::NotReady), + Ok(Some(stream)) => { return Ok(Async::Ready((stream, closed.clone()))); } - (Err(res), Some(replace)) => (replace, Err(res)), - (Ok(None), _) => return Ok(Async::NotReady), - (Err(res), None ) => return Err(res), + Err(err) => { + let io_err = IoError::new(err.kind(), err.to_string()); + (PeerState::Errored(io_err), Err(err)) + } } } } @@ -376,7 +380,8 @@ where task.notify(); } match poll_for_substream(muxer.clone().outbound(), &addr) { - (Ok(Some(stream)), _) => { + Ok(None) => return Ok(Async::NotReady), + Ok(Some(stream)) => { let closed = Arc::new(AtomicBool::new(false)); let state = PeerState::Active { muxer, @@ -386,9 +391,10 @@ where (state, Ok(Async::Ready((stream, closed)))) } - (Err(res), Some(replace)) => (replace, Err(res)), - (Ok(None), _) => return Ok(Async::NotReady), - (Err(res), None ) => return Err(res), + Err(err) => { + let io_err = IoError::new(err.kind(), err.to_string()); + (PeerState::Errored(io_err), Err(err)) + } } }