From 1cfcadaf10fb851360e173ef3dd4af488a962c2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ale=C5=A1=20Bizjak?= Date: Sat, 9 Jan 2021 21:39:17 +0100 Subject: [PATCH 1/3] Make `waitStream` update connection flow control window. 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. --- examples/SimpleGet.lhs | 2 +- src/Network/HTTP2/Client/Helpers.hs | 16 +++++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/examples/SimpleGet.lhs b/examples/SimpleGet.lhs index 47b00e47..3e1351b7 100644 --- a/examples/SimpleGet.lhs +++ b/examples/SimpleGet.lhs @@ -85,7 +85,7 @@ client-originated streams (however with its own flow-control context). > resetPushPromises _ pps _ _ _ = _rst pps RefusedStream > > handler sfc _ = do -> waitStream stream sfc resetPushPromises >>= print . fromStreamResult +> waitStream conn stream sfc resetPushPromises >>= print . fromStreamResult We've defined an initializer and a handler so far. diff --git a/src/Network/HTTP2/Client/Helpers.hs b/src/Network/HTTP2/Client/Helpers.hs index 56497cf3..b4ae48af 100644 --- a/src/Network/HTTP2/Client/Helpers.hs +++ b/src/Network/HTTP2/Client/Helpers.hs @@ -116,11 +116,12 @@ upload dat flagmod conn connectionFlowControl stream streamFlowControl = do -- This function is fine if you don't want to consume results in chunks. See -- 'fromStreamResult' to collect the complicated 'StreamResult' into a simpler -- 'StramResponse'. -waitStream :: Http2Stream +waitStream :: Http2Client + -> Http2Stream -> IncomingFlowControl -> PushPromiseHandler -> ClientIO StreamResult -waitStream stream streamFlowControl ppHandler = do +waitStream conn stream streamFlowControl ppHandler = do ev <- _waitEvent stream case ev of StreamHeadersEvent fH hdrs @@ -131,10 +132,11 @@ waitStream stream streamFlowControl ppHandler = do return (Right hdrs, reverse dfrms, trls) StreamPushPromiseEvent _ ppSid ppHdrs -> do _handlePushPromise stream ppSid ppHdrs ppHandler - waitStream stream streamFlowControl ppHandler + waitStream conn stream streamFlowControl ppHandler _ -> error $ "expecting StreamHeadersEvent but got " ++ show ev where + connFlowControl = _incomingFlowControl conn waitDataFrames xs = do ev <- _waitEvent stream case ev of @@ -142,9 +144,13 @@ waitStream stream streamFlowControl ppHandler = do | HTTP2.testEndStream (HTTP2.flags fh) -> return ((Right x):xs, Nothing) | otherwise -> do - _ <- lift $ _consumeCredit streamFlowControl (HTTP2.payloadLength fh) - lift $ _addCredit streamFlowControl (HTTP2.payloadLength fh) + let size = HTTP2.payloadLength fh + _ <- lift $ _consumeCredit streamFlowControl size + lift $ _addCredit streamFlowControl size _ <- _updateWindow $ streamFlowControl + _ <- lift $ _consumeCredit connFlowControl size + lift $ _addCredit connFlowControl size + _ <- _updateWindow $ connFlowControl waitDataFrames ((Right x):xs) StreamPushPromiseEvent _ ppSid ppHdrs -> do _handlePushPromise stream ppSid ppHdrs ppHandler From 2ce0c0022d86782ac30287912c76eccfae18c6c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ale=C5=A1=20Bizjak?= Date: Sun, 10 Jan 2021 12:59:24 +0100 Subject: [PATCH 2/3] Do not double count connection credits. `creditDataFrames` is run as part of the main dispatch loop already, and does the job of crediting, but not sending the updates. --- src/Network/HTTP2/Client/Helpers.hs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Network/HTTP2/Client/Helpers.hs b/src/Network/HTTP2/Client/Helpers.hs index b4ae48af..c5f7c167 100644 --- a/src/Network/HTTP2/Client/Helpers.hs +++ b/src/Network/HTTP2/Client/Helpers.hs @@ -148,8 +148,6 @@ waitStream conn stream streamFlowControl ppHandler = do _ <- lift $ _consumeCredit streamFlowControl size lift $ _addCredit streamFlowControl size _ <- _updateWindow $ streamFlowControl - _ <- lift $ _consumeCredit connFlowControl size - lift $ _addCredit connFlowControl size _ <- _updateWindow $ connFlowControl waitDataFrames ((Right x):xs) StreamPushPromiseEvent _ ppSid ppHdrs -> do From ea5663731dc3ad37b278932e4ec8274510b7134e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ale=C5=A1=20Bizjak?= Date: Tue, 12 Jan 2021 20:04:31 +0100 Subject: [PATCH 3/3] Add documentation and a changelog entry. --- ChangeLog.md | 3 +++ src/Network/HTTP2/Client/Helpers.hs | 13 ++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/ChangeLog.md b/ChangeLog.md index 9360a29d..bd77ea60 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -2,6 +2,9 @@ ## Unreleased changes +- Change `waitStream` so that it automatically credits the connection. This + is a breaking change due to function signature change. + ## v0.10.0.0 - Export frameHttp2RawConnection to convert an RawHttp2Connection into an Http2FrameConnection. diff --git a/src/Network/HTTP2/Client/Helpers.hs b/src/Network/HTTP2/Client/Helpers.hs index c5f7c167..66192ec3 100644 --- a/src/Network/HTTP2/Client/Helpers.hs +++ b/src/Network/HTTP2/Client/Helpers.hs @@ -115,10 +115,13 @@ upload dat flagmod conn connectionFlowControl stream streamFlowControl = do -- -- This function is fine if you don't want to consume results in chunks. See -- 'fromStreamResult' to collect the complicated 'StreamResult' into a simpler --- 'StramResponse'. +-- 'StreamResponse'. waitStream :: Http2Client + -- ^The connection. -> Http2Stream + -- ^The stream to wait on. This stream must be part of the connection. -> IncomingFlowControl + -- ^Incoming flow control for the __stream__. -> PushPromiseHandler -> ClientIO StreamResult waitStream conn stream streamFlowControl ppHandler = do @@ -148,6 +151,14 @@ waitStream conn stream streamFlowControl ppHandler = do _ <- lift $ _consumeCredit streamFlowControl size lift $ _addCredit streamFlowControl size _ <- _updateWindow $ streamFlowControl + -- We also send a WINDOW_UPDATE for the connection in order + -- to not rely on external updateWindow calls. This reduces + -- latency and makes the function less error-prone to use. + -- Note that the main loop (dispatchLoop) already credits + -- the connection for received data frames, but it does not + -- send the WINDOW_UPDATE frames.. That is why we only send + -- those frames here, and we do not credit the connection + -- as we do the stream in the preceding lines. _ <- _updateWindow $ connFlowControl waitDataFrames ((Right x):xs) StreamPushPromiseEvent _ ppSid ppHdrs -> do