Implement ConnectionReuse correctly #111
Conversation
tomaka
force-pushed
the
connection-reuse-correct
branch
from
5458495
to
6e2d491
Compare
tomaka
force-pushed
the
connection-reuse-correct
branch
from
6e2d491
to
b5aa092
Compare
tomaka
force-pushed
the
connection-reuse-correct
branch
from
3f18f87
to
c374bc5
Compare
| to_signal: Vec<task::Task>, | ||
| struct Shared<M> where M: StreamMuxer { | ||
| // List of active muxers. | ||
| active_connections: FnvHashMap<Multiaddr, M>, |
There was a problem hiding this comment.
fnv is faster for small keys. do you know what is the threshold? when the key is considered to be 'short' and when it is not 'short' any more? I'm just curious
There was a problem hiding this comment.
I found this (old) link: http://cglab.ca/~abeinges/blah/hash-rs/
Most the time the multiaddr will be something like /ip4/a.b.c.d/tcp/e which is 8 bytes. Fnv may indeed be more appropriate here.
| @@ -127,31 +142,36 @@ where | |||
| } | |||
|
|
|||
| fn dial(self, addr: Multiaddr) -> Result<Self::Dial, (Self, Multiaddr)> { | |||
There was a problem hiding this comment.
what does Err variant represent? and in what cases is it returned?
There was a problem hiding this comment.
Happens if the multiaddress is invalid or not supported. This should be documented in the Transport trait if it isn't the case.
| { | ||
| type Item = (O, Multiaddr); | ||
| type Item = (future::FutureResult<M::Substream, IoError>, Multiaddr); |
There was a problem hiding this comment.
what does item represent and how should each case be handled?
| @@ -233,7 +235,7 @@ impl<T, C, H, If, F> Future for SwarmFuture<T, C, H, F> | |||
| self.listeners.push(listener); | |||
| }, | |||
| Ok(Async::Ready(None)) => {}, | |||
| Err(err) => return Err(err), | |||
| Err(_err) => {}, // Ignoring errors | |||
There was a problem hiding this comment.
why are errors being ignored here?
There was a problem hiding this comment.
An error happens if the remote sends us bad data, so we just drop the connection. This probably needs a comment and a log entry.
libp2p-swarm/src/connection_reuse.rs
Outdated
| // We extract everything at the start, then insert back the elements that we still want at | ||
| // the next iteration. | ||
| for n in (0 .. self.connections.len()).rev() { | ||
| let (muxer, mut next_incoming, client_addr) = self.connections.swap_remove(n); |
There was a problem hiding this comment.
formatting is inconsistent here (spaces)
tomaka
force-pushed
the
connection-reuse-correct
branch
from
4105cfa
to
e5de1fb
Compare
libp2p-swarm/src/connection_reuse.rs
Outdated
|
|
||
| // Check whether any upgrade (to a muxer) on an incoming connection is ready. | ||
| for n in (0 .. self.current_upgrades.len()).rev() { | ||
| let (mut current_upgrade, client_addr) = self.current_upgrades.swap_remove(n); | ||
| match current_upgrade.poll() { |
There was a problem hiding this comment.
Mixed tabs and spaces
libp2p-swarm/src/connection_reuse.rs
Outdated
| Ok(Async::Ready(None)) | Err(_) => { | ||
| // The sender and receiver are both in the same struct, therefore the link can | ||
| // never break. | ||
| unreachable!() |
There was a problem hiding this comment.
Move this comment into a string inside unreachable!
libp2p-swarm/src/connection_reuse.rs
Outdated
|
|
||
| for (offset, future) in lock.incoming.iter_mut().enumerate() { | ||
| // Check whether any incoming substream is ready. | ||
| for n in (0 .. lock.next_incoming.len()).rev() { |
There was a problem hiding this comment.
Mixed tabs and spaces
libp2p-swarm/src/transport.rs
Outdated
| @@ -874,26 +891,31 @@ where | |||
| /// | |||
| /// This function returns the next incoming substream. You are strongly encouraged to call it | |||
| /// if you have a muxed transport. | |||
There was a problem hiding this comment.
Maybe word this "Muxed transports may block processing incoming connections if this function is not called" or similar, "strongly encouraged" is unhelpful and obscure.
| upgrade.upgrade(connection, upgrade_id, Endpoint::Dialer, &addr) | ||
| .map(|u| (u, addr)) | ||
| }); | ||
| .map(|(future, addr)| { |
There was a problem hiding this comment.
Mixed tabs and spaces
tomaka
force-pushed
the
connection-reuse-correct
branch
from
e8b8dbd
to
339c639
Compare
|
Closing in favour of #117 |
* Implement ConnectionReuse correctly * Add some tests and fixes * Remove useless boolean in active_connections * Correctly run tests * Optimize the processing * Next incoming is now in two steps * Remove log * Fix dialing a node even if we already have a connection * Add a proper PeerId to Peerstore * Turn identify into a transport layer * Expose the dialed multiaddress * Add identified nodes to the peerstore * Allow configuring the TTL of the addresses * Split identify in two modules * Some comments and tweaks * Run rustfmt * Add test and bugfix * Fix wrong address reported when dialing * Fix websocket browser code * Ignore errors in the swarm * Fix multiplex test * Fix some style concerns * Fix concerns
The current implementation is a dummy. This PR replaces it with a better one.