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

Out-of-order Flow Control #370

Closed
MikeBishop opened this issue Mar 8, 2017 · 4 comments · Fixed by #378
Closed

Out-of-order Flow Control #370

MikeBishop opened this issue Mar 8, 2017 · 4 comments · Fixed by #378
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@MikeBishop
Copy link
Contributor

ILubashev's suggestion on #171 alludes to this; separating it out as a new issue.

Connection-level flow control is currently defined as a cumulative sum of all bytes received on all streams. However, if you receive STREAM frames out of order, you can know that more data is in flight, even if you haven't received it. Currently, that data implicitly counts against the receiver's stream window (since it's a max offset), but not against the connection window.

This would allow the receiver of a DISINTEREST frame to silently stop retransmission if the STREAM w/ FIN had already been acknowledged (without generating a RST_STREAM) since the flow control states on both sides would be consistent. However, it would also require more logic on STREAM receipt, since it's no longer just a running counter of the size of all received STREAM frames.

@martinthomson
Copy link
Member

Don't we already have a problem if a sender sends a STREAM frame that overlaps with what has already been sent. For example, you can't just count STREAM frame sizes if you receive octets 0-10 and 5-15 for the same stream. That's possibly already a problem receivers have to handle because it's just a generalized form of receiving 0-10 twice.

@MikeBishop
Copy link
Contributor Author

That's true; I misremembered the resolution to #146. Yes, the "sum of STREAM frames" has to come after duplicate detection -- it probably actually gets updated at the point where you add the new data into the buffer.

@marten-seemann
Copy link
Contributor

I don't think it makes sense to only count the bytes received for connection-level flow control. It makes sense to check for flow control violations as early as possible, so if a peer receives a certain offset on a stream, it makes sense to account for all bytes up to that offset in connection level flow control, no matter if they have already been received or if they're still outstanding.
A more compelling reason is that the duplicate detection you mentioned above is not even possible, as we discussed in #146 when we considered the CONFLICTING_STREAM_DATA error (and this was one of the reasons we decided against this error).

Since the reply to a DISINTEREST frame is a RST_STREAM frame (which contains a final byte offset), I don't see any problems with connection-level flow control (I can't find ILubashev's suggestion in #171 that @MikeBishop is referring to though, so maybe I missed his point).

Just as a side note, this is how GQUIC and quic-go are doing connection-level flow control at the moment.

@igorlord
Copy link
Contributor

igorlord commented Mar 9, 2017

The discussion was on the mailing list. I added a quick summary to #171.The most general form of the suggestion is:

  1. Treating STREAM+FIN and STREAM_RST much more similarly (for the purposes of window accounting as well as other purposes)
  2. Treating STREAM and STREAM+FIN/STREAM_RST similarly for the purposes of window accounting -- the topic of this issue.

martinthomson added a commit that referenced this issue Mar 9, 2017
Also, scales the connection-level flow control window by 1024

Closes #370, #340.
@mnot mnot added the design An issue that affects the design of the protocol; resolution requires consensus. label Apr 19, 2017
@mnot mnot added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Sep 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants