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

Flow control accounting uses the maximum data offset on each stream #378

Merged
merged 2 commits into from Mar 10, 2017

Conversation

martinthomson
Copy link
Member

@martinthomson martinthomson commented Mar 9, 2017

Also, scales the connection-level flow control window by 1024

Closes #370, #340, #192.

Note: I was just going to fix these separately, but they both hit exactly the same text. Seemed like I might as well kill two birds.

Also, scales the connection-level flow control window by 1024

Closes #370, #340.

An endpoint accounts for the maximum offset of data that is sent or received on
a stream. Loss or reordering can mean that the size of STREAM frames can be
less than the maximum data offset on a stream; similarly, STREAM frames might
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the size of a STREAM frame have to do with the maximum offset?

Copy link
Contributor

Choose a reason for hiding this comment

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

"...can mean that the maximum offset is greater than the total size of data received on a stream."

of 1024 octets. That is, the value here is multiplied by 1024 to determine
the actual flow control offset. The sender of this parameter sets the byte
offset for connection level flow control to this value. This is equivalent to
sending a WINDOW_UPDATE ({{frame-window-update}}) for stream 0 immediately
Copy link
Contributor

Choose a reason for hiding this comment

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

Better: for the connection. There is no stream 0.

Copy link
Contributor

@MikeBishop MikeBishop left a comment

Choose a reason for hiding this comment

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

Minor nits. Looks fine.


An endpoint accounts for the maximum offset of data that is sent or received on
a stream. Loss or reordering can mean that the size of STREAM frames can be
less than the maximum data offset on a stream; similarly, STREAM frames might
Copy link
Contributor

Choose a reason for hiding this comment

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

"...can mean that the maximum offset is greater than the total size of data received on a stream."

An endpoint accounts for the maximum offset of data that is sent or received on
a stream. Loss or reordering can mean that the size of STREAM frames can be
less than the maximum data offset on a stream; similarly, STREAM frames might
not cause the maximum offset on a stream. A STREAM frame with a FIN bit set or
Copy link
Contributor

Choose a reason for hiding this comment

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

"...might not cause the maximum offset on a stream to increase."

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

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

One thought, but looks fine. I'll merge anyways.

: A 64-bit unsigned integer indicating the flow control offset for the given
stream (for a stream ID other than 0) or the entire connection.

The flow control offset is expressed in units of octets for individual streams
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use a 1K multiplier for both stream and connection-level?

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered it, but streams don't need as much space and they might benefit from a more granular expression of where they are up to. We can refine this later.

@janaiyengar janaiyengar merged commit af1b2a0 into master Mar 10, 2017
@martinthomson martinthomson deleted the scale_connection_flow_control branch March 13, 2017 00:56
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.

Out-of-order Flow Control
5 participants