Repository navigation
Keep the HTTP/2 request until its request body stream ends - #942
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: An HTTP/2 server can send a complete response with a status below 300 while the client still streams the request body. In this case, `HTTPRequestStateMachine` returns `.forwardResponseEnd(.none, ...)`. The `.none` action means that the request is not done yet. The request body stream continues. `HTTP2ClientRequestHandler` ignored this action and released the request. The state machine still expected the request. This caused three failures: - The channel became writable again and the state machine resumed the request body stream. The handler force unwrapped the released request and crashed. - A stream reset or a connection close failed the request. The handler force unwrapped the released request and crashed. This is the crash in swift-server#922. - The handler dropped the remaining request body parts without sending them. It also dropped the request body end. It did not write END_STREAM, so the HTTP/2 stream stayed half-open. `Transaction` and `RequestBag` did not report an error to the body writer. Modifications: - Keep the request when the final action of `.forwardResponseEnd` is `.none`. `HTTP1ClientChannelHandler` does the same. - Release the request in `.sendRequestEnd` when the final action is `.requestDone` or `.close`. - Add tests for a resumed request body stream, a stream reset, a connection close and the request body end after an early response end. - Add a test for a request body part that arrives after the request end. The handler must reject the part with `HTTPClientError.requestStreamCancelled`. Without the release in `.sendRequestEnd`, the idle write timeout state machine crashes. Result: After an early response end with a status below 300, the HTTP/2 handler sends the remaining request body and END_STREAM. Then the stream closes. A stream reset or a connection close fails the request and does not crash the client. The behavior for responses with a status of 300 or more does not change. Fixes swift-server#922.
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.
Fixes #922
Motivation
On HTTP/2, a server can end a response with a status below 300 before the request body ends. Since #879 (1.31.0),
HTTPRequestStateMachinethen returns.forwardResponseEnd(.none, ...). The request body continues, butHTTP2ClientRequestHandlersetsself.requesttonil. The next action force unwrapsnil:.failRequestcrashes atHTTP2ClientRequestHandler.swift:249. A stream reset, a connection close or a GOAWAY with a lower last stream ID causes it. This is the crash in Crash inHTTP2ClientRequestHandler#922..resumeRequestBodyStreamcrashes at line 228 when the channel becomes writable again.Before the crash, the handler rejects the remaining body parts with
HTTPClientError.requestStreamCancelledand ignores the request end. The client sends no END_STREAM and no RST_STREAM. The stream stays half-closed (remote).Changes
.forwardResponseEndwith the final action.none, the handler keeps the request.HTTP1ClientChannelHandlerdoes the same..sendRequestEndwith the final action.requestDoneor.close, the handler releases the request.5 new tests in
HTTP2ClientRequestHandlerTestsend a 200 response while the body streams. Then each test does one step:NIOHTTP2Errors.StreamClosedwithCANCEL).channelInactiveevent.Result
On macOS 27.0.1 with Swift 6.4:
HTTP2ClientRequestHandlerTests0171152Other PRs
HTTP2ClientRequestHandler#922, changesself.request!.fail(error)toself.request?.fail(error). This stops the crash at line 249, but the handler still releases the request too early. The two PRs merge without conflicts. With both, the request gets the stream error. Thus the test of fix(http2): tolerate stream errors after response completion #933 must expect.failas the last event.