Conversation
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.
Problem
A client that advertises a larger
SETTINGS_INITIAL_WINDOW_SIZEthan the server stalls afteruploading 65535 bytes of request body on a stream. The server never sends a stream-level
WINDOW_UPDATE, so the client waits for flow-control credit that never comes.This came up with Envoy as a client of a gRPC server built on this driver. Envoy advertises a
256 MiB stream window by default. On a long-lived bidirectional stream it sent
16384 × 3 + 8663 + 7720 = 65535 bytes (exactly the window the server advertised) and stopped.
Limiting Envoy's
initial_stream_window_sizeto 65535 works around it.Root cause
Http2Driver::createStream()initialises both flow-control windows of a new stream from$this->initialWindowSize:handleSettings()overwrites$this->initialWindowSizewith the client'sSETTINGS_INITIAL_WINDOW_SIZE. Per RFC 9113 §6.5.2, that setting is the sender's initial windowfor receiving data. It therefore limits only what the server sends (
clientWindow). Theserver's receive window (
serverWindow) has to start at the size the server advertised in itsown SETTINGS frame, which is
DEFAULT_WINDOW_SIZE.With a 256 MiB client window,
serverWindowstarts at 268435456.handleData()only requestsmore credit once
$stream->serverWindow <= MINIMUM_WINDOW, which in practice never happens.The client, meanwhile, is correctly tracking the 65535-byte window the server advertised, so it
runs out of credit after 65535 bytes.
The existing tests miss this.
testFlowControldoes send a larger client window (66000), butit only checks data sent by the server, not a request body uploaded by the client.
Fix
serverWindownow starts atself::DEFAULT_WINDOW_SIZE. This is the same constant the driverwrites into its SETTINGS frame, so the advertised window and the tracked window cannot drift
apart.
clientWindowstill starts at the client's advertised value.handleSettings()isunchanged, because it already adjusts only the send windows of open streams.
golang.org/x/net/http2handles this the same way. It keepsinitialStreamSendWindowSize(taken from the peer's SETTINGS) separate from
initialStreamRecvWindowSize(what the serveradvertises), and
newStream()initialisesst.flowfrom the former andst.inflowfrom thelatter.
Test
testRequestStreamWindowIsReplenishedWhenClientAdvertisesLargerInitialWindow:WINDOW_UPDATEframe on that stream.The test does not check the size of the increment, since that depends on how the driver decides
to refill the window. On
3.xit fails with "The server did not replenish the stream window theclient has used up". With the fix it passes, and so does the full suite.