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] Send whole data if window size is > 0 #319

Merged
merged 6 commits into from
Sep 14, 2023

Conversation

StefanBratanov
Copy link
Collaborator

@StefanBratanov StefanBratanov commented Sep 13, 2023

There were some inconsistencies with sendFrames method and the send buffer flush approach to sending data.

  • sendFrames was checking if window size is <= 0 and adding to buffer if so and afterwards was slicing the data based on the window size and sending across. So essentially if window size is 10 and data is 100 bytes, it will send across 10 frames of 10 bytes.
  • flush was respecting the window size and was sending partial data if the length of the data is less than the window size.

I would imagine a better approach is if window size is > 0, just send the whole data respecting the maxFrameDataLength.

  • Made flush not use sendFrames since the data is already limited by maxFrameDataLength so no slicing needed
  • Renamed sendFrames to sendData
  • Changed sendData to respect the window size and add to buffer there instead of onChildWrite
  • Modified tests accordingly
  • Release message when onChildWrite and no window size is found.

Copy link
Collaborator

@Nashatyrev Nashatyrev left a comment

Choose a reason for hiding this comment

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

LGTM

@StefanBratanov StefanBratanov merged commit 6f7e485 into libp2p:develop Sep 14, 2023
2 checks passed
@StefanBratanov StefanBratanov deleted the yamux_improv branch September 14, 2023 09:36
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.

2 participants