Skip to content

Conversation

@tomaka
Copy link
Member

@tomaka tomaka commented Dec 20, 2017

Right now the way the Transport trait works is that it produces a stream of sockets over which modifiers are applied in order to negotiate a protocol. Unfortunately an overlook of this design is that elements of a stream are polled serially, and not in parallel. The consequence is that right now a server cannot accept any further client while a client is already being negotiated.

This PR fixes that by changing the item yielded by the Listener stream from a Result<impl AsyncRead + AsyncWrite, IoError> to a impl Future<Item = impl AsyncRead + AsyncWrite, Error = IoError>.

Two notes:

  • This doesn't change the various dial_and_listen methods because a general improvement of multiplexing is needed anyway.
  • The changes to ConnectionReuse introduced by this PR are very unsatisfactory, for the same reason.

@tomaka
Copy link
Member Author

tomaka commented Dec 28, 2017

For the record this doesn't allow the echo example to handle multiple simultaneously clients, because the code to do so is non-trivial (requires creating a custom implementation of Stream or Future).

@tomaka tomaka force-pushed the fix-multi-connec branch 3 times, most recently from 75f3118 to d545f76 Compare December 29, 2017 21:17
Copy link
Contributor

@folsen folsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

.and_then(move |(msg, rest)| {
if let Some(msg) = msg {
// One message has been received. We send it back to the client.
println!("Received a message from {}: {:?}\n => Sending back \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just for debugging now or should we bring in a logging library like the parity client, allowing users to flip some flags for extra debug data? Just a general question that I thought of when I saw this, doesn't have to be addressed in this PR obviously.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example is here to quickly demonstrate how to use libp2p, so I don't think a logging library is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No definitely not, I was just talking generally, might be nice to be able to flip on something like trace messages, we can move that conversation to Riot :P

@folsen folsen merged commit 642d18e into libp2p:master Jan 3, 2018
@tomaka tomaka deleted the fix-multi-connec branch January 3, 2018 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants