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

Don't retransmit old MAX_STREAM_DATA and MAX_DATA #807

Merged
merged 2 commits into from Oct 10, 2017
Merged

Conversation

janaiyengar
Copy link
Contributor

Closes #806

@janaiyengar janaiyengar self-assigned this Sep 28, 2017
previous unacknowledged MAX_STREAM_DATA frame for the same stream SHOULD NOT
be retransmitted since a newer MAX_STREAM_DATA frame for a stream obviates the
need for delivering older ones. Similarly, the most recent MAX_DATA frame MUST
be retransmitted; previous unacknowledged ones SHOULD NOT be retransmitted.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am starting to think it's a mistake to phrase this in terms of frames at all. It's the information you want to retransmit. It seems like there are lots of cases where you would be constantly updating your willingness to receive (if, say, you want to continue offering a given buffer size) but you only transmit occasionally. If I have to retransmit, why would I want to advertise stale data rather than the most current data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Anytime the words "retransmit frame" appear in the text, it's probably an error. And the phrase 'retransmit packet' should occur twice: Once to tell you not to do it and once to tell you can do it for connection close packets only.

@mikkelfj
Copy link
Contributor

mikkelfj commented Sep 29, 2017 via email

@ianswett
Copy link
Contributor

ianswett commented Sep 29, 2017 via email

@martinthomson
Copy link
Member

As Ian says, don't put anything else in the packet with CONNECTION_CLOSE. There are a few things that I would put in, but only maybe: RST_STREAM (so that the receiver can identify the stream that triggered the error), and PADDING.

@martinthomson
Copy link
Member

Taking this for now, on the understanding that we want to properly fix this later.

@martinthomson martinthomson merged commit d7b3a39 into master Oct 10, 2017
@martinthomson martinthomson deleted the old-max-data branch October 10, 2017 23:55
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

5 participants