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

Properly handle receipt of StreamDataBlocked frame #754

Merged
merged 1 commit into from
Jun 22, 2020

Conversation

agrover
Copy link
Contributor

@agrover agrover commented Jun 22, 2020

Generate a MaxStreamData frame if we receive a StreamDataBlocked frame.

We should now send MaxStreamData when either a packet with MaxStreamData is lost (#726) or if we receive StreamDataBlocked (this PR).

@agrover agrover requested a review from ddragana June 22, 2020 19:56
@ddragana
Copy link
Contributor

I would like to understand what was the issue in #741. Was the neqo flow control updates lost? if they were lost neqo would retransmit them at some point, so probably neqo has not been sending the updates.
We also need a better algorithm for, similar to #733 otherwise we will be flow control limited.

@agrover
Copy link
Contributor Author

agrover commented Jun 22, 2020

I would like to understand what was the issue in #741. Was the neqo flow control updates lost? if they were lost neqo would retransmit them at some point, so probably neqo has not been sending the updates.

Neqo wasn't properly re-sending FC updates when lost until #726 was merged 6 days ago. #741 was opened 5 days ago and I'm not sure Docker image was immediately updated, so that part of #741 could already be fixed. The other part of #741, not sending MaxStreamData when received StreamDataBlocked, is what this PR fixes.

We also need a better algorithm for, similar to #733 otherwise we will be flow control limited.

Yes that's #733. That shouldn't block this PR. We should be able to pass the QNS transferloss and transfercorruption tests with the current algorithm, and then work to improve the algorithm in a later PR.

@ddragana
Copy link
Contributor

I am sure I have seen the same or similar problem after the docker update.
For example: https://interop.seemann.io/logs_2020-06-21T09:20:46UTC/quiche_neqo/transferloss/output.txt

@ddragana
Copy link
Contributor

I am not sure this will fix #741

@agrover
Copy link
Contributor Author

agrover commented Jun 22, 2020

OK, let's keep #741 open for now then.

Generate a MaxStreamData frame if we receive a StreamDataBlocked frame.

fixes mozilla#741
@mergify mergify bot merged commit f968009 into mozilla:master Jun 22, 2020
@agrover agrover deleted the fix-741 branch June 23, 2020 00:31
@martinthomson
Copy link
Member

Why are we doing this for STREAM_DATA_BLOCKED and not DATA_BLOCKED ? I know that our defaults are different, but this is a little inconsistent.

@agrover
Copy link
Contributor Author

agrover commented Jun 23, 2020

I think we're already doing the right thing for data blocked, if you want to look and see if you are in agreement.

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.

None yet

3 participants