Make waitStream update connection flow control window.#75
Merged
lucasdicioccio merged 3 commits intohaskell-grpc-native:masterfrom Jan 13, 2021
Merged
Conversation
From RFC 7540 ``` Flow control in HTTP/2 is implemented using a window kept by each sender on every stream. The flow-control window is a simple integer value that indicates how many octets of data the sender is permitted to transmit; as such, its size is a measure of the buffering capacity of the receiver. Two flow-control windows are applicable: the stream flow-control window and the connection flow-control window. The sender MUST NOT send a flow-controlled frame with a length that exceeds the space available in either of the flow-control windows advertised by the receiver. Frames with zero length with the END_STREAM flag set (that is, an empty DATA frame) MAY be sent if there is no available space in either flow-control window. For flow-control calculations, the 9-octet frame header is not counted. After sending a flow-controlled frame, the sender reduces the space available in both windows by the length of the transmitted frame. The receiver of a frame sends a WINDOW_UPDATE frame as it consumes data and frees up space in flow-control windows. Separate WINDOW_UPDATE frames are sent for the stream- and connection-level flow-control windows. A sender that receives a WINDOW_UPDATE frame updates the corresponding window by the amount specified in the frame. ``` In particular `waitStream` did not automatically update the connection flow control window, leading to stalls, and reliance on external updates (i.e., another background thread had to periodically send connection window updates), which is less than ideal.
`creditDataFrames` is run as part of the main dispatch loop already, and does the job of crediting, but not sending the updates.
Member
|
Looks good, would you mind: adding some Changelog entry (especially, to credit yourself). I'll do some tests over next week and do some release (will need to bump the version). |
Contributor
Author
|
Yes, will do. |
This was referenced Jan 12, 2021
Contributor
Author
|
I added additional documentation and a changelog entry, as well as created two PRs in the repos you mentioned |
Member
|
Looks great! will need to do some manual checks before releasing but will be merging. Thanks, and making the other PRs is grand! |
10 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From RFC 7540
In particular
waitStreamdid not automatically update the connection flowcontrol window, leading to stalls, and reliance on external updates (i.e.,
another background thread had to periodically send connection window updates),
which is less than ideal.
Closes #74