From 910a1093e54ee049b1194c02a57f72517e9ab35d Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Sat, 13 Jun 2020 18:54:17 +0100 Subject: [PATCH 1/8] notify delegate about connect errors --- Sources/AsyncHTTPClient/HTTPClient.swift | 5 ++- .../HTTPClientTests.swift | 34 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index ee0f2613e..65689c34d 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -556,7 +556,10 @@ public class HTTPClient { } }.always { _ in setupComplete.succeed(()) - }.cascadeFailure(to: task.promise) + }.whenFailure { error in + delegate.didReceiveError(task: task, error) + task.promise.fail(error) + } return task } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 0e4e25d6e..718ae4219 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2005,4 +2005,38 @@ class HTTPClientTests: XCTestCase { self.defaultClient = nil // so it doesn't get shut down again. } + + func testConnectErrorPropagatedToDelegate() throws { + class TestDelegate: HTTPClientResponseDelegate { + typealias Response = Void + var error : Error? + func didFinishRequest(task: HTTPClient.Task) throws -> Void {} + func didReceiveError(task: HTTPClient.Task, _ error: Error) { + self.error = error + } + } + + let httpBin = HTTPBin() + let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) + defer { + XCTAssertNoThrow(try httpClient.syncShutdown()) + } + + let delegate = TestDelegate() + let request = try HTTPClient.Request(url: "http://localhost:\(httpBin.port)/get") + do { + try httpBin.shutdown() + _ = try httpClient.execute(request: request, delegate: delegate).wait() + XCTFail("Should fail") + } catch { + switch (error, delegate.error) { + case (_ as NIOConnectionError, _ as NIOConnectionError): + break + case (_ as NIOConnectionError, .none): + XCTFail("Delegate error is not \(error)") + default: + XCTFail("Unexpected error: \(error)") + } + } + } } From e2b46a271999e68938c384fa4b76d94cfee58609 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Sat, 13 Jun 2020 18:59:04 +0100 Subject: [PATCH 2/8] linux tests --- 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 7d61f805e..694564e33 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -109,6 +109,7 @@ extension HTTPClientTests { ("testNothingIsLoggedAtInfoOrHigher", testNothingIsLoggedAtInfoOrHigher), ("testAllMethodsLog", testAllMethodsLog), ("testClosingIdleConnectionsInPoolLogsInTheBackground", testClosingIdleConnectionsInPoolLogsInTheBackground), + ("testConnectErrorPropagatedToDelegate", testConnectErrorPropagatedToDelegate), ] } } From 1687c8900fd9d9582c4039ec1ce2ebaebc4d33f0 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Sat, 13 Jun 2020 19:00:31 +0100 Subject: [PATCH 3/8] formatting] --- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 718ae4219..abaffc6a6 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2009,8 +2009,8 @@ class HTTPClientTests: XCTestCase { func testConnectErrorPropagatedToDelegate() throws { class TestDelegate: HTTPClientResponseDelegate { typealias Response = Void - var error : Error? - func didFinishRequest(task: HTTPClient.Task) throws -> Void {} + var error: Error? + func didFinishRequest(task: HTTPClient.Task) throws {} func didReceiveError(task: HTTPClient.Task, _ error: Error) { self.error = error } From c282a937fe4527cbc6ea8a5a38866edfdd87ff66 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Tue, 16 Jun 2020 19:59:02 +0100 Subject: [PATCH 4/8] review fix --- Sources/AsyncHTTPClient/ConnectionPool.swift | 2 +- Sources/AsyncHTTPClient/HTTPClient.swift | 18 +++++---- .../HTTPClientInternalTests.swift | 40 +++++++++++++++++++ 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool.swift b/Sources/AsyncHTTPClient/ConnectionPool.swift index f18a30a98..27b651fe3 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool.swift @@ -562,7 +562,7 @@ class HTTP1ConnectionProvider { error = HTTPClient.NWErrorHandler.translateError(error) } #endif - return self.eventLoop.makeFailedFuture(error) + return eventLoop.makeFailedFuture(error) } } diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 8627392ab..cd1dd36db 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -511,6 +511,14 @@ public class HTTPClient { deadline: deadline, setupComplete: setupComplete.futureResult, logger: logger) + + let taskHandler = TaskHandler(task: task, + kind: request.kind, + delegate: delegate, + redirectHandler: redirectHandler, + ignoreUncleanSSLShutdown: self.configuration.ignoreUncleanSSLShutdown, + logger: logger) + connection.flatMap { connection -> EventLoopFuture in logger.debug("got connection for request", metadata: ["ahc-connection": "\(connection)", @@ -527,12 +535,6 @@ public class HTTPClient { } return future.flatMap { - let taskHandler = TaskHandler(task: task, - kind: request.kind, - delegate: delegate, - redirectHandler: redirectHandler, - ignoreUncleanSSLShutdown: self.configuration.ignoreUncleanSSLShutdown, - logger: logger) return channel.pipeline.addHandler(taskHandler) }.flatMap { task.setConnection(connection) @@ -557,7 +559,9 @@ public class HTTPClient { }.always { _ in setupComplete.succeed(()) }.whenFailure { error in - delegate.didReceiveError(task: task, error) + taskHandler.callOutToDelegateFireAndForget { task in + delegate.didReceiveError(task: task, error) + } task.promise.fail(error) } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index f8b0bd59c..5cf67530f 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -959,4 +959,44 @@ class HTTPClientInternalTests: XCTestCase { XCTAssertTrue(task.futureResult.eventLoop === el2) XCTAssertNoThrow(try task.wait()) } + + func testConnectErrorCalloutOnCorrectEL() throws { + class TestDelegate: HTTPClientResponseDelegate { + typealias Response = Void + + let expectedEL: EventLoop + var receivedError: Bool = false + + init(expectedEL: EventLoop) { + self.expectedEL = expectedEL + } + + func didFinishRequest(task: HTTPClient.Task) throws -> Void { + } + + func didReceiveError(task: HTTPClient.Task, _ error: Error) { + self.receivedError = true + XCTAssertTrue(self.expectedEL.inEventLoop) + } + } + + let elg = getDefaultEventLoopGroup(numberOfThreads: 2) + let el1 = elg.next() + let el2 = elg.next() + + let httpBin = HTTPBin(refusesConnections: true) + let client = HTTPClient(eventLoopGroupProvider: .shared(elg)) + + defer { + XCTAssertNoThrow(try client.syncShutdown()) + XCTAssertNoThrow(try elg.syncShutdownGracefully()) + } + + let request = try HTTPClient.Request(url: "http://localhost:\(httpBin.port)/get") + let delegate = TestDelegate(expectedEL: el1) + XCTAssertNoThrow(try httpBin.shutdown()) + let task = client.execute(request: request, delegate: delegate, eventLoop: .init(.testOnly_exact(channelOn: el2, delegateOn: el1))) + XCTAssertThrowsError(try task.wait()) + XCTAssertTrue(delegate.receivedError) + } } From 49f742b3646326abf2a07ef9c83f911aa9989315 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Tue, 16 Jun 2020 20:01:15 +0100 Subject: [PATCH 5/8] swiftformat and linux tests --- Sources/AsyncHTTPClient/HTTPClient.swift | 10 +++++----- .../HTTPClientInternalTests+XCTest.swift | 1 + .../AsyncHTTPClientTests/HTTPClientInternalTests.swift | 3 +-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index cd1dd36db..54a708852 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -513,11 +513,11 @@ public class HTTPClient { logger: logger) let taskHandler = TaskHandler(task: task, - kind: request.kind, - delegate: delegate, - redirectHandler: redirectHandler, - ignoreUncleanSSLShutdown: self.configuration.ignoreUncleanSSLShutdown, - logger: logger) + kind: request.kind, + delegate: delegate, + redirectHandler: redirectHandler, + ignoreUncleanSSLShutdown: self.configuration.ignoreUncleanSSLShutdown, + logger: logger) connection.flatMap { connection -> EventLoopFuture in logger.debug("got connection for request", diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift index 9a3e11b50..c6a54c9d8 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift @@ -43,6 +43,7 @@ extension HTTPClientInternalTests { ("testUploadStreamingIsCalledOnTaskEL", testUploadStreamingIsCalledOnTaskEL), ("testWeCanActuallyExactlySetTheEventLoops", testWeCanActuallyExactlySetTheEventLoops), ("testTaskPromiseBoundToEL", testTaskPromiseBoundToEL), + ("testConnectErrorCalloutOnCorrectEL", testConnectErrorCalloutOnCorrectEL), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index 5cf67530f..21e244712 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -971,8 +971,7 @@ class HTTPClientInternalTests: XCTestCase { self.expectedEL = expectedEL } - func didFinishRequest(task: HTTPClient.Task) throws -> Void { - } + func didFinishRequest(task: HTTPClient.Task) throws {} func didReceiveError(task: HTTPClient.Task, _ error: Error) { self.receivedError = true From 747b8a9721c29a57b9a257dcbafc25403a6e96a6 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Fri, 19 Jun 2020 10:45:54 +0100 Subject: [PATCH 6/8] review fix --- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 70bcaaeab..9cd25e654 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2026,11 +2026,9 @@ class HTTPClientTests: XCTestCase { let delegate = TestDelegate() let request = try HTTPClient.Request(url: "http://localhost:\(httpBin.port)/get") - do { - try httpBin.shutdown() - _ = try httpClient.execute(request: request, delegate: delegate).wait() - XCTFail("Should fail") - } catch { + + XCTAssertNoThrow(try httpBin.shutdown()) + XCTAssertThrowsError(try httpClient.execute(request: request, delegate: delegate).wait()) { error in switch (error, delegate.error) { case (_ as NIOConnectionError, _ as NIOConnectionError): break From 7696be3481eb1a6fa8684cf04c38a8870d7d5b30 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Tue, 23 Jun 2020 14:35:18 +0100 Subject: [PATCH 7/8] refactor test a bit --- .../HTTPClientTests.swift | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index f25a4f651..eaf4c6564 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2384,25 +2384,20 @@ class HTTPClientTests: XCTestCase { } } - let httpBin = HTTPBin() - let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) + let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + configuration: .init(timeout: .init(connect: .milliseconds(10)))) + defer { XCTAssertNoThrow(try httpClient.syncShutdown()) } + // This must throw as 198.51.100.254 is reserved for documentation only + let request = try HTTPClient.Request(url: "http://198.51.100.254:65535/get") let delegate = TestDelegate() - let request = try HTTPClient.Request(url: "http://localhost:\(httpBin.port)/get") - XCTAssertNoThrow(try httpBin.shutdown()) XCTAssertThrowsError(try httpClient.execute(request: request, delegate: delegate).wait()) { error in - switch (error, delegate.error) { - case (_ as NIOConnectionError, _ as NIOConnectionError): - break - case (_ as NIOConnectionError, .none): - XCTFail("Delegate error is not \(error)") - default: - XCTFail("Unexpected error: \(error)") - } + XCTAssertEqual(.connectTimeout(.milliseconds(10)), error as? ChannelError) + XCTAssertEqual(.connectTimeout(.milliseconds(10)), delegate.error as? ChannelError) } } From ffeb515c98413d942ba88d250619630050497003 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Tue, 23 Jun 2020 14:53:38 +0100 Subject: [PATCH 8/8] rename variable --- Sources/AsyncHTTPClient/ConnectionPool.swift | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool.swift b/Sources/AsyncHTTPClient/ConnectionPool.swift index eeb709286..5e44da8a9 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool.swift @@ -513,13 +513,13 @@ class HTTP1ConnectionProvider { } private func makeChannel(preference: HTTPClient.EventLoopPreference) -> EventLoopFuture { - let eventLoop = preference.bestEventLoop ?? self.eventLoop + let channelEventLoop = preference.bestEventLoop ?? self.eventLoop let requiresTLS = self.key.scheme.requiresTLS let bootstrap: NIOClientTCPBootstrap do { - bootstrap = try NIOClientTCPBootstrap.makeHTTPClientBootstrapBase(on: eventLoop, host: self.key.host, port: self.key.port, requiresTLS: requiresTLS, configuration: self.configuration) + bootstrap = try NIOClientTCPBootstrap.makeHTTPClientBootstrapBase(on: channelEventLoop, host: self.key.host, port: self.key.port, requiresTLS: requiresTLS, configuration: self.configuration) } catch { - return eventLoop.makeFailedFuture(error) + return channelEventLoop.makeFailedFuture(error) } let channel: EventLoopFuture @@ -564,7 +564,7 @@ class HTTP1ConnectionProvider { error = HTTPClient.NWErrorHandler.translateError(error) } #endif - return eventLoop.makeFailedFuture(error) + return channelEventLoop.makeFailedFuture(error) } }