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

x/net/http2: Server window updates are too chatty #54730

Closed
hawkinsw opened this issue Aug 29, 2022 · 4 comments
Closed

x/net/http2: Server window updates are too chatty #54730

hawkinsw opened this issue Aug 29, 2022 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@hawkinsw
Copy link
Contributor

First, thank you all for the work that you do implementing and maintaining an excellent H2 library! Working with Go to implement a protocol over H2 is an absolute dream because of the work that you do!

In our use case (an implementation of the new proposed IETF RPM protocol for measuring responsiveness under working conditions) we make heavy use of the POST capabilities of the server implementation in H2. What we have noticed is that the Server code is a little too chatty sending per-connection and per-stream window update messages.

We know why this behavior happens. Not that you need to hear this (you implemented it, after all), but for the sake of completeness, here is our understanding:

  1. (after lots of initial code executes to initialize a stream [and subsequent to connection initialization]), a pipe (ultimately) connects
  • the independently executing go function that reads frames from the wire that the client is sending (this will call the Write method on the pipe); and
  • a user-specified handler function that calls Read on the requestBody (this will call the Read method on the pipe)
  1. A Read on the pipe blocks on a condition variable that is signaled by the Write side of the pipe whenever data is sent in to the pipe.
  2. Because the Write-side of the pipe happens every time a data frame is received, Writes put MaxReadFrameSize (approximately 16k) bytes in to the pipe at a time.
  3. For every MaxReadFrameSize bytes put in to the pipe, the condition variable is signaled which
  4. ultimately wakes up the Read blocked in the user-supplied handler.
  5. Because of how efficient and awesome the go runtime is, the Read usually unblocks before another frame is read from the wire (no matter how fast you can push data along the wire).
  6. Therefore, at a max, the user-supplied handler function is only (again, usually) able to successfully read MaxReadFrameSize bytes at a time from the request body (and that is after optimization [NB: In our application we discard the bytes pushed by the server and even io.Discard is too timid in this respect! (yes, I am running out nested bracket sigils!)]).
  7. When the user-supplied handler acknowledges the bytes it read from the body of a request, serverConn.noteReadFromBodyHandler is invoked which (ultimately) triggers an unconditional window update for both the stream and the connection.
  8. Voila, we arrive at the ultimate behavior that the server implementation of the H2 library sends window updates at increments that are more frequent than 16k.

Whew. Sorry for the word salad! I hope that is legible and reasonable.

According to the H2 spec:

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. Receivers are advised to have mechanisms in place to avoid sending WINDOW_UPDATE frames with very small increments; see Section 4.2.3.3 of [RFC1122].

The quoted RFC encourages window updates only be sent when they expand the buffer by 1/2 the window size. [p 98[(https://www.rfc-editor.org/rfc/rfc1122.html#page-98).

We propose that the server implementation of the H2 library be modified to work in this fashion. We are accompanying this proposal (which we hope is following the proper procedures!) with a patch to implement the behavior. The patch does not add additional public APIs -- we think that's a plus.

Again, thank you so much for all the work that you put in to creating and maintaining this excellent library and we hope that this proposal and patch are helpful.

Sincerely,
Will

@gopherbot gopherbot added this to the Proposal milestone Aug 29, 2022
@hawkinsw
Copy link
Contributor Author

As promised, here is the corresponding proposed patch: https://go-review.googlesource.com/c/net/+/426414

@seankhliao seankhliao changed the title proposal: x/net/http2: Server window updates are too chatty x/net/http2: Server window updates are too chatty Aug 29, 2022
@seankhliao seankhliao removed this from the Proposal milestone Aug 29, 2022
@seankhliao seankhliao added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Aug 29, 2022
@seankhliao
Copy link
Member

not a proposal as there's no api change

is this the same as #28732 ?

cc @neild

@neild
Copy link
Contributor

neild commented Aug 29, 2022

Yeah, this is the same as #28732, which I hadn't seen until now. Left a comment on @fraenkel's CL (https://go-review.git.corp.google.com/c/net/+/150197/) as well.

@neild
Copy link
Contributor

neild commented Aug 29, 2022

Duplicate of #28732.

@neild neild closed this as completed Aug 29, 2022
@golang golang locked and limited conversation to collaborators Aug 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants