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

muxers/mplex: Implement AsyncRead and AsyncWrite for Substream #2706

Merged
merged 7 commits into from
Jun 20, 2022

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Jun 15, 2022

Description

This aligns the public API of the libp2p-mplex module with the one
from libp2p-yamux. This change has two benefits:

  1. For standalone users of libp2p-mplex, the substreams itself are
    now useful, similar to libp2p-yamux and don't necessarily need to
    be polled via the StreamMuxer. The StreamMuxer only forwards to
    the Async{Read,Write} implementations.

  2. This will reduce the diff of core/muxing: Redesign StreamMuxer trait #2648 because we can chunk the one
    giant commit into smaller atomic ones.

Whilst keeping master functional, the state of StreamMuxer is now
a bit awkward until we merge #2648. However, given we are already
doing things this way for libp2p-yamux it shouldn't be an issue?

Links to any relevant issues

Will reduce diff in #2648.

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

This aligns the public API of the `libp2p-mplex` module with the one
from `libp2p-yamux`. This change has two benefits:

1. For standalone users of `libp2p-mplex`, the substreams itself are
now useful, similar to `libp2p-yamux` and don't necessarily need to
be polled via the `StreamMuxer`. The `StreamMuxer` only forwards to
the `Async{Read,Write}` implementations.

2. This will reduce the diff of #2648 because we can chunk the one
giant commit into smaller atomic ones.
@thomaseizinger
Copy link
Contributor Author

I've benchmarked this branch separately and cannot reliably find any regressions. The benchmarks do sometimes report changes in the 5% range but those are not stable so I think this change is harmless for performance.

@mxinden
Copy link
Member

mxinden commented Jun 19, 2022

Whilst keeping master functional, the state of StreamMuxer is now
a bit awkward until we merge #2648. However, given we are already
doing things this way for libp2p-yamux it shouldn't be an issue?

Agreed. I like that this is making it more consistent with libp2p-yamux.

Thanks isolating this change!

@@ -3,7 +3,7 @@ name = "libp2p-mplex"
edition = "2021"
rust-version = "1.56.1"
description = "Mplex multiplexing protocol for libp2p"
version = "0.33.0"
version = "0.34.0"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
version = "0.34.0"
version = "0.33.1"

Is this a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because Substream is a public type and has type parameter now!

Copy link
Member

Choose a reason for hiding this comment

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

Oh, good point. Thanks!

muxers/mplex/CHANGELOG.md Outdated Show resolved Hide resolved
thomaseizinger and others added 3 commits June 19, 2022 18:30
v0.46.0 is not yet released, thus no need to bump to v0.47.0.
muxers/mplex/CHANGELOG.md Outdated Show resolved Hide resolved
@mxinden mxinden merged commit ea487ae into master Jun 20, 2022
@thomaseizinger thomaseizinger deleted the refactor/mplex-async-read-write-stream branch June 20, 2022 04:42
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