From 30ac16b5377785944635632c8e437b26a0e30660 Mon Sep 17 00:00:00 2001 From: Fabian Fett Date: Wed, 9 Feb 2022 20:51:08 +0100 Subject: [PATCH 1/2] Crash fix: HTTP2 can handle requests are cancelled --- .../HTTP2/HTTP2ClientRequestHandler.swift | 8 ++++-- .../HTTPRequestStateMachine.swift | 26 ++++++++++++++----- .../HTTP2ClientTests+XCTest.swift | 1 + .../HTTP2ClientTests.swift | 26 +++++++++++++++++++ 4 files changed, 52 insertions(+), 9 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift index 60c76a46d..8e78eb74c 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift @@ -193,11 +193,15 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler { case .forwardResponseBodyParts(let parts): self.request!.receiveResponseBodyParts(parts) - case .failRequest(let error, let finalAction): + case .failRequest(let error, _): self.request!.fail(error) self.request = nil self.runTimeoutAction(.clearIdleReadTimeoutTimer, context: context) - self.runFinalAction(finalAction, context: context) + // No matter the error reason, we must always make sure the h2 stream is closed. Only + // once the h2 stream is closed, it is released form the h2 multiplexer. The + // HTTPRequestStateMachine may signal finalAction: .none in the error case (as this is + // the right result for HTTP/1). In the h2 case we MUST always close. + self.runFinalAction(.close, context: context) case .succeedRequest(let finalAction, let finalParts): self.request!.succeedRequest(finalParts) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift index 08d042707..44e5e3b80 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift @@ -108,16 +108,24 @@ struct HTTPRequestStateMachine { } mutating func startRequest(head: HTTPRequestHead, metadata: RequestFramingMetadata) -> Action { - guard case .initialized = self.state else { - preconditionFailure("`start()` must be called first, and exactly once. Invalid state: \(self.state)") - } + switch self.state { + case .initialized: + guard self.isChannelWritable else { + self.state = .waitForChannelToBecomeWritable(head, metadata) + return .wait + } + return self.startSendingRequest(head: head, metadata: metadata) - guard self.isChannelWritable else { - self.state = .waitForChannelToBecomeWritable(head, metadata) + case .failed: + // The request state machine is marked as failed before the request is started, if + // the request was cancelled before hitting the channel handler. Before `startRequest` + // is called on the state machine, `willExecuteRequest` is called on + // `HTTPExecutableRequest`, which might loopback to state machines cancel method. return .wait - } - return self.startSendingRequest(head: head, metadata: metadata) + case .running, .finished, .waitForChannelToBecomeWritable, .modifying: + preconditionFailure("`startRequest()` must be called first, and exactly once. Invalid state: \(self.state)") + } } mutating func writabilityChanged(writable: Bool) -> Action { @@ -381,6 +389,10 @@ struct HTTPRequestStateMachine { case .initialized, .waitForChannelToBecomeWritable: let error = HTTPClientError.cancelled self.state = .failed(error) + // Okay, this has different semantics for HTTP/1 and HTTP/2. In HTTP/1 we don't want to + // close the connection, if we haven't send anything yet, to reuse the connection for + // another request. In HTTP/2 we must close the channel to ensure it is released from + // HTTP/2 multiplexer. return .failRequest(error, .none) case .running: diff --git a/Tests/AsyncHTTPClientTests/HTTP2ClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTP2ClientTests+XCTest.swift index b74b66820..e7f399658 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ClientTests+XCTest.swift @@ -34,6 +34,7 @@ extension HTTP2ClientTests { ("testUncleanShutdownCancelsExecutingAndQueuedTasks", testUncleanShutdownCancelsExecutingAndQueuedTasks), ("testCancelingRunningRequest", testCancelingRunningRequest), ("testReadTimeout", testReadTimeout), + ("testH2CanHandleRequestsThatHaveAlreadyHitTheDeadline", testH2CanHandleRequestsThatHaveAlreadyHitTheDeadline), ("testStressCancelingRunningRequestFromDifferentThreads", testStressCancelingRunningRequestFromDifferentThreads), ("testPlatformConnectErrorIsForwardedOnTimeout", testPlatformConnectErrorIsForwardedOnTimeout), ] diff --git a/Tests/AsyncHTTPClientTests/HTTP2ClientTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ClientTests.swift index 0269dafa7..eb1ac2ddc 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ClientTests.swift @@ -313,6 +313,32 @@ class HTTP2ClientTests: XCTestCase { } } + func testH2CanHandleRequestsThatHaveAlreadyHitTheDeadline() { + let bin = HTTPBin(.http2(compress: false)) + defer { XCTAssertNoThrow(try bin.shutdown()) } + var config = HTTPClient.Configuration() + var tlsConfig = TLSConfiguration.makeClientConfiguration() + tlsConfig.certificateVerification = .none + config.tlsConfiguration = tlsConfig + config.httpVersion = .automatic + let client = HTTPClient( + eventLoopGroupProvider: .createNew, + configuration: config, + backgroundActivityLogger: Logger(label: "HTTPClient", factory: StreamLogHandler.standardOutput(label:)) + ) + defer { XCTAssertNoThrow(try client.syncShutdown()) } + + var request: HTTPClient.Request? + XCTAssertNoThrow(request = try HTTPClient.Request(url: "https://localhost:\(bin.port)")) + + // just to establish an existing connection + XCTAssertNoThrow(try client.execute(request: XCTUnwrap(request)).wait()) + + XCTAssertThrowsError(try client.execute(request: XCTUnwrap(request), deadline: .now() - .seconds(2)).wait()) { + XCTAssertEqual($0 as? HTTPClientError, .deadlineExceeded) + } + } + func testStressCancelingRunningRequestFromDifferentThreads() { let bin = HTTPBin(.http2(compress: false)) { _ in SendHeaderAndWaitChannelHandler() } defer { XCTAssertNoThrow(try bin.shutdown()) } From d7a2cad3b84b4fbc6a3e868714b615b109875a6e Mon Sep 17 00:00:00 2001 From: Fabian Fett Date: Thu, 10 Feb 2022 10:12:38 +0100 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: George Barnett --- .../ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift | 2 +- .../ConnectionPool/HTTPRequestStateMachine.swift | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift index 8e78eb74c..cc6bd4899 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift @@ -198,7 +198,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler { self.request = nil self.runTimeoutAction(.clearIdleReadTimeoutTimer, context: context) // No matter the error reason, we must always make sure the h2 stream is closed. Only - // once the h2 stream is closed, it is released form the h2 multiplexer. The + // once the h2 stream is closed, it is released from the h2 multiplexer. The // HTTPRequestStateMachine may signal finalAction: .none in the error case (as this is // the right result for HTTP/1). In the h2 case we MUST always close. self.runFinalAction(.close, context: context) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift index 44e5e3b80..fa520a865 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift @@ -390,7 +390,7 @@ struct HTTPRequestStateMachine { let error = HTTPClientError.cancelled self.state = .failed(error) // Okay, this has different semantics for HTTP/1 and HTTP/2. In HTTP/1 we don't want to - // close the connection, if we haven't send anything yet, to reuse the connection for + // close the connection, if we haven't sent anything yet, to reuse the connection for // another request. In HTTP/2 we must close the channel to ensure it is released from // HTTP/2 multiplexer. return .failRequest(error, .none)