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

yamux: Update yamux with window tuning and backport fixes #256

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Sep 26, 2024

This PR updates the internal yamux crate to include the following:

  • yamux window auto tuning similar to QUIC
  • Wake up readers to not miss EOF
  • pending frames are replace with pending read and write frame for propagating backpressure

Updates are backported from upstream to work with the current yamux::Control and yamux::ControlledConnection.
The Control and ControlleConnection are used by litep2p and have been removed by upstream. This will lead to a major litep2p refactor which may take significantly longer than backporting.

@lexnv lexnv added the enhancement New feature or request label Sep 26, 2024
@lexnv lexnv self-assigned this Sep 26, 2024
Signed-off-by: Alexandru Vasile <[email protected]>
@dmitry-markin
Copy link
Collaborator

Can you remind why we carry our own yamux fork with litep2p? I remember some functionality was removed from the upstream?

May be we can extract yamux as litep2p-yamux and try keeping it in sync with upstream, only with the needed patch applied? Backporting every yamux change in-tree of litep2p seems to be a lot of work, and they will diverge sooner or later anyway.

@altonen
Copy link
Contributor

altonen commented Nov 11, 2024

Yamux was included in litep2p because there was a memory leak which had been fixed in a later version but that version also had quite significant API breakage which would've required at least a partial rewrite of TCP/WebSocket connections handlers. More context here: #55

@lexnv
Copy link
Collaborator Author

lexnv commented Nov 11, 2024

I've also had a brief look, the refactor needed to adjust to the upstream API is quite involved. We can probably handle this as a follow-up post stabilization in substrate 🙏

I think multistream-select has a similar story, however I'm not sure what changed in terms of compatibility there

Comment on lines +805 to +807
if let Some(w) = shared.reader.take() {
w.wake()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this backported from the upstream yamux?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've backported all commits IIRC, will double check to be sure. Cherry-picking was not an option since we diverge in the API

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was 066dae3 cherry picked completely from upstream yamux? If not, what parts were updated manually?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was 8301fff cherry-picked from the upstream? What parts were manually modified?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants