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

feat: remove MPLEX by default #9641

Closed
wants to merge 1 commit into from
Closed

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Feb 9, 2023

Mplex does not implement backpressure, our implementation will randomly reset streams if buffers overflow instead of risking deadlocks.

In the past we had a bug where kubo nodes would prefer mplex over yamux. Turning off mplex make our connections to thoses nodes negociate yamux.

Mplex does not implement backpressure, our implementation will randomly reset streams if buffers overflow instead of risking deadlocks.

In the past we had a bug where kubo nodes would prefer mplex over yamux. Turning off mplex make our connections to thoses nodes negociate yamux.
@Jorropo
Copy link
Contributor Author

Jorropo commented Feb 9, 2023

@aschmahmann
Copy link
Contributor

@ianopolous I recall you recently asked about mplex in jvm-libp2p. Not sure if this will effect you.

@ianopolous
Copy link
Member

Thanks for the pointer @aschmahmann . Yes jvm-libp2p currently only has the mplex muxer and no yamux or quic. So disabling mplex would partition all those nodes from the network.

@Jorropo
Copy link
Contributor Author

Jorropo commented Feb 9, 2023

@ianopolous that a blocker, thx for speaking up, do you have plans to add yamux at some point in the future ? I guess you must be running in the same stream resets issues we were seeing in the past.

@achingbrain
Copy link
Member

achingbrain commented Feb 9, 2023

js-ipfs doesn't ship with yamux by default, though it can be used via custom config. There should probably be a release with it included, if mplex is being removed from kubo.

@Jorropo
Copy link
Contributor Author

Jorropo commented Feb 9, 2023

I'll not do this now, this PR was a beacon for "do people complain if we drop this broken protocol ?" answer is yes 🙏.

We will revisit this in the future (1y ?), please consider adding support for the not broken muxer (yamux) in your implementations. ^^

@Jorropo Jorropo closed this Feb 9, 2023
@Jorropo Jorropo deleted the no-more-mplex branch February 9, 2023 18:34
@ianopolous
Copy link
Member

@Jorropo Yep I'll prioritise adding yamux support now.

@Jorropo
Copy link
Contributor Author

Jorropo commented Feb 9, 2023

@ianopolous I'm not asking to prioritize anything, feel free to do it if you want, but if you don't it's fine, we can keep mplex in Kubo.

The main goals are:

  1. Avoid divergent defaults between Kubo and go-libp2p. (go-libp2p does not have mplex by default anymore)
  2. Better auto negotiation with buggy Kubo nodes that prioritize mplex over yamux.

Neither are urgent or even real problems right now, the point I was trying to make is that if mplex is completely useless might as well remove it as this have slight positive benefits, turns out, it is still usefull so we can keep it.

@ianopolous
Copy link
Member

@Jorropo FYI, we've implemented yamux now.

@lidel
Copy link
Member

lidel commented Feb 14, 2023

Continued in ipfs/js-ipfs#4318 (js-ipfs needs to support both, so when we remove mplex from kubo in the future, it wont break)

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.

5 participants