Split WINDOW_UPDATE into MAX_DATA and MAX_STREAM_DATA #450

martinthomson · 2017-04-20T04:27:43Z

This also changes the name of the LIMIT_UPDATE frame to MAX_STREAM_ID to match.

A lot of the text talked about stream offsets and had flow control affect
maximum stream offsets. This turned out to be unnecessarily obtuse. The text
in this changeset simply says that there is a limit to the amount of data that
can be sent. This turns out to be a lot of changes, but I think that it is
easier to understand as a result.

Closes #443.

This also changes the name of the LIMIT_UPDATE frame to MAX_STREAM_ID to match. A lot of the text talked about stream offsets and had flow control affect maximum stream offsets. This turned out to be unnecessarily obtuse. The text in this changeset simply says that there is a limit to the amount of data that can be sent. This turns out to be a lot of changes, but I think that it is easier to understand as a result. Closes #443.

janaiyengar

I like this change -- a few comments inline

janaiyengar · 2017-04-20T05:14:10Z

draft-ietf-quic-transport.md

+When counting data toward this limit, an endpoint accounts for the maximum
+offset of data that is sent or received on every stream.  Loss or reordering can
+mean that the maximum offset is greater than the total size of data received on
+a stream.  Receiving STREAM frames might not increase the maximum offset on a


"Loss or reordering can
+mean that the maximum offset is greater than the total size of data received on
+a stream." -- Not sure what this means.

Hmm, that's old text, it says that for any given stream max_offset >= sum([x.len for x in all_stream_frames]) (as opposed to ==).

janaiyengar · 2017-04-20T05:15:28Z

draft-ietf-quic-transport.md

+When counting data toward this limit, an endpoint accounts for the maximum
+offset of data that is sent or received on every stream.  Loss or reordering can
+mean that the maximum offset is greater than the total size of data received on
+a stream.  Receiving STREAM frames might not increase the maximum offset on a


This sentence does not make sense: "Receiving STREAM frames might not increase the maximum offset on a stream if the maximum offset doesn't increase."

It's true, just a little repetitive.

janaiyengar · 2017-04-20T05:16:43Z

draft-ietf-quic-transport.md

+mean that the maximum offset is greater than the total size of data received on
+a stream.  Receiving STREAM frames might not increase the maximum offset on a
+stream if the maximum offset doesn't increase.  A STREAM frame with a FIN bit
+set or RST_STREAM causes the final offset for a stream to be fixed.


I don't think you need to describe how RST_STREAM or FIN affects STREAM offsets here. MAX_DATA is for Connection flow control, and this information is useful only for STREAM flow control.

OK, dropping this (BTW, this is existing text).

Can you move this text to the MAX_STREAM_DATA section instead? It was written for WINDOW_UPDATE, which covered both connection and stream flow control.

janaiyengar · 2017-04-20T05:30:41Z

draft-ietf-quic-transport.md


 A receiver MUST close the connection with a
 QUIC_FLOW_CONTROL_RECEIVED_TOO_MUCH_DATA error ({{error-handling}}) if the
-peer violates the advertised stream or connection flow control windows.
+peer violates the advertised limits on data on a connection or stream.


suggestion: "violates the advertised connection or stream data limits."

janaiyengar · 2017-04-20T05:33:00Z

draft-ietf-quic-transport.md

+
+Implementations will likely want to increase the maximum stream ID as
+peer-initiated streams close.  Sending a MAX_STREAM_ID frame along with ACK
+frames ensures a regular increase in the stream limit without generating excess


Sending a MAX_STREAM_ID with each ACK makes no sense. Perhaps you meant to say: "... along with acknowledgements of frames that close a stream"?

Not exactly. Implies a particular strategy for increasing the maximum stream ID, which I want to avoid. The point is that you might send any MAX_STREAM_ID frames you might want to send on the same schedule as ACK. I'll try to improve it some.

I realize that the context is bad for that, so I'll try to fix that too.

An ACK is sent about every two packets received, which is not the schedule that a MAX_STREAM_ID frame would be sent. I've left a comment in my review of the new text.

janaiyengar · 2017-04-20T16:03:08Z

draft-ietf-quic-transport.md

+mean that the maximum offset on a stream can be greater than the total size of
+data received on that stream.  Conversely, receiving STREAM frames might not
+increase the maximum offset on a stream if the maximum offset in the frame is
+lower than the maximum received for that stream.


I understand now what this says, but it's confusing, since maximum offset and MAX_DATA both represent maxima. I would suggest using "largest received offset" instead of "maximum offset".

janaiyengar · 2017-04-20T16:05:37Z

draft-ietf-quic-transport.md

-set or RST_STREAM causes the final offset for a stream to be fixed.
+mean that the maximum offset on a stream can be greater than the total size of
+data received on that stream.  Conversely, receiving STREAM frames might not
+increase the maximum offset on a stream if the maximum offset in the frame is


"might not" --> "will not". If the STREAM frame's largest offset is lower than the largest received so far, the largest received offset will not increase.

janaiyengar · 2017-04-20T16:30:20Z

draft-ietf-quic-transport.md

-peer-initiated streams close.  Sending a MAX_STREAM_ID frame along with ACK
-frames ensures a regular increase in the stream limit without generating excess
+Sending any increase to the maximum stream ID in the same packet as an ACK frame
+ensures a regular increase in the stream limit without generating excess


This advice is true for all frames, including MAX_DATA and MAX_STREAM_DATA frames. I'd remove this sentence or say something about when to send the MAX_STREAM_ID frame rather than which frame to bundle it with. It might make sense to send it immediately after/along with a STREAM frame that carries a FIN or a RST_STREAM frame on a peer-initiated stream, for instance.

Yep, I think that I will just remove the paragraph. The advice on when to send is still present in the next paragraph, which is what you are looking for.

martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels Apr 20, 2017

martinthomson assigned janaiyengar Apr 20, 2017

martinthomson added 2 commits April 20, 2017 14:38

The hazards of renaming things automatically

eba4f04

Fix IANA table

b825954

janaiyengar reviewed Apr 20, 2017

View reviewed changes

martinthomson added 2 commits April 20, 2017 15:47

Jana's review comments

747885f

Fix frame type numbers

1f02a95

martinthomson mentioned this pull request Apr 20, 2017

Split BLOCKED #454

Merged

janaiyengar reviewed Apr 20, 2017

View reviewed changes

More feedback from Jana

f76f760

martinthomson mentioned this pull request Apr 21, 2017

Change RST_STREAM impact on state #459

Merged

janaiyengar approved these changes Apr 25, 2017

View reviewed changes

janaiyengar merged commit 735db70 into master Apr 25, 2017

martinthomson deleted the limit_update branch May 3, 2017 03:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split WINDOW_UPDATE into MAX_DATA and MAX_STREAM_DATA #450

Split WINDOW_UPDATE into MAX_DATA and MAX_STREAM_DATA #450

martinthomson commented Apr 20, 2017

janaiyengar left a comment

janaiyengar Apr 20, 2017

martinthomson Apr 20, 2017

janaiyengar Apr 20, 2017

martinthomson Apr 20, 2017

janaiyengar Apr 20, 2017

martinthomson Apr 20, 2017

janaiyengar Apr 20, 2017

janaiyengar Apr 20, 2017

janaiyengar Apr 20, 2017

martinthomson Apr 20, 2017

martinthomson Apr 20, 2017

janaiyengar Apr 20, 2017

janaiyengar Apr 20, 2017

janaiyengar Apr 20, 2017

janaiyengar Apr 20, 2017

martinthomson Apr 20, 2017

Split WINDOW_UPDATE into MAX_DATA and MAX_STREAM_DATA #450

Split WINDOW_UPDATE into MAX_DATA and MAX_STREAM_DATA #450

Conversation

martinthomson commented Apr 20, 2017

janaiyengar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment