Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add helpers to create a transport with secio and mplex/yamux #735

Closed
tomaka opened this issue Dec 5, 2018 · 8 comments
Closed

Add helpers to create a transport with secio and mplex/yamux #735

tomaka opened this issue Dec 5, 2018 · 8 comments
Assignees
Labels
difficulty:moderate priority:important The changes needed are critical for libp2p, or are blocking another project
Milestone

Comments

@tomaka
Copy link
Member

tomaka commented Dec 5, 2018

Most people just want secio, mplex and yamux. We should add a helper that easily creates a transport like that.

@tomaka tomaka added priority:important The changes needed are critical for libp2p, or are blocking another project difficulty:moderate labels Dec 5, 2018
@tomaka
Copy link
Member Author

tomaka commented Dec 5, 2018

An important motivation for this change is that we want to reserve the right to break the API of the underlying combinators, and it would be nicer if we could avoid breaking everyone's code in the process.

@dvdplm
Copy link
Contributor

dvdplm commented Dec 5, 2018

What API do you have in mind for this? let t = AllYouEverNeedTransport::new() with a builder to set up options? Or more similar to what we do today, something like

let t = CommonTransport::new()
    .with_upgrade(secio::SecioConfig::new(key))
    .with_upgrade(yamux::Config::default())

@tomaka
Copy link
Member Author

tomaka commented Dec 5, 2018

What API do you have in mind for this?

That's the difficult part of this issue!

Using with_upgrade doesn't work, as secio produces a SecioOutput while yamux expects a simple stream as input.

@tomaka tomaka added this to the publish-v0.1 milestone Dec 7, 2018
@miketang84
Copy link
Contributor

What API do you have in mind for this?

That's the difficult part of this issue!

Using with_upgrade doesn't work, as secio produces a SecioOutput while yamux expects a simple stream as input.

This is ugly and rigid on current API. Very unfriendly!

@dvdplm
Copy link
Contributor

dvdplm commented Dec 12, 2018

@daogangtang as a user, what would the ideal API look like?

@miketang84
Copy link
Contributor

@daogangtang as a user, what would the ideal API look like?

We wish multiple with_upgrade(), one by one, as above.

@tomaka
Copy link
Member Author

tomaka commented Dec 12, 2018

The approach with AllYouEverNeedTransport::new() is the best to me, because:

  • It's the most concise and the most simple. Makes it easy to write examples and tests.
  • I don't think there are that many different schemes that are going to be used in practice. For example, why would you not want to support TCP or WebSockets in your application? Consequently I don't see why someone would use something else than TCP+WebSockets. Same for muxers. Therefore, in practice, very few combinations of protocols will be used in the wild.
  • Right now if we wanted to allow piling up with_upgrade, then we would have to build in mplex and yamux the assumption that they are on top of secio. This is not great. I don't think it is possible to combine with_upgrade and genericity. Combinators (and_then, map, ...) are a bit complicated to use.
  • In the future we probably want to add support for protocols such as QUIC, except that QUIC doesn't need secio/mplex/yamux. This complicates even more the stack of combinators.

More specifically, I would go for something like that:

/// Can add support for new protocols, and maybe break backwards compatibility by removing deprecated protocols.
pub fn build_general_transport(k: SecioKeyPair) -> impl Transport {
     build_tcp_ws_secio_mplex_yamux(k)
}
/// Guaranteed to never change.
pub fn build_tcp_ws_secio_mplex_yamux(k: SecioKeyPair) -> impl Transport { ... }

In other words, we would provide one transport for the "general" case, and one transport for the common situations. If the user needs something more fine-grained, they can always fall back to using combinators.

@tomaka
Copy link
Member Author

tomaka commented Dec 12, 2018

Similarly I would automatically add a timeout to the Transport returned by these helper functions, but not automatically to the one that the user builds manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty:moderate priority:important The changes needed are critical for libp2p, or are blocking another project
Projects
None yet
Development

No branches or pull requests

3 participants