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

[mplex, yamux] Streamline configuration API. #1822

Merged
merged 3 commits into from
Nov 6, 2020

Conversation

romanb
Copy link
Contributor

@romanb romanb commented Nov 4, 2020

This is a proposal for streamlining the configuration APIs of libp2p-mplex and libp2p-yamux, i.e. to close #1531. I made the following choices:

  • For all configuration options that exist for both multiplexers and have the same semantics, use the same names for the
    configuration.
  • Rename yamux::Config to YamuxConfig for consistency with the majority of other protocols, e.g. MplexConfig, PingConfig, KademliaConfig, etc.
  • Completely hide yamux APIs within libp2p-yamux. This is a trade-off. It allows to fully control the libp2p API and streamline it with other muxer APIs, consciously choosing e.g. which configuration options to make configurable in libp2p and which to fix to certain values (e.g. we currently always fix read_after_close which is not actually configurable for libp2p). It also means that new incompatible version bumps of yamux need not necessarily require analogous bumps for libp2p-yamux, as no yamux types are exposed. The cost is naturally some more duplication of configuration options in the API, as well as the need to update libp2p-yamux if yamux introduces new configuration options that libp2p-yamux wants to expose as well. Overall I think fully hiding yamux from the API, i.e. making it an implementation detail, is preferable.

  * For all configuration options that exist for both multiplexers
    and have the same semantics, use the same names for the
    configuration.
  * Rename `Config` to `YamuxConfig` for consistentcy with
    the majority of other protocols, e.g. `MplexConfig`, `PingConfig`,
    `KademliaConfig`, etc.
  * Completely hide `yamux` APIs within `libp2p-yamux`. This allows
    to fully control the libp2p API and streamline it with other
    muxer APIs, consciously choosing e.g. which configuration options
    to make configurable in libp2p and which to fix to certain values.
    It does also not necessarily prescribe new incompatible version bumps of
    yamux for `libp2p-yamux`, as no `yamux` types are exposed. The cost
    is some more duplication of configuration options in the API, as well
    as the need to update `libp2p-yamux` if `yamux` introduces new
    configuration options that `libp2p-yamux` wants to expose as well.
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.

Completely hide yamux APIs within libp2p-yamux.

I am indifferent in regards to this change. It does add another layer of indirection and forces us to keep documentation in sync manually, e.g. for WindowUpdateMode. Either way, given that neither yamux nor libp2p-yamux change at a fast pace, I think this is manageable.

Thanks for doing the cleanup!

@romanb
Copy link
Contributor Author

romanb commented Nov 5, 2020

I am indifferent in regards to this change.

I'd just like to point out that as I see it there is only the choice between either hiding the yamux API to fully control the libp2p-yamux API and achieve better consistency with other libp2p APIs, or to close #1531 as a "won't fix", because it does not seem sensible to adjust yamux APIs so that they conform better to other libp2p muxer APIs, i.e. it is clearly up to libp2p to decide whether it wants to fully encapsulate and control the muxer APIs or whether it accepts exposing potentially very different-looking APIs through direct exposure of underlying libraries. The case in point that makes me think the approach proposed here is worth a try is the fact that there is already one yamux configuration option which libp2p-yamux does not actually allow to configure, namely read_after_close. Whether there will be more such cases remains to be seen, but giving the impression on the outside that all of the yamux configuration API is available in libp2p when it is not is at least slightly misleading.

@romanb romanb merged commit 2ba78b4 into libp2p:master Nov 6, 2020
@romanb romanb deleted the yamux-mplex-config-streamline branch November 6, 2020 08:46
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.

Consistency of mplex and yamux settings
2 participants