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

Refine boxing during transport construction. #1794

Merged
merged 5 commits into from
Oct 16, 2020
Merged

Conversation

romanb
Copy link
Contributor

@romanb romanb commented Oct 15, 2020

After #1783 I had to realise while experimenting with the latest rust-libp2p in substrate that there is still a use-case for boxing non-multiplexed transports, mainly the odd case where a transport is used without a Network. Instead of just re-exposing the transport::upgrade::boxed::boxed() constructor directly for these cases, I decided to try some refinements of the API.

In summary:

  • Introduced Authenticated and Multiplexed newtypes in the transport upgrade builder pipeline for a more type-directed API.
  • There is now both the revived Transport::boxed() -> Boxed<Self::Output> and the new Multiplexed::boxed() -> Boxed<(TConnInfo, StreamMuxerBox)>.
  • Transport::timeout() moved to Multiplexed::timeout() to better reflect the common use-case and avoid the pitfall where the timeout is applied too early in the transport builder pipeline. It also makes the Transport trait a bit leaner.
  • The Swarm now expects a Boxed<(PeerId, StreamMuxerBox)> transport. This avoids repeating Send / Sync / 'static constraints required for boxing the transport in the Swarm API (which always boxes the transport) and also avoids common accidental use of double-boxed transports if the transport passed to the Swarm is already boxed (which also happens right now in substrate).
  • The build_* functions of src/lib.rs now return a transport::Boxed<(PeerId, StreamMuxerBox)>. This is just a change of return type from impl Transport to Boxed, the implementation already boxed anyway. This obsoletes The return value of libp2p::build_* cannot be .boxed() #783.

Some more details:

  • Transport::boxed() no longer requires a multiplexed transport, as was the case before [multistream-select] Fix panic with V1Lazy (regression) and more convenient transport boxing. #1783. I.e. Transport::boxed() can be used with any transport and yields a transport::boxed::Boxed for some opaque output Transport::Output.
  • Transport::boxed() now always also boxes the transport errors, leaving only the opaque Transport::Output unboxed. Every use-case I've seen for Transport::boxed() also wanted boxed transport errors.
  • Transport::upgrade() yields a transport::upgrade::Builder as usual.
  • transport::upgrade::Builder::authenticate() yields a transport::upgrade::Authenticated (newtype).
  • transport::upgrade::Authenticated::apply() yields a transport::upgrade::Authenticated (previously this was Builder::apply() -> Builder).
  • transport::upgrade::Authenticated::multiplex() yields a transport::upgrade::Multiplexed transport (newtype).
  • transport::upgrade::Multiplexed::timeout() yields a transport::upgrade::Multiplexed<TransportTimeout<T>>.
  • transport::upgrade::Multiplexed::boxed() yields a transport::boxed::Boxed whose output muxer is a StreamMuxerBox.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

For what my review is worth, this looks good to me. Authenticated and
Multiplexed ease understanding. Using Boxed instead of impl Transport
simplifies a lot of signatures. While I would guess double boxing of transports
does not induce a performance hit (I might be wrong?), I appreciate the
reduction in complexity.

@romanb
Copy link
Contributor Author

romanb commented Oct 15, 2020

While I would guess double boxing of transports does not induce a performance hit (I might be wrong?), I appreciate the reduction in complexity.

I don't think so either, but as you noted, it is easy to avoid unnecessary double-boxing while at the same time removing repeated bounds on the API, so it seemed like a win on two fronts. Given that the Swarm is currently always boxing the transport anyway, it just seems clearer to just require one to be given, instead of boxing (possibly redundantly) whatever transport is given. Thanks for the review 🙏. If there's no further comments by tomorrow I plan to merge and finally prepare some new releases. Please flag any other open PRs that you think should be merged before that.

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