Repository navigation
Stop the RequestBag body stream after an early >= 300 response - #945
Open
beshkenadze wants to merge 1 commit into
Open
beshkenadze wants to merge 1 commit into
beshkenadze wants to merge 1 commit into
Conversation
Motivation: A server can send a complete response with a status >= 300 while a streamed request body is paused for backpressure. `HTTPRequestStateMachine` does not resume a request body stream after a >= 300 response head. When the response ends, the HTTP/1 and HTTP/2 channel handlers drop the request. After this, nothing completes the backpressure promise of the `RequestBag`. The `RequestBag` is the request of `HTTPClient.execute(request:delegate:)` with a `HTTPClient.Body.stream`. It has three failures in this case: - The response end arrives after the delegate consumed all response body parts, and a body write waits for the backpressure promise. The `RequestBag` waits for the end of the request body stream, but the body writer waits for the backpressure promise. The task never completes, and the write future never completes. - The response end arrives after the delegate consumed all response body parts, and no body write waits. The task does not complete at the response end. A later body write never completes. If the body stream ends without a write, HTTP/1 fails the task with `HTTPClientError.requestStreamCancelled`, and HTTP/2 never completes the task. - The response end arrives while the delegate still consumes response body parts. The task succeeds when the delegate is done, but the `RequestBag` drops the backpressure promise and does not complete it. The write future never completes. In all three cases, a body write that does not complete keeps the `RequestBag`, the delegate and all data that the writer buffers in memory. For example, a client that uploads a large file without credentials and gets a 401 keeps the upload code and its data alive for every such request. Modifications: - Record in `RequestBag.StateMachine` if the response status is >= 300. - When a response with a status >= 300 ends while the request body stream is paused, finish the `RequestBag` and succeed the task with the response. If a body write waits, fail its backpressure promise with `HTTPClientError.requestStreamCancelled`. This follows the rule of `HTTPRequestStateMachine`, which does not send more request body parts after a >= 300 response. Later body writes fail at once with the same error. A response with a status < 300 keeps the request body stream open, as before. - Add `RequestBagTests` for a response end after the delegate consumed the response, with and without a waiting body write, for a response end while the delegate consumes the response, and for a response with a status < 300 that keeps the upload. - Add a `HTTP1ClientChannelHandlerTests` test that runs the same flow through the HTTP/1 channel handler. Result: After a complete >= 300 response, the task succeeds with the response. A paused body write fails with `HTTPClientError.requestStreamCancelled`, and a later body write fails at once with the same error. The body writer ends and releases the `RequestBag`, the delegate and the buffered data. This changes the task result if no body write waits when the response ends. Before this change, the task did not complete at the response end. If the body stream then ended, HTTP/1 failed the task with `HTTPClientError.requestStreamCancelled`, and HTTP/2 did not complete the task. Now the task succeeds with the response at the response end. The behaviour for responses with a status < 300 does not change.
This branch has not been deployed
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.
Closes #941
Motivation
If the response ends while the
RequestBagbody stream is paused, no event completes the backpressure promise. The write future does not complete. In 2 of 3 cases, the task does not complete at the response end either.Modifications
RequestBag.StateMachinerecords if the response status is >= 300.RequestBagfinishes when the delegate consumes a complete >= 300 response while the request body stream is paused:HTTPClientError.requestStreamCancelled.One result changes if the delegate consumes a >= 300 response before its end and no body write waits:
HTTPClientError.requestStreamCancelled. HTTP/2 does not complete the task.Result
0171152, only the new tests addedRequestBagTests: 413, a body write waitsRequestBagTests: 413, no body write waitsRequestBagTests: 413, the delegate did not consume the response before its endRequestBagTests: 200HTTP1ClientChannelHandlerTests: 413 through the HTTP/1 channel handlerTransaction(async/await API) has the same bug: #944. The two changes modify different files. You can merge them in any order.