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

BLOCKED, SHOULD, and retransmission #924

Merged
merged 1 commit into from
Nov 14, 2017
Merged

Conversation

MikeBishop
Copy link
Contributor

Fixes #452, #65.

Sending BLOCKED is a SHOULD (not a MUST), since it's a debug mechanism and not vital to protocol functionality. Clarified retransmission rules to say that you SHOULD retransmit if the limit hasn't changed since the frame was sent, and not otherwise. (If it changed and you're blocked again, you presumably sent a new frame already.)

@marten-seemann
Copy link
Contributor

sgtm, but I'm still wondering if we should include the byte offset / max stream id in those debugging frames. It seems like we're making interpretation of these frame unnecessarily hard by having the receiver guess these values.

@ianswett
Copy link
Contributor

Interesting suggestion Marten. Given we acknowledge MAX_STREAM_DATA frames and such, I think we get most of the information we need, but I could be wrong.

@marten-seemann
Copy link
Contributor

I'm not saying it's impossible, but you'd need a data structure keeping tracking of sent MAX_[DATA, STREAM_DATA, STREAM_ID] frames and which of them was acked. There would still be some uncertainty, since it's not guaranteed that the ACK for those frames is received before or at the same time as the debugging frame.

@MikeBishop
Copy link
Contributor Author

I don't think it would be unreasonable to package the offset or stream ID in question that triggered sending the frame. That could also help the other side understand whether you're blocked because they misunderstimicated the window you would need, or if the packet containing their update was simply delayed and you should be fine now.

@marten-seemann
Copy link
Contributor

From the feedback here it sounds like this is worth writing a separate PR. I'll submit one soon. Don't want to block the merge of this one.

@MikeBishop
Copy link
Contributor Author

Split it to a second issue -- feel free to submit a PR against it.

@martinthomson martinthomson merged commit 85ffb9b into master Nov 14, 2017
@MikeBishop MikeBishop deleted the blocked_semantics branch January 21, 2018 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants