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

turn mplex off by default #9958

Closed
3 of 6 tasks
Tracked by #553
marten-seemann opened this issue Jun 15, 2023 · 11 comments · Fixed by #10051 or #10070
Closed
3 of 6 tasks
Tracked by #553

turn mplex off by default #9958

marten-seemann opened this issue Jun 15, 2023 · 11 comments · Fixed by #10051 or #10070
Assignees
Labels
kind/enhancement A net-new feature or improvement to an existing feature

Comments

@marten-seemann
Copy link
Member

marten-seemann commented Jun 15, 2023

Checklist

  • My issue is specific & actionable.
  • I am not suggesting a protocol enhancement.
  • I have searched on the issue tracker for my issue.

Description

libp2p has deprecated mplex for a long time. The implementation is not being maintained any more, and we're planning to drop it from our transport integration test suite due to its flakiness (read: bugs that we don't feel like debugging).

js-libp2p has had yamux support for a while now, as does rust-libp2p. Would it be a good time to say goodbye to our beloved mplex?

Done criteria

  • mplex is not enabled by default
  • Changelog specifies that mplex will be deprecated in the next Kubo release, how to reenable it, and points them that we'd like their feedback if they still require mplex. We should point to fully remove mplex #10069
  • (nice to have) Log a warning if someone has mplex enabled.
@marten-seemann marten-seemann added the kind/enhancement A net-new feature or improvement to an existing feature label Jun 15, 2023
@Jorropo
Copy link
Contributor

Jorropo commented Jun 15, 2023

👍 js-ipfs never got the yamux update, you have to add it in manually.

Given js-ipfs isn't maintained anymore we should remove thoses integration tests, ideally have new ones with helia. I did not had the courage to touch some JS when I tried in #9641.

@marten-seemann
Copy link
Member Author

@ipfs/kubo-maintainers Is this in scope for the next release? Any reason to not do this rather sooner than later?

@BigLep
Copy link
Contributor

BigLep commented Jun 29, 2023

Thanks for surfacing. We haven't set the scope for the next release. We will do so next week. I've put this down for consideration.

@BigLep
Copy link
Contributor

BigLep commented Jul 6, 2023

@marten-seemann : this won't make it in for 0.22 since we need to clean up the interoperability story as well: #10013

I have this slotted for the next release (0.23) instead.

@aschmahmann aschmahmann changed the title deprecate mplex remove mplex support Jul 17, 2023
@aschmahmann
Copy link
Contributor

note: updated the title to note that this about removing mplex support given that mplex has been the non-recommended transport for quite a while now.

@aschmahmann
Copy link
Contributor

@achingbrain does js-ipfs support yamux or just helia? Trying to understand the scope of things that will break (and their update stories) if/when mplex support is removed.

Jorropo added a commit to Jorropo/go-ipfs that referenced this issue Aug 4, 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.

Closes ipfs#9958
Jorropo added a commit to Jorropo/go-ipfs that referenced this issue Aug 4, 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.

Closes ipfs#9958
Jorropo added a commit to Jorropo/go-ipfs that referenced this issue Aug 4, 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.

Closes ipfs#9958
@BigLep BigLep moved this to 🔎 In Review in IPFS Shipyard Team Aug 4, 2023
hacdias pushed a commit to Jorropo/go-ipfs that referenced this issue Aug 14, 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.

Closes ipfs#9958
Jorropo added a commit to Jorropo/go-ipfs that referenced this issue Aug 15, 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.

Closes ipfs#9958
Jorropo added a commit to Jorropo/go-ipfs that referenced this issue Aug 15, 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.

Closes ipfs#9958
@github-project-automation github-project-automation bot moved this from 🔎 In Review to 🎉 Done in IPFS Shipyard Team Aug 15, 2023
hacdias pushed a commit that referenced this issue Aug 15, 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.

Closes #9958
@Jorropo
Copy link
Contributor

Jorropo commented Aug 15, 2023

I was not happy with the last PR, I wanted to turn it off by default for a release and remove it next release.

This would give a 1 month gap for people to add yamux back to their js-ipfs instance or migrate to helia.

@Jorropo Jorropo reopened this Aug 15, 2023
@Jorropo Jorropo changed the title remove mplex support deprecate mplex Aug 15, 2023
@Jorropo Jorropo changed the title deprecate mplex turnoff mplex by default Aug 15, 2023
@Jorropo Jorropo mentioned this issue Aug 15, 2023
3 tasks
@Jorropo Jorropo changed the title turnoff mplex by default turn mplex off by default Aug 15, 2023
Jorropo added a commit to Jorropo/go-ipfs that referenced this issue Aug 15, 2023
Jorropo added a commit that referenced this issue Aug 15, 2023
This is a partial revert of 7220409.

Closes #9958
@BigLep BigLep reopened this Aug 22, 2023
@BigLep
Copy link
Contributor

BigLep commented Aug 22, 2023

I'm reopening this issue. This issue is done when we have code in place to disable mplex by default. This is happening in #10095 . Fully removing mplex is tracked in #10069

@BigLep BigLep moved this from 🎉 Done to 🔎 In Review in IPFS Shipyard Team Aug 22, 2023
@BigLep
Copy link
Contributor

BigLep commented Aug 22, 2023

@Jorropo @hacdias : I put some other comments in the done criteria. Feel free to adjust.

@Jorropo : given you're focused on the Bitswap test work, would this be useful to have @hacdias to fully finish off?

@Jorropo
Copy link
Contributor

Jorropo commented Aug 23, 2023

Mplex has been turned off by default in #10070.
#10095 exists to update go-libp2p and precisely handle the fact that libp2p don't have mplex support anymore while we do.

I think this should be closed.

I have already told @hacdias what were the mplex items and he already implemented them in #10095, #10095's remainings things are:

  • deal with the removal of quic draft 29 support (because all of our hardcoded maddrs are string we can't rely on the compiler to tell us everywhere they show up and have to manually handle them with grep and running tests until they fail)
    This maybe has been completed
  • Reviews

@BigLep
Copy link
Contributor

BigLep commented Aug 23, 2023

Thanks @Jorropo, you're right - my bad. Agreed this can stay close since we have met the done criteria and we're making sure new work (i.e., go-libp2p upgrade) continues to meet this criteria.

@BigLep BigLep closed this as completed Aug 23, 2023
@github-project-automation github-project-automation bot moved this from 🔎 In Review to 🎉 Done in IPFS Shipyard Team Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants