diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift index 60c76a46d..cc6bd4899 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 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) 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..fa520a865 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 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) 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()) }