search

haskell-grpc-native / http2-client

A native HTTP2 client in Haskell
BSD 3-Clause "New" or "Revised" License
43 stars 27 forks source link

Issue with `waitStream` connection window updates #74

Closed abizjak closed 3 years ago

abizjak commented 3 years ago

I'll preface the issue with a caveat that my understanding of http2 flow control is fairly limited.

The Network.HTTP2.Client.Helpers.waitStream function only seems to send WINDOW_UPDATE frames for the specific stream, but not for the connection. It is my understanding that both need to be done separately, and I have experienced blocking behaviour because of this, concretely in connection with http2-grpc-haskell and its singleRequest function. This is a quote from RFC 7540, section 6.9.1

   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.

Since waitStream itself does not send WINDOW_UPDATE frames for the connection, in order for the stream itself to progress an external entity must be sending these updates. In the case of http2-grpc-haskell that is handled by a background thread, but the problem with that is that it is either wasteful if grpcClientConfigWindowUpdateDelay is set too low, or the latency is needlessly high if set too high.

I propose to add an additional argument, either Http2Client or IncomingFlowControl so that the waitStream function would make sure to send WINDOW_UPDATE frames for the connection.

If there is another way to achieve an analogous outcome then please let me know, and I suggest adding it to the documentation of the waitStream method.

lucasdicioccio commented 3 years ago

Hi Ales, your analysis is right.

The waitStream function is a helper that only knows about individual streams. And typically a background job https://github.com/lucasdicioccio/http2-client-exe/blob/master/app/Main.hs#L227-L230 or https://github.com/lucasdicioccio/http2-client-grpc/blob/master/src/Network/GRPC/Client/Helpers.hs#L54 ) is a simplistic way to solve the issue.

As far as Helpers are concerned, I'm not sure what the best approach is yet (modify waitStream here, provide another implementation here, or another implementation in the http2-grpc library). However I do not mind complexifying these modules as it is the right layer to "hide" this sort of complexity.

Will look at your PR to check if it double-counts the flow-control (i admit i don't remember all details either). If i remember right the library already records the per-connection flow-control credit.

abizjak commented 3 years ago

Hi, thanks for the prompt response.

I think you are right that my PR is double counting the flow since creditDataFrames is called in the main loop already, and that seems to credit the connection flow for each data frame already,.

UPDATE: I've amended the PR with this.