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

prevent receive gap #127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

rkuhn
Copy link

@rkuhn rkuhn commented Feb 4, 2022

When testing with a high-volume long-running substream, I found that a reset caused by exceeding the max_buffer_size led to subsequent reception of follow-up frames.
This is a rare occurrence, I was not able to write a test case for this: my assumption is that one frame triggers the error, the reset is sent, but data are in flight, and before the next frame is passed to on_data the client has already consumed a few bytes, freeing up space.
I’m not intimitately familiar with yamux, so please take this PR with a grain of salt, it may not be the right way of fixing the issue.

@mxinden
Copy link
Member

mxinden commented Feb 7, 2022

Can you share the yamux config with which you saw the issue?

I was not able to write a test case for this

Do you have a simple way to reproduce it?

@rkuhn
Copy link
Author

rkuhn commented Feb 7, 2022

The config was the default, and the use-case is not so trivial: I saw it while loading a substream as much as I could. What I actually saw was that my framing protocol was broken, instead of the usual (AB)* sequence of alternating header and frame that I wrote as separate writes into yamux, I saw once case of ABB, i.e. a header was missing. Closer inspection showed that a couple hundred frames earlier yamux had decided to ERROR out since “the buffer is growing without bound”, sending a reset to the other side, but reads kept on being served from the full buffer (client was backlogged due to “division by ten” a.k.a. JSON serialisation). This bug has been fixed in 0.42, thanks for that! (I was testing with 0.41).

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