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

chore: using mplex instead of yamux #2958

Closed
wants to merge 1 commit into from

Conversation

gabrielmer
Copy link
Contributor

Description

Switching our multiplexer from yamux to mplex after finding the Futures leak in yamux reported in vacp2p/nim-libp2p#1165

Once it's taken care of we can switch back to yamux

Copy link

github-actions bot commented Aug 6, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2958

Built from 559c340

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Thanks for swift action here. I notice a corresponding issue has been raised in nim-libp2p. Has someone from that team been made aware and are they looking into it?

@gabrielmer
Copy link
Contributor Author

gabrielmer commented Aug 6, 2024

I notice a corresponding issue has been raised in nim-libp2p. Has someone from that team been made aware and are they looking into it?

I just opened the issue, however they have already been informed yesterday through a Discord message

Not sure if they started taking a look into it yet, I was waiting a bit to get some kind of answer/update on the new issue and if not ask directly. In any case, I will keep on top of this :))

@richard-ramos
Copy link
Member

We will need to add back to go-waku support to mplex since go-libp2p dropped support to it. Do you think this is something that we should do soon? or do you think the issue with yamux will be fixed before next nwaku release?

@gabrielmer
Copy link
Contributor Author

We will need to add back to go-waku support to mplex since go-libp2p dropped support to it. Do you think this is something that we should do soon? or do you think the issue with yamux will be fixed before next nwaku release?

Oh thank you, I wasn't aware of this. So I will create a thread on the Vac's p2p-pm Discord channel to see if they can estimate how long it will take to fix it. I'm personally not very familiar with the muxers' internals so couldn't propose a fix myself.

Will update on how it goes, but won't merge this until we coordinating it with go-waku. Depending on the estimation, either we wait for the fix or add again support for mplex in go-waku. Hopefully it's the former option.

@gabrielmer
Copy link
Contributor Author

It seems that a fix for the leak was found :)) so it seems that we won't need to switch back to mplex. Just in case, I'll close this PR once the fix is merged to nim-libp2p

cc @richard-ramos

@gabrielmer gabrielmer marked this pull request as draft August 9, 2024 10:11
@lchenut
Copy link

lchenut commented Aug 12, 2024

For your information, the fix is merged (vacp2p/nim-libp2p#1171).
@kaiserd will probably add it to the 1.5 release (vacp2p/nim-libp2p#1166).

@gabrielmer
Copy link
Contributor Author

gabrielmer commented Aug 14, 2024

Closing PR as the leak is fixed in nim-libp2p. Thank you @lchenut !

@gabrielmer gabrielmer closed this Aug 14, 2024
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.

4 participants