Repository navigation
Stop the request body stream after an early >= 300 response - #944
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 the client still uploads a streamed request body. `HTTPRequestStateMachine` pauses the request body stream when the head arrives. It moves to `.finished` when the end arrives. The HTTP/1 and HTTP/2 channel handlers then drop the request, so nothing asks the `Transaction` to resume its request body stream again. A body write that waits for demand in `Transaction.writeRequestBodyPart` stays suspended forever. The suspended write keeps the request body pump task, the `Transaction` and the body's async iterator in memory. The iterator keeps all the data that it buffers. This occurs even after the user drops the response. Only an explicit `Transaction.cancel()` releases them, but `execute()` has already returned at that point. For example, a client can upload a large blob without credentials first and get a 401 while the blob streams. That client keeps the complete buffered blob in memory for every such upload. Modifications: - Keep the response status in the `streamingBody` response state of `Transaction.StateMachine`. - When the response ends with a status >= 300 while the request body stream is paused, finish the transaction. Then resume the waiting body write with `HTTPClientError.requestStreamCancelled`. This follows the rule of `HTTPRequestStateMachine`, which does not send more request body parts after a >= 300 response. A response with a status < 300 keeps the request body stream open, as before. - Add `TransactionTests` that check that the body's async iterator is released after an early 401. One test covers a body write that waits for demand. One test covers a pump that waits for the next body part. In this case, the next body write fails and does not wait. A third test checks that the request body stream still resumes after an early 200. Result: After a complete >= 300 response, the request body stream ends and releases its iterator and the buffered data without a cancellation. The response and the behaviour for responses with a status < 300 do 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 #939
Motivation
After a complete >= 300 response, a paused body write stays suspended. It keeps the
Transaction, the body's async iterator and its buffered data, even afterHTTPClient.shutdown().Modifications
Transaction.StateMachinekeeps the response status instreamingBody.HTTPClientError.requestStreamCancelled. This followsHTTPRequestStateMachine, which sends no more body parts.TransactionTestsmain(commands in #939)testRequestBodyStreamIsReleasedIfResponseAtLeast300EndsWhileUploadIsPausedEventLoopFutureTimeoutError. AHC does not release the iterator in 5 s.testRequestBodyStreamIsReleasedIfResponseAtLeast300EndsWhileWaitingForNextBodyPartEventLoopFutureTimeoutError. AHC does not release the iterator in 5 s.testRequestBodyStreamResumesIfResponseBelow300EndsWhileUploadIsPausedResult
After the write fails, the body task ends and releases the iterator. A task in
next()ends whennext()returns. Buffered body data stays until the caller releases the response and request. The response does not change.Memory test: https://github.com/beshkenadze/async-http-client-early-response-leak (64 MiB
PUT, 401 response). The program releases the response and request, then waits 30 s.mainand 1.36.2, HTTP/1.1 and HTTP/2 (8 runs for each body)afcf468, HTTP/1.1 and HTTP/2 (4 runs for each body)To repeat in the repro directory:
The README has the HTTP/1.1 and lazy body variants.
Raw data:
results/upstream.jsonl,results/fix-verification.jsonl.afcf468("ahcRef":"local@afcf468") is an earlier local version of this commit, not on GitHub. ItsSources/tree (6b69592) matches this branch. OnlyTransactionTests.swiftdiffers.#945 fixes the same bug in
HTTPClient.execute(request:delegate:)(RequestBag). The two PRs change different files and merge in any order.