From 4c5733d10f6bf08a6309ec49f0567990430fe009 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 22 Jun 2022 17:58:42 +0200 Subject: [PATCH 1/9] throw connection error if deadline is exceeded --- ...HTTPConnectionPool+HTTP1StateMachine.swift | 5 +- ...HTTPConnectionPool+HTTP2StateMachine.swift | 5 +- Sources/AsyncHTTPClient/HTTPClient.swift | 2 +- Sources/AsyncHTTPClient/HTTPHandler.swift | 4 +- .../RequestBag+StateMachine.swift | 58 +++++++++++++------ Sources/AsyncHTTPClient/RequestBag.swift | 39 +++++++++++-- .../HTTP1ClientChannelHandlerTests.swift | 2 +- .../HTTP2ClientRequestHandlerTests.swift | 2 +- .../HTTPClientTests.swift | 41 +++++++++++++ .../HTTPConnectionPool+HTTP1StateTests.swift | 9 +-- ...onnectionPool+HTTP2StateMachineTests.swift | 6 +- .../HTTPConnectionPoolTests.swift | 2 +- .../Mocks/MockRequestQueuer.swift | 8 +-- .../RequestBagTests.swift | 8 +-- 14 files changed, 145 insertions(+), 46 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift index d654f5a87..766ec25f7 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift @@ -323,9 +323,10 @@ extension HTTPConnectionPool { mutating func cancelRequest(_ requestID: Request.ID) -> Action { // 1. check requests in queue - if self.requests.remove(requestID) != nil { + if let request = self.requests.remove(requestID) { + let error = lastConnectFailure ?? HTTPClientError.cancelled return .init( - request: .cancelRequestTimeout(requestID), + request: .failRequest(request, error, cancelTimeout: true), connection: .none ) } diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift index 06fc36ad0..d778dda35 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift @@ -444,9 +444,10 @@ extension HTTPConnectionPool { mutating func cancelRequest(_ requestID: Request.ID) -> Action { // 1. check requests in queue - if self.requests.remove(requestID) != nil { + if let request = self.requests.remove(requestID) { + let error = lastConnectFailure ?? HTTPClientError.cancelled return .init( - request: .cancelRequestTimeout(requestID), + request: .failRequest(request, error, cancelTimeout: true), connection: .none ) } diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 45b2ce0ff..42e132535 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -606,7 +606,7 @@ public class HTTPClient { var deadlineSchedule: Scheduled? if let deadline = deadline { deadlineSchedule = taskEL.scheduleTask(deadline: deadline) { - requestBag.fail(HTTPClientError.deadlineExceeded) + requestBag.cancel(.deadlineExceeded) } task.promise.futureResult.whenComplete { _ in diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index c1ce39632..abf3443a1 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -556,7 +556,7 @@ extension URL { } protocol HTTPClientTaskDelegate { - func cancel() + func cancel(_ reason: CancelationReason) } extension HTTPClient { @@ -618,7 +618,7 @@ extension HTTPClient { return self._taskDelegate } - taskDelegate?.cancel() + taskDelegate?.cancel(.userInitiated) } func succeed(promise: EventLoopPromise?, diff --git a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift index 63cb15758..af5431577 100644 --- a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift +++ b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift @@ -31,6 +31,7 @@ extension RequestBag { fileprivate enum State { case initialized case queued(HTTPRequestScheduler) + case canceledWhileQueued(CancelationReason) case executing(HTTPRequestExecutor, RequestStreamState, ResponseStreamState) case finished(error: Error?) case redirected(HTTPRequestExecutor, Int, HTTPResponseHead, URL) @@ -95,7 +96,7 @@ extension RequestBag.StateMachine { case .initialized, .queued: self.state = .executing(executor, .initialized, .initialized) return true - case .finished(error: .some): + case .finished(error: .some), .canceledWhileQueued: return false case .executing, .redirected, .finished(error: .none), .modifying: preconditionFailure("Invalid state: \(self.state)") @@ -110,7 +111,7 @@ extension RequestBag.StateMachine { mutating func resumeRequestBodyStream() -> ResumeProducingAction { switch self.state { - case .initialized, .queued: + case .initialized, .queued, .canceledWhileQueued: preconditionFailure("A request stream can only be resumed, if the request was started") case .executing(let executor, .initialized, .initialized): @@ -150,7 +151,7 @@ extension RequestBag.StateMachine { mutating func pauseRequestBodyStream() { switch self.state { - case .initialized, .queued: + case .initialized, .queued, .canceledWhileQueued: preconditionFailure("A request stream can only be paused, if the request was started") case .executing(let executor, let requestState, let responseState): switch requestState { @@ -185,7 +186,7 @@ extension RequestBag.StateMachine { mutating func writeNextRequestPart(_ part: IOData, taskEventLoop: EventLoop) -> WriteAction { switch self.state { - case .initialized, .queued: + case .initialized, .queued, .canceledWhileQueued: preconditionFailure("Invalid state: \(self.state)") case .executing(let executor, let requestState, let responseState): switch requestState { @@ -231,7 +232,7 @@ extension RequestBag.StateMachine { mutating func finishRequestBodyStream(_ result: Result) -> FinishAction { switch self.state { - case .initialized, .queued: + case .initialized, .queued, .canceledWhileQueued: preconditionFailure("Invalid state: \(self.state)") case .executing(let executor, let requestState, let responseState): switch requestState { @@ -282,7 +283,7 @@ extension RequestBag.StateMachine { /// - Returns: Whether the response should be forwarded to the delegate. Will be `false` if the request follows a redirect. mutating func receiveResponseHead(_ head: HTTPResponseHead) -> ReceiveResponseHeadAction { switch self.state { - case .initialized, .queued: + case .initialized, .queued, .canceledWhileQueued: preconditionFailure("How can we receive a response, if the request hasn't started yet.") case .executing(let executor, let requestState, let responseState): guard case .initialized = responseState else { @@ -328,7 +329,7 @@ extension RequestBag.StateMachine { mutating func receiveResponseBodyParts(_ buffer: CircularBuffer) -> ReceiveResponseBodyAction { switch self.state { - case .initialized, .queued: + case .initialized, .queued, .canceledWhileQueued: preconditionFailure("How can we receive a response body part, if the request hasn't started yet.") case .executing(_, _, .initialized): preconditionFailure("If we receive a response body, we must have received a head before") @@ -385,7 +386,7 @@ extension RequestBag.StateMachine { mutating func succeedRequest(_ newChunks: CircularBuffer?) -> ReceiveResponseEndAction { switch self.state { - case .initialized, .queued: + case .initialized, .queued, .canceledWhileQueued: preconditionFailure("How can we receive a response body part, if the request hasn't started yet.") case .executing(_, _, .initialized): preconditionFailure("If we receive a response body, we must have received a head before") @@ -447,7 +448,7 @@ extension RequestBag.StateMachine { private mutating func failWithConsumptionError(_ error: Error) -> ConsumeAction { switch self.state { - case .initialized, .queued: + case .initialized, .queued, .canceledWhileQueued: preconditionFailure("Invalid state: \(self.state)") case .executing(_, _, .initialized): preconditionFailure("Invalid state: Must have received response head, before this method is called for the first time") @@ -482,7 +483,7 @@ extension RequestBag.StateMachine { private mutating func consumeMoreBodyData() -> ConsumeAction { switch self.state { - case .initialized, .queued: + case .initialized, .queued, .canceledWhileQueued: preconditionFailure("Invalid state: \(self.state)") case .executing(_, _, .initialized): @@ -531,9 +532,24 @@ extension RequestBag.StateMachine { preconditionFailure() } } + + enum CancelAction { + case cancelScheduler(HTTPRequestScheduler?) + case fail(FailAction) + } + + mutating func cancel(_ reason: CancelationReason) -> CancelAction { + switch self.state { + case .queued(let queuer) where reason == .deadlineExceeded: + self.state = .canceledWhileQueued(reason) + return .cancelScheduler(queuer) + default: + return .fail(self.fail(reason.error)) + } + } enum FailAction { - case failTask(HTTPRequestScheduler?, HTTPRequestExecutor?) + case failTask(Error, HTTPRequestScheduler?, HTTPRequestExecutor?) case cancelExecutor(HTTPRequestExecutor) case none } @@ -542,31 +558,39 @@ extension RequestBag.StateMachine { switch self.state { case .initialized: self.state = .finished(error: error) - return .failTask(nil, nil) + return .failTask(error, nil, nil) case .queued(let queuer): self.state = .finished(error: error) - return .failTask(queuer, nil) + return .failTask(error, queuer, nil) case .executing(let executor, let requestState, .buffering(_, next: .eof)): self.state = .executing(executor, requestState, .buffering(.init(), next: .error(error))) return .cancelExecutor(executor) case .executing(let executor, _, .buffering(_, next: .askExecutorForMore)): self.state = .finished(error: error) - return .failTask(nil, executor) + return .failTask(error, nil, executor) case .executing(let executor, _, .buffering(_, next: .error(_))): // this would override another error, let's keep the first one return .cancelExecutor(executor) case .executing(let executor, _, .initialized): self.state = .finished(error: error) - return .failTask(nil, executor) + return .failTask(error, nil, executor) case .executing(let executor, _, .waitingForRemote): self.state = .finished(error: error) - return .failTask(nil, executor) + return .failTask(error, nil, executor) case .redirected: self.state = .finished(error: error) - return .failTask(nil, nil) + return .failTask(error, nil, nil) case .finished(.none): // An error occurred after the request has finished. Ignore... return .none + case .canceledWhileQueued(let reason): + // if we just get a `HTTPClientError.cancelled` we can use the orignal cancelation reason + // to give a more descriptive error to the user. + if (error as? HTTPClientError) == .cancelled { + return .failTask(reason.error, nil, nil) + } + // otherwise we already had an intermidate connection error which we should present to the user instead + return .failTask(error, nil, nil) case .finished(.some(_)): // this might happen, if the stream consumer has failed... let's just drop the data return .none diff --git a/Sources/AsyncHTTPClient/RequestBag.swift b/Sources/AsyncHTTPClient/RequestBag.swift index dbef802e9..f27525ddd 100644 --- a/Sources/AsyncHTTPClient/RequestBag.swift +++ b/Sources/AsyncHTTPClient/RequestBag.swift @@ -320,8 +320,12 @@ final class RequestBag { let action = self.state.fail(error) + self.executeFailAction0(action) + } + + private func executeFailAction0(_ action: RequestBag.StateMachine.FailAction) { switch action { - case .failTask(let scheduler, let executor): + case .failTask(let error, let scheduler, let executor): scheduler?.cancelRequest(self) executor?.cancelRequest(self) self.failTask0(error) @@ -331,6 +335,33 @@ final class RequestBag { break } } + + private func cancel0(_ reason: CancelationReason) { + self.task.eventLoop.assertInEventLoop() + + let action = self.state.cancel(reason) + + switch action { + case .cancelScheduler(let scheduler): + scheduler?.cancelRequest(self) + case .fail(let failAction): + self.executeFailAction0(failAction) + } + } +} + +enum CancelationReason { + case userInitiated + case deadlineExceeded + + var error: HTTPClientError { + switch self { + case .userInitiated: + return .cancelled + case .deadlineExceeded: + return .deadlineExceeded + } + } } extension RequestBag: HTTPSchedulableRequest { @@ -456,12 +487,12 @@ extension RequestBag: HTTPExecutableRequest { } extension RequestBag: HTTPClientTaskDelegate { - func cancel() { + func cancel(_ reason: CancelationReason) { if self.task.eventLoop.inEventLoop { - self.fail0(HTTPClientError.cancelled) + self.cancel0(reason) } else { self.task.eventLoop.execute { - self.fail0(HTTPClientError.cancelled) + self.cancel0(reason) } } } diff --git a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift index f97580372..20aadacde 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift @@ -327,7 +327,7 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { XCTAssertNoThrow(try embedded.writeInbound(HTTPClientResponsePart.head(responseHead))) // canceling the request - requestBag.cancel() + requestBag.cancel(.userInitiated) XCTAssertThrowsError(try requestBag.task.futureResult.wait()) { XCTAssertEqual($0 as? HTTPClientError, .cancelled) } diff --git a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift index e67529ad8..f25e33d25 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift @@ -276,7 +276,7 @@ class HTTP2ClientRequestHandlerTests: XCTestCase { XCTAssertNoThrow(try embedded.writeInbound(HTTPClientResponsePart.head(responseHead))) // canceling the request - requestBag.cancel() + requestBag.cancel(.userInitiated) XCTAssertThrowsError(try requestBag.task.futureResult.wait()) { XCTAssertEqual($0 as? HTTPClientError, .cancelled) } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index ece09c52d..f6e74cffe 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -1268,6 +1268,47 @@ class HTTPClientTests: XCTestCase { #endif } } + + func testSelfSignedCertificateIsRejectedWithCorrectErrorIfRequestDeadlineIsExceeded() throws { + /// key + cert was created with the follwing command: + /// openssl req -x509 -newkey rsa:4096 -keyout self_signed_key.pem -out self_signed_cert.pem -sha256 -days 99999 -nodes -subj '/CN=localhost' + let certPath = Bundle.module.path(forResource: "self_signed_cert", ofType: "pem")! + let keyPath = Bundle.module.path(forResource: "self_signed_key", ofType: "pem")! + let configuration = TLSConfiguration.makeServerConfiguration( + certificateChain: try NIOSSLCertificate.fromPEMFile(certPath).map { .certificate($0) }, + privateKey: .file(keyPath) + ) + let sslContext = try NIOSSLContext(configuration: configuration) + + let server = ServerBootstrap(group: serverGroup) + .childChannelInitializer { channel in + channel.pipeline.addHandler(NIOSSLServerHandler(context: sslContext)) + } + let serverChannel = try server.bind(host: "localhost", port: 0).wait() + defer { XCTAssertNoThrow(try serverChannel.close().wait()) } + let port = serverChannel.localAddress!.port! + + var config = HTTPClient.Configuration() + config.timeout.connect = .seconds(3) + let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), configuration: config) + defer { XCTAssertNoThrow(try localClient.syncShutdown()) } + + XCTAssertThrowsError(try localClient.get(url: "https://localhost:\(port)", deadline: .now() + .seconds(2)).wait()) { error in + #if canImport(Network) + guard let nwTLSError = error as? HTTPClient.NWTLSError else { + XCTFail("could not cast \(error) of type \(type(of: error)) to \(HTTPClient.NWTLSError.self)") + return + } + XCTAssertEqual(nwTLSError.status, errSSLBadCert, "unexpected tls error: \(nwTLSError)") + #else + guard let sslError = error as? NIOSSLError, + case .handshakeFailed(.sslError) = sslError else { + XCTFail("unexpected error \(error)") + return + } + #endif + } + } func testFailingConnectionIsReleased() { let localHTTPBin = HTTPBin(.refuse) diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift index 49a6fb574..307869dd1 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift @@ -21,6 +21,7 @@ import XCTest class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { func testCreatingAndFailingConnections() { + struct SomeError: Error, Equatable {} let elg = EmbeddedEventLoopGroup(loops: 4) defer { XCTAssertNoThrow(try elg.syncShutdownGracefully()) } @@ -65,7 +66,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { // fail all connection attempts while let randomConnectionID = connections.randomStartingConnection() { - struct SomeError: Error, Equatable {} + XCTAssertNoThrow(try connections.failConnectionCreation(randomConnectionID)) let action = state.failedToCreateNewConnection(SomeError(), connectionID: randomConnectionID) @@ -86,9 +87,9 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { // cancel all queued requests while let request = queuer.timeoutRandomRequest() { - let cancelAction = state.cancelRequest(request) + let cancelAction = state.cancelRequest(request.0) XCTAssertEqual(cancelAction.connection, .none) - XCTAssertEqual(cancelAction.request, .cancelRequestTimeout(request)) + XCTAssertEqual(cancelAction.request, .failRequest(.init(request.1), SomeError(), cancelTimeout: true)) } // connection backoff done @@ -184,7 +185,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { // 2. cancel request let cancelAction = state.cancelRequest(request.id) - XCTAssertEqual(cancelAction.request, .cancelRequestTimeout(request.id)) + XCTAssertEqual(cancelAction.request, .failRequest(request, HTTPClientError.cancelled, cancelTimeout: true)) XCTAssertEqual(cancelAction.connection, .none) // 3. request timeout triggers to late diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift index 2574d3da2..e42a98ac7 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift @@ -212,7 +212,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { // 2. cancel request let cancelAction = state.cancelRequest(request.id) - XCTAssertEqual(cancelAction.request, .cancelRequestTimeout(request.id)) + XCTAssertEqual(cancelAction.request, .failRequest(request, HTTPClientError.cancelled, cancelTimeout: true)) XCTAssertEqual(cancelAction.connection, .none) // 3. request timeout triggers to late @@ -1242,9 +1242,9 @@ func XCTAssertEqualTypeAndValue( let lhs = try lhs() let rhs = try rhs() guard let lhsAsRhs = lhs as? Right else { - XCTFail("could not cast \(lhs) of type \(type(of: lhs)) to \(type(of: rhs))") + XCTFail("could not cast \(lhs) of type \(type(of: lhs)) to \(type(of: rhs))", file: file, line: line) return } - XCTAssertEqual(lhsAsRhs, rhs) + XCTAssertEqual(lhsAsRhs, rhs, file: file, line: line) }(), file: file, line: line) } diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPoolTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPoolTests.swift index 60e5077ee..72d94b3de 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPoolTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPoolTests.swift @@ -334,7 +334,7 @@ class HTTPConnectionPoolTests: XCTestCase { pool.executeRequest(requestBag) XCTAssertNoThrow(try eventLoop.scheduleTask(in: .seconds(1)) {}.futureResult.wait()) - requestBag.cancel() + requestBag.cancel(.userInitiated) XCTAssertThrowsError(try requestBag.task.futureResult.wait()) { XCTAssertEqual($0 as? HTTPClientError, .cancelled) diff --git a/Tests/AsyncHTTPClientTests/Mocks/MockRequestQueuer.swift b/Tests/AsyncHTTPClientTests/Mocks/MockRequestQueuer.swift index e81f1ed0a..520b51875 100644 --- a/Tests/AsyncHTTPClientTests/Mocks/MockRequestQueuer.swift +++ b/Tests/AsyncHTTPClientTests/Mocks/MockRequestQueuer.swift @@ -82,11 +82,11 @@ struct MockRequestQueuer { return waiter.request } - mutating func timeoutRandomRequest() -> RequestID? { - guard let waiterID = self.waiters.randomElement().map(\.0) else { + mutating func timeoutRandomRequest() -> (RequestID, HTTPSchedulableRequest)? { + guard let waiter = self.waiters.randomElement() else { return nil } - self.waiters.removeValue(forKey: waiterID) - return waiterID + self.waiters.removeValue(forKey: waiter.key) + return (waiter.key, waiter.value.request) } } diff --git a/Tests/AsyncHTTPClientTests/RequestBagTests.swift b/Tests/AsyncHTTPClientTests/RequestBagTests.swift index c80f8846b..d27b7eb8e 100644 --- a/Tests/AsyncHTTPClientTests/RequestBagTests.swift +++ b/Tests/AsyncHTTPClientTests/RequestBagTests.swift @@ -219,7 +219,7 @@ final class RequestBagTests: XCTestCase { XCTAssert(bag.eventLoop === embeddedEventLoop) let executor = MockRequestExecutor(eventLoop: embeddedEventLoop) - bag.cancel() + bag.cancel(.userInitiated) bag.willExecuteRequest(executor) XCTAssertTrue(executor.isCancelled, "The request bag, should call cancel immediately on the executor") @@ -261,7 +261,7 @@ final class RequestBagTests: XCTestCase { XCTAssertEqual(delegate.hitDidSendRequestHead, 1) XCTAssertEqual(delegate.hitDidSendRequest, 1) - bag.cancel() + bag.cancel(.userInitiated) XCTAssertTrue(executor.isCancelled, "The request bag, should call cancel immediately on the executor") XCTAssertThrowsError(try bag.task.futureResult.wait()) { @@ -295,7 +295,7 @@ final class RequestBagTests: XCTestCase { bag.requestWasQueued(queuer) XCTAssertEqual(queuer.hitCancelCount, 0) - bag.cancel() + bag.cancel(.userInitiated) XCTAssertEqual(queuer.hitCancelCount, 1) XCTAssertThrowsError(try bag.task.futureResult.wait()) { @@ -366,7 +366,7 @@ final class RequestBagTests: XCTestCase { // This simulates a race between the user cancelling the task (which invokes `RequestBag.cancel`) and the // call to `resumeRequestBodyStream` (which comes from the `Channel` event loop and so may have to hop. - bag.cancel() + bag.cancel(.userInitiated) bag.resumeRequestBodyStream() XCTAssertEqual(executor.isCancelled, true) From 4243ab880e902b21979083eebdfcfa52fbbb5b03 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Fri, 24 Jun 2022 10:20:01 +0200 Subject: [PATCH 2/9] ./scripts/generate_linux_tests.rb --- Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 3975036ea..b3a13486c 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -79,6 +79,7 @@ extension HTTPClientTests { ("testStressGetHttps", testStressGetHttps), ("testStressGetHttpsSSLError", testStressGetHttpsSSLError), ("testSelfSignedCertificateIsRejectedWithCorrectError", testSelfSignedCertificateIsRejectedWithCorrectError), + ("testSelfSignedCertificateIsRejectedWithCorrectErrorIfRequestDeadlineIsExceeded", testSelfSignedCertificateIsRejectedWithCorrectErrorIfRequestDeadlineIsExceeded), ("testFailingConnectionIsReleased", testFailingConnectionIsReleased), ("testResponseDelayGet", testResponseDelayGet), ("testIdleTimeoutNoReuse", testIdleTimeoutNoReuse), From 34f8a34f8b3d95c69b93525f4668433437421e63 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Fri, 24 Jun 2022 10:20:29 +0200 Subject: [PATCH 3/9] SwiftFormat --- .../HTTPConnectionPool+HTTP1StateMachine.swift | 2 +- .../HTTPConnectionPool+HTTP2StateMachine.swift | 2 +- Sources/AsyncHTTPClient/RequestBag+StateMachine.swift | 2 +- Sources/AsyncHTTPClient/RequestBag.swift | 6 +++--- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 4 ++-- .../HTTPConnectionPool+HTTP1StateTests.swift | 2 -- 6 files changed, 8 insertions(+), 10 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift index 766ec25f7..d1e705796 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift @@ -324,7 +324,7 @@ extension HTTPConnectionPool { mutating func cancelRequest(_ requestID: Request.ID) -> Action { // 1. check requests in queue if let request = self.requests.remove(requestID) { - let error = lastConnectFailure ?? HTTPClientError.cancelled + let error = self.lastConnectFailure ?? HTTPClientError.cancelled return .init( request: .failRequest(request, error, cancelTimeout: true), connection: .none diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift index d778dda35..f8054791d 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift @@ -445,7 +445,7 @@ extension HTTPConnectionPool { mutating func cancelRequest(_ requestID: Request.ID) -> Action { // 1. check requests in queue if let request = self.requests.remove(requestID) { - let error = lastConnectFailure ?? HTTPClientError.cancelled + let error = self.lastConnectFailure ?? HTTPClientError.cancelled return .init( request: .failRequest(request, error, cancelTimeout: true), connection: .none diff --git a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift index af5431577..a3b9199d8 100644 --- a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift +++ b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift @@ -532,7 +532,7 @@ extension RequestBag.StateMachine { preconditionFailure() } } - + enum CancelAction { case cancelScheduler(HTTPRequestScheduler?) case fail(FailAction) diff --git a/Sources/AsyncHTTPClient/RequestBag.swift b/Sources/AsyncHTTPClient/RequestBag.swift index f27525ddd..39e43a49a 100644 --- a/Sources/AsyncHTTPClient/RequestBag.swift +++ b/Sources/AsyncHTTPClient/RequestBag.swift @@ -322,7 +322,7 @@ final class RequestBag { self.executeFailAction0(action) } - + private func executeFailAction0(_ action: RequestBag.StateMachine.FailAction) { switch action { case .failTask(let error, let scheduler, let executor): @@ -335,7 +335,7 @@ final class RequestBag { break } } - + private func cancel0(_ reason: CancelationReason) { self.task.eventLoop.assertInEventLoop() @@ -353,7 +353,7 @@ final class RequestBag { enum CancelationReason { case userInitiated case deadlineExceeded - + var error: HTTPClientError { switch self { case .userInitiated: diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index f6e74cffe..02c60d177 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -1268,7 +1268,7 @@ class HTTPClientTests: XCTestCase { #endif } } - + func testSelfSignedCertificateIsRejectedWithCorrectErrorIfRequestDeadlineIsExceeded() throws { /// key + cert was created with the follwing command: /// openssl req -x509 -newkey rsa:4096 -keyout self_signed_key.pem -out self_signed_cert.pem -sha256 -days 99999 -nodes -subj '/CN=localhost' @@ -1292,7 +1292,7 @@ class HTTPClientTests: XCTestCase { config.timeout.connect = .seconds(3) let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), configuration: config) defer { XCTAssertNoThrow(try localClient.syncShutdown()) } - + XCTAssertThrowsError(try localClient.get(url: "https://localhost:\(port)", deadline: .now() + .seconds(2)).wait()) { error in #if canImport(Network) guard let nwTLSError = error as? HTTPClient.NWTLSError else { diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift index 307869dd1..7f59fd4e1 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift @@ -66,8 +66,6 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { // fail all connection attempts while let randomConnectionID = connections.randomStartingConnection() { - - XCTAssertNoThrow(try connections.failConnectionCreation(randomConnectionID)) let action = state.failedToCreateNewConnection(SomeError(), connectionID: randomConnectionID) From 0477ef9ee9bdc1e538d8ba20096a94b7b852388e Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Mon, 27 Jun 2022 13:27:15 +0200 Subject: [PATCH 4/9] Refactor code to make it more explcitit that we only throw connection level errors if the deadline was exceeded and not if the user cancels the request manually --- Sources/AsyncHTTPClient/HTTPClient.swift | 2 +- Sources/AsyncHTTPClient/HTTPHandler.swift | 4 +- .../RequestBag+StateMachine.swift | 37 ++++++++++--------- Sources/AsyncHTTPClient/RequestBag.swift | 37 +++++++------------ .../HTTP1ClientChannelHandlerTests.swift | 2 +- .../HTTP2ClientRequestHandlerTests.swift | 2 +- .../HTTPConnectionPoolTests.swift | 2 +- .../RequestBagTests.swift | 8 ++-- 8 files changed, 42 insertions(+), 52 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 42e132535..1f08fb41d 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -606,7 +606,7 @@ public class HTTPClient { var deadlineSchedule: Scheduled? if let deadline = deadline { deadlineSchedule = taskEL.scheduleTask(deadline: deadline) { - requestBag.cancel(.deadlineExceeded) + requestBag.deadlineExceeded() } task.promise.futureResult.whenComplete { _ in diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index abf3443a1..c1ce39632 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -556,7 +556,7 @@ extension URL { } protocol HTTPClientTaskDelegate { - func cancel(_ reason: CancelationReason) + func cancel() } extension HTTPClient { @@ -618,7 +618,7 @@ extension HTTPClient { return self._taskDelegate } - taskDelegate?.cancel(.userInitiated) + taskDelegate?.cancel() } func succeed(promise: EventLoopPromise?, diff --git a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift index a3b9199d8..44126b27d 100644 --- a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift +++ b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift @@ -31,7 +31,8 @@ extension RequestBag { fileprivate enum State { case initialized case queued(HTTPRequestScheduler) - case canceledWhileQueued(CancelationReason) + /// if the deadline was exceeded while in the `.queued(_:)` state, we wait until the request pool fails the request with a potential more descriptive error message if a connection failure has occured while the request was queued. + case deadlineExceededWhileQueued case executing(HTTPRequestExecutor, RequestStreamState, ResponseStreamState) case finished(error: Error?) case redirected(HTTPRequestExecutor, Int, HTTPResponseHead, URL) @@ -96,7 +97,7 @@ extension RequestBag.StateMachine { case .initialized, .queued: self.state = .executing(executor, .initialized, .initialized) return true - case .finished(error: .some), .canceledWhileQueued: + case .finished(error: .some), .deadlineExceededWhileQueued: return false case .executing, .redirected, .finished(error: .none), .modifying: preconditionFailure("Invalid state: \(self.state)") @@ -111,7 +112,7 @@ extension RequestBag.StateMachine { mutating func resumeRequestBodyStream() -> ResumeProducingAction { switch self.state { - case .initialized, .queued, .canceledWhileQueued: + case .initialized, .queued, .deadlineExceededWhileQueued: preconditionFailure("A request stream can only be resumed, if the request was started") case .executing(let executor, .initialized, .initialized): @@ -151,7 +152,7 @@ extension RequestBag.StateMachine { mutating func pauseRequestBodyStream() { switch self.state { - case .initialized, .queued, .canceledWhileQueued: + case .initialized, .queued, .deadlineExceededWhileQueued: preconditionFailure("A request stream can only be paused, if the request was started") case .executing(let executor, let requestState, let responseState): switch requestState { @@ -186,7 +187,7 @@ extension RequestBag.StateMachine { mutating func writeNextRequestPart(_ part: IOData, taskEventLoop: EventLoop) -> WriteAction { switch self.state { - case .initialized, .queued, .canceledWhileQueued: + case .initialized, .queued, .deadlineExceededWhileQueued: preconditionFailure("Invalid state: \(self.state)") case .executing(let executor, let requestState, let responseState): switch requestState { @@ -232,7 +233,7 @@ extension RequestBag.StateMachine { mutating func finishRequestBodyStream(_ result: Result) -> FinishAction { switch self.state { - case .initialized, .queued, .canceledWhileQueued: + case .initialized, .queued, .deadlineExceededWhileQueued: preconditionFailure("Invalid state: \(self.state)") case .executing(let executor, let requestState, let responseState): switch requestState { @@ -283,7 +284,7 @@ extension RequestBag.StateMachine { /// - Returns: Whether the response should be forwarded to the delegate. Will be `false` if the request follows a redirect. mutating func receiveResponseHead(_ head: HTTPResponseHead) -> ReceiveResponseHeadAction { switch self.state { - case .initialized, .queued, .canceledWhileQueued: + case .initialized, .queued, .deadlineExceededWhileQueued: preconditionFailure("How can we receive a response, if the request hasn't started yet.") case .executing(let executor, let requestState, let responseState): guard case .initialized = responseState else { @@ -329,7 +330,7 @@ extension RequestBag.StateMachine { mutating func receiveResponseBodyParts(_ buffer: CircularBuffer) -> ReceiveResponseBodyAction { switch self.state { - case .initialized, .queued, .canceledWhileQueued: + case .initialized, .queued, .deadlineExceededWhileQueued: preconditionFailure("How can we receive a response body part, if the request hasn't started yet.") case .executing(_, _, .initialized): preconditionFailure("If we receive a response body, we must have received a head before") @@ -386,7 +387,7 @@ extension RequestBag.StateMachine { mutating func succeedRequest(_ newChunks: CircularBuffer?) -> ReceiveResponseEndAction { switch self.state { - case .initialized, .queued, .canceledWhileQueued: + case .initialized, .queued, .deadlineExceededWhileQueued: preconditionFailure("How can we receive a response body part, if the request hasn't started yet.") case .executing(_, _, .initialized): preconditionFailure("If we receive a response body, we must have received a head before") @@ -448,7 +449,7 @@ extension RequestBag.StateMachine { private mutating func failWithConsumptionError(_ error: Error) -> ConsumeAction { switch self.state { - case .initialized, .queued, .canceledWhileQueued: + case .initialized, .queued, .deadlineExceededWhileQueued: preconditionFailure("Invalid state: \(self.state)") case .executing(_, _, .initialized): preconditionFailure("Invalid state: Must have received response head, before this method is called for the first time") @@ -483,7 +484,7 @@ extension RequestBag.StateMachine { private mutating func consumeMoreBodyData() -> ConsumeAction { switch self.state { - case .initialized, .queued, .canceledWhileQueued: + case .initialized, .queued, .deadlineExceededWhileQueued: preconditionFailure("Invalid state: \(self.state)") case .executing(_, _, .initialized): @@ -533,18 +534,18 @@ extension RequestBag.StateMachine { } } - enum CancelAction { + enum DeadlineExceededAction { case cancelScheduler(HTTPRequestScheduler?) case fail(FailAction) } - mutating func cancel(_ reason: CancelationReason) -> CancelAction { + mutating func deadlineExceeded() -> DeadlineExceededAction { switch self.state { - case .queued(let queuer) where reason == .deadlineExceeded: - self.state = .canceledWhileQueued(reason) + case .queued(let queuer): + self.state = .deadlineExceededWhileQueued return .cancelScheduler(queuer) default: - return .fail(self.fail(reason.error)) + return .fail(self.fail(HTTPClientError.deadlineExceeded)) } } @@ -583,11 +584,11 @@ extension RequestBag.StateMachine { case .finished(.none): // An error occurred after the request has finished. Ignore... return .none - case .canceledWhileQueued(let reason): + case .deadlineExceededWhileQueued: // if we just get a `HTTPClientError.cancelled` we can use the orignal cancelation reason // to give a more descriptive error to the user. if (error as? HTTPClientError) == .cancelled { - return .failTask(reason.error, nil, nil) + return .failTask(HTTPClientError.deadlineExceeded, nil, nil) } // otherwise we already had an intermidate connection error which we should present to the user instead return .failTask(error, nil, nil) diff --git a/Sources/AsyncHTTPClient/RequestBag.swift b/Sources/AsyncHTTPClient/RequestBag.swift index 39e43a49a..3b698cf48 100644 --- a/Sources/AsyncHTTPClient/RequestBag.swift +++ b/Sources/AsyncHTTPClient/RequestBag.swift @@ -335,11 +335,10 @@ final class RequestBag { break } } - - private func cancel0(_ reason: CancelationReason) { + + func deadlineExceeded0() { self.task.eventLoop.assertInEventLoop() - - let action = self.state.cancel(reason) + let action = self.state.deadlineExceeded() switch action { case .cancelScheduler(let scheduler): @@ -348,18 +347,14 @@ final class RequestBag { self.executeFailAction0(failAction) } } -} - -enum CancelationReason { - case userInitiated - case deadlineExceeded - - var error: HTTPClientError { - switch self { - case .userInitiated: - return .cancelled - case .deadlineExceeded: - return .deadlineExceeded + + func deadlineExceeded() { + if self.task.eventLoop.inEventLoop { + self.deadlineExceeded0() + } else { + self.task.eventLoop.execute { + self.deadlineExceeded0() + } } } } @@ -487,13 +482,7 @@ extension RequestBag: HTTPExecutableRequest { } extension RequestBag: HTTPClientTaskDelegate { - func cancel(_ reason: CancelationReason) { - if self.task.eventLoop.inEventLoop { - self.cancel0(reason) - } else { - self.task.eventLoop.execute { - self.cancel0(reason) - } - } + func cancel() { + self.fail(HTTPClientError.cancelled) } } diff --git a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift index 20aadacde..f97580372 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift @@ -327,7 +327,7 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { XCTAssertNoThrow(try embedded.writeInbound(HTTPClientResponsePart.head(responseHead))) // canceling the request - requestBag.cancel(.userInitiated) + requestBag.cancel() XCTAssertThrowsError(try requestBag.task.futureResult.wait()) { XCTAssertEqual($0 as? HTTPClientError, .cancelled) } diff --git a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift index f25e33d25..e67529ad8 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift @@ -276,7 +276,7 @@ class HTTP2ClientRequestHandlerTests: XCTestCase { XCTAssertNoThrow(try embedded.writeInbound(HTTPClientResponsePart.head(responseHead))) // canceling the request - requestBag.cancel(.userInitiated) + requestBag.cancel() XCTAssertThrowsError(try requestBag.task.futureResult.wait()) { XCTAssertEqual($0 as? HTTPClientError, .cancelled) } diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPoolTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPoolTests.swift index 72d94b3de..60e5077ee 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPoolTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPoolTests.swift @@ -334,7 +334,7 @@ class HTTPConnectionPoolTests: XCTestCase { pool.executeRequest(requestBag) XCTAssertNoThrow(try eventLoop.scheduleTask(in: .seconds(1)) {}.futureResult.wait()) - requestBag.cancel(.userInitiated) + requestBag.cancel() XCTAssertThrowsError(try requestBag.task.futureResult.wait()) { XCTAssertEqual($0 as? HTTPClientError, .cancelled) diff --git a/Tests/AsyncHTTPClientTests/RequestBagTests.swift b/Tests/AsyncHTTPClientTests/RequestBagTests.swift index d27b7eb8e..c80f8846b 100644 --- a/Tests/AsyncHTTPClientTests/RequestBagTests.swift +++ b/Tests/AsyncHTTPClientTests/RequestBagTests.swift @@ -219,7 +219,7 @@ final class RequestBagTests: XCTestCase { XCTAssert(bag.eventLoop === embeddedEventLoop) let executor = MockRequestExecutor(eventLoop: embeddedEventLoop) - bag.cancel(.userInitiated) + bag.cancel() bag.willExecuteRequest(executor) XCTAssertTrue(executor.isCancelled, "The request bag, should call cancel immediately on the executor") @@ -261,7 +261,7 @@ final class RequestBagTests: XCTestCase { XCTAssertEqual(delegate.hitDidSendRequestHead, 1) XCTAssertEqual(delegate.hitDidSendRequest, 1) - bag.cancel(.userInitiated) + bag.cancel() XCTAssertTrue(executor.isCancelled, "The request bag, should call cancel immediately on the executor") XCTAssertThrowsError(try bag.task.futureResult.wait()) { @@ -295,7 +295,7 @@ final class RequestBagTests: XCTestCase { bag.requestWasQueued(queuer) XCTAssertEqual(queuer.hitCancelCount, 0) - bag.cancel(.userInitiated) + bag.cancel() XCTAssertEqual(queuer.hitCancelCount, 1) XCTAssertThrowsError(try bag.task.futureResult.wait()) { @@ -366,7 +366,7 @@ final class RequestBagTests: XCTestCase { // This simulates a race between the user cancelling the task (which invokes `RequestBag.cancel`) and the // call to `resumeRequestBodyStream` (which comes from the `Channel` event loop and so may have to hop. - bag.cancel(.userInitiated) + bag.cancel() bag.resumeRequestBodyStream() XCTAssertEqual(executor.isCancelled, true) From 5e575855576050753c764145ed6e0068222fea59 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Mon, 27 Jun 2022 13:28:36 +0200 Subject: [PATCH 5/9] SwiftFormat --- Sources/AsyncHTTPClient/RequestBag.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/AsyncHTTPClient/RequestBag.swift b/Sources/AsyncHTTPClient/RequestBag.swift index 3b698cf48..b80b75f8d 100644 --- a/Sources/AsyncHTTPClient/RequestBag.swift +++ b/Sources/AsyncHTTPClient/RequestBag.swift @@ -335,7 +335,7 @@ final class RequestBag { break } } - + func deadlineExceeded0() { self.task.eventLoop.assertInEventLoop() let action = self.state.deadlineExceeded() @@ -347,7 +347,7 @@ final class RequestBag { self.executeFailAction0(failAction) } } - + func deadlineExceeded() { if self.task.eventLoop.inEventLoop { self.deadlineExceeded0() From 8cbeeeafa5e04177842972e44a50170f77ed75a4 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 28 Jun 2022 14:03:31 +0200 Subject: [PATCH 6/9] fix typos --- Sources/AsyncHTTPClient/RequestBag+StateMachine.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift index 44126b27d..27e96312b 100644 --- a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift +++ b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift @@ -585,12 +585,12 @@ extension RequestBag.StateMachine { // An error occurred after the request has finished. Ignore... return .none case .deadlineExceededWhileQueued: - // if we just get a `HTTPClientError.cancelled` we can use the orignal cancelation reason + // if we just get a `HTTPClientError.cancelled` we can use the original cancellation reason // to give a more descriptive error to the user. if (error as? HTTPClientError) == .cancelled { return .failTask(HTTPClientError.deadlineExceeded, nil, nil) } - // otherwise we already had an intermidate connection error which we should present to the user instead + // otherwise we already had an intermediate connection error which we should present to the user instead return .failTask(error, nil, nil) case .finished(.some(_)): // this might happen, if the stream consumer has failed... let's just drop the data From c7640da4227db0da213c7d88709b13b4e390e715 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 28 Jun 2022 15:12:27 +0200 Subject: [PATCH 7/9] Explain where the state transition happens --- Sources/AsyncHTTPClient/RequestBag+StateMachine.swift | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift index 27e96312b..986f810bc 100644 --- a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift +++ b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift @@ -545,6 +545,8 @@ extension RequestBag.StateMachine { self.state = .deadlineExceededWhileQueued return .cancelScheduler(queuer) default: + /// if we are not in the queued state, we can fail early by just calling down to `self.fail(_:)` + /// which does the appropriate state transition for us. return .fail(self.fail(HTTPClientError.deadlineExceeded)) } } From 05b329e4f804645ba996598efbb0b538bc5e9afe Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Thu, 30 Jun 2022 10:14:27 +0200 Subject: [PATCH 8/9] fix review comments --- .../ConnectionPool/HTTPConnectionPool.swift | 2 - ...HTTPConnectionPool+HTTP1StateMachine.swift | 1 + ...HTTPConnectionPool+HTTP2StateMachine.swift | 1 + .../HTTPConnectionPool+StateMachine.swift | 1 - .../RequestBag+StateMachine.swift | 32 +++++++++++++--- Sources/AsyncHTTPClient/RequestBag.swift | 12 +++++- .../HTTPConnectionPool+StateTestUtils.swift | 2 - .../RequestBagTests.swift | 38 +++++++++++++++++++ 8 files changed, 76 insertions(+), 13 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift index 49e755733..5a0b2708e 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift @@ -147,8 +147,6 @@ final class HTTPConnectionPool { self.unlocked = Unlocked(connection: .none, request: .none) switch stateMachineAction.request { - case .cancelRequestTimeout(let requestID): - self.locked.request = .cancelRequestTimeout(requestID) case .executeRequest(let request, let connection, cancelTimeout: let cancelTimeout): if cancelTimeout { self.locked.request = .cancelRequestTimeout(request.id) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift index d1e705796..2cd667bb3 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift @@ -324,6 +324,7 @@ extension HTTPConnectionPool { mutating func cancelRequest(_ requestID: Request.ID) -> Action { // 1. check requests in queue if let request = self.requests.remove(requestID) { + // Use the last connection error to let the user know why the request was never scheduled let error = self.lastConnectFailure ?? HTTPClientError.cancelled return .init( request: .failRequest(request, error, cancelTimeout: true), diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift index f8054791d..d517d82e6 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift @@ -445,6 +445,7 @@ extension HTTPConnectionPool { mutating func cancelRequest(_ requestID: Request.ID) -> Action { // 1. check requests in queue if let request = self.requests.remove(requestID) { + // Use the last connection error to let the user know why the request was never scheduled let error = self.lastConnectFailure ?? HTTPClientError.cancelled return .init( request: .failRequest(request, error, cancelTimeout: true), diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift index 61e57941a..63f3e5a9a 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift @@ -61,7 +61,6 @@ extension HTTPConnectionPool { case failRequestsAndCancelTimeouts([Request], Error) case scheduleRequestTimeout(for: Request, on: EventLoop) - case cancelRequestTimeout(Request.ID) case none } diff --git a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift index 986f810bc..900c8404f 100644 --- a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift +++ b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift @@ -31,7 +31,9 @@ extension RequestBag { fileprivate enum State { case initialized case queued(HTTPRequestScheduler) - /// if the deadline was exceeded while in the `.queued(_:)` state, we wait until the request pool fails the request with a potential more descriptive error message if a connection failure has occured while the request was queued. + /// if the deadline was exceeded while in the `.queued(_:)` state, + /// we wait until the request pool fails the request with a potential more descriptive error message, + /// if a connection failure has occured while the request was queued. case deadlineExceededWhileQueued case executing(HTTPRequestExecutor, RequestStreamState, ResponseStreamState) case finished(error: Error?) @@ -91,14 +93,24 @@ extension RequestBag.StateMachine { self.state = .queued(scheduler) } + + enum WillExecuteRequestAction { + case cancelExecuter(HTTPRequestExecutor) + case failTaskAndCancelExecutor(Error, HTTPRequestExecutor) + case none + } - mutating func willExecuteRequest(_ executor: HTTPRequestExecutor) -> Bool { + mutating func willExecuteRequest(_ executor: HTTPRequestExecutor) -> WillExecuteRequestAction { switch self.state { case .initialized, .queued: self.state = .executing(executor, .initialized, .initialized) - return true - case .finished(error: .some), .deadlineExceededWhileQueued: - return false + return .none + case .deadlineExceededWhileQueued: + let error: Error = HTTPClientError.deadlineExceeded + self.state = .finished(error: error) + return .failTaskAndCancelExecutor(error, executor) + case .finished(error: .some): + return .cancelExecuter(executor) case .executing, .redirected, .finished(error: .none), .modifying: preconditionFailure("Invalid state: \(self.state)") } @@ -542,9 +554,17 @@ extension RequestBag.StateMachine { mutating func deadlineExceeded() -> DeadlineExceededAction { switch self.state { case .queued(let queuer): + /// We do not fail the request immediately because we want to give the scheduler a chance of throwing a better error message + /// We therefore depend on the scheduler failing the request after we cancel the request. self.state = .deadlineExceededWhileQueued return .cancelScheduler(queuer) - default: + + case .initialized, + .deadlineExceededWhileQueued, + .executing, + .finished, + .redirected, + .modifying: /// if we are not in the queued state, we can fail early by just calling down to `self.fail(_:)` /// which does the appropriate state transition for us. return .fail(self.fail(HTTPClientError.deadlineExceeded)) diff --git a/Sources/AsyncHTTPClient/RequestBag.swift b/Sources/AsyncHTTPClient/RequestBag.swift index b80b75f8d..4ec7004c1 100644 --- a/Sources/AsyncHTTPClient/RequestBag.swift +++ b/Sources/AsyncHTTPClient/RequestBag.swift @@ -81,8 +81,16 @@ final class RequestBag { private func willExecuteRequest0(_ executor: HTTPRequestExecutor) { self.task.eventLoop.assertInEventLoop() - if !self.state.willExecuteRequest(executor) { - return executor.cancelRequest(self) + let action = self.state.willExecuteRequest(executor) + switch action { + case .cancelExecuter(let executor): + executor.cancelRequest(self) + case .failTaskAndCancelExecutor(let error, let executor): + self.delegate.didReceiveError(task: self.task, error) + self.task.fail(with: error, delegateType: Delegate.self) + executor.cancelRequest(self) + case .none: + break } } diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+StateTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+StateTestUtils.swift index cb67837d7..0ffdeebd8 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+StateTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+StateTestUtils.swift @@ -126,8 +126,6 @@ extension HTTPConnectionPool.StateMachine.RequestAction: Equatable { return lhsReqs.elementsEqual(rhsReqs, by: { $0 == $1 }) case (.scheduleRequestTimeout(for: let lhsReq, on: let lhsEL), .scheduleRequestTimeout(for: let rhsReq, on: let rhsEL)): return lhsReq == rhsReq && lhsEL === rhsEL - case (.cancelRequestTimeout(let lhsReqID), .cancelRequestTimeout(let rhsReqID)): - return lhsReqID == rhsReqID case (.none, .none): return true default: diff --git a/Tests/AsyncHTTPClientTests/RequestBagTests.swift b/Tests/AsyncHTTPClientTests/RequestBagTests.swift index c80f8846b..82a445fb4 100644 --- a/Tests/AsyncHTTPClientTests/RequestBagTests.swift +++ b/Tests/AsyncHTTPClientTests/RequestBagTests.swift @@ -227,6 +227,44 @@ final class RequestBagTests: XCTestCase { XCTAssertEqual($0 as? HTTPClientError, .cancelled) } } + + func testDeadlineExceededFailsTaskEvenIfRaceBetweenCancelingSchedulerAndRequestStart() { + let embeddedEventLoop = EmbeddedEventLoop() + defer { XCTAssertNoThrow(try embeddedEventLoop.syncShutdownGracefully()) } + let logger = Logger(label: "test") + + var maybeRequest: HTTPClient.Request? + XCTAssertNoThrow(maybeRequest = try HTTPClient.Request(url: "https://swift.org")) + guard let request = maybeRequest else { return XCTFail("Expected to have a request") } + + let delegate = UploadCountingDelegate(eventLoop: embeddedEventLoop) + var maybeRequestBag: RequestBag? + XCTAssertNoThrow(maybeRequestBag = try RequestBag( + request: request, + eventLoopPreference: .delegate(on: embeddedEventLoop), + task: .init(eventLoop: embeddedEventLoop, logger: logger), + redirectHandler: nil, + connectionDeadline: .now() + .seconds(30), + requestOptions: .forTests(), + delegate: delegate + )) + guard let bag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag.") } + XCTAssert(bag.eventLoop === embeddedEventLoop) + + let queuer = MockTaskQueuer() + bag.requestWasQueued(queuer) + + let executor = MockRequestExecutor(eventLoop: embeddedEventLoop) + XCTAssertEqual(queuer.hitCancelCount, 0) + bag.deadlineExceeded() + XCTAssertEqual(queuer.hitCancelCount, 1) + + bag.willExecuteRequest(executor) + XCTAssertTrue(executor.isCancelled, "The request bag, should call cancel immediately on the executor") + XCTAssertThrowsError(try bag.task.futureResult.wait()) { + XCTAssertEqual($0 as? HTTPClientError, .deadlineExceeded) + } + } func testCancelFailsTaskAfterRequestIsSent() { let embeddedEventLoop = EmbeddedEventLoop() From 5b88cc8551be7aab851115bce827af352ba6247e Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Thu, 30 Jun 2022 13:51:41 +0200 Subject: [PATCH 9/9] run SwiftFormat and generate_linux_tests.rb --- .../AsyncHTTPClient/RequestBag+StateMachine.swift | 14 +++++++------- .../RequestBagTests+XCTest.swift | 1 + Tests/AsyncHTTPClientTests/RequestBagTests.swift | 4 ++-- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift index 900c8404f..a2a90749a 100644 --- a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift +++ b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift @@ -93,7 +93,7 @@ extension RequestBag.StateMachine { self.state = .queued(scheduler) } - + enum WillExecuteRequestAction { case cancelExecuter(HTTPRequestExecutor) case failTaskAndCancelExecutor(Error, HTTPRequestExecutor) @@ -558,13 +558,13 @@ extension RequestBag.StateMachine { /// We therefore depend on the scheduler failing the request after we cancel the request. self.state = .deadlineExceededWhileQueued return .cancelScheduler(queuer) - + case .initialized, - .deadlineExceededWhileQueued, - .executing, - .finished, - .redirected, - .modifying: + .deadlineExceededWhileQueued, + .executing, + .finished, + .redirected, + .modifying: /// if we are not in the queued state, we can fail early by just calling down to `self.fail(_:)` /// which does the appropriate state transition for us. return .fail(self.fail(HTTPClientError.deadlineExceeded)) diff --git a/Tests/AsyncHTTPClientTests/RequestBagTests+XCTest.swift b/Tests/AsyncHTTPClientTests/RequestBagTests+XCTest.swift index 74c68fd1f..19de474c2 100644 --- a/Tests/AsyncHTTPClientTests/RequestBagTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/RequestBagTests+XCTest.swift @@ -28,6 +28,7 @@ extension RequestBagTests { ("testWriteBackpressureWorks", testWriteBackpressureWorks), ("testTaskIsFailedIfWritingFails", testTaskIsFailedIfWritingFails), ("testCancelFailsTaskBeforeRequestIsSent", testCancelFailsTaskBeforeRequestIsSent), + ("testDeadlineExceededFailsTaskEvenIfRaceBetweenCancelingSchedulerAndRequestStart", testDeadlineExceededFailsTaskEvenIfRaceBetweenCancelingSchedulerAndRequestStart), ("testCancelFailsTaskAfterRequestIsSent", testCancelFailsTaskAfterRequestIsSent), ("testCancelFailsTaskWhenTaskIsQueued", testCancelFailsTaskWhenTaskIsQueued), ("testFailsTaskWhenTaskIsWaitingForMoreFromServer", testFailsTaskWhenTaskIsWaitingForMoreFromServer), diff --git a/Tests/AsyncHTTPClientTests/RequestBagTests.swift b/Tests/AsyncHTTPClientTests/RequestBagTests.swift index 82a445fb4..b896aca0a 100644 --- a/Tests/AsyncHTTPClientTests/RequestBagTests.swift +++ b/Tests/AsyncHTTPClientTests/RequestBagTests.swift @@ -227,7 +227,7 @@ final class RequestBagTests: XCTestCase { XCTAssertEqual($0 as? HTTPClientError, .cancelled) } } - + func testDeadlineExceededFailsTaskEvenIfRaceBetweenCancelingSchedulerAndRequestStart() { let embeddedEventLoop = EmbeddedEventLoop() defer { XCTAssertNoThrow(try embeddedEventLoop.syncShutdownGracefully()) } @@ -250,7 +250,7 @@ final class RequestBagTests: XCTestCase { )) guard let bag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag.") } XCTAssert(bag.eventLoop === embeddedEventLoop) - + let queuer = MockTaskQueuer() bag.requestWasQueued(queuer)