From d5b61f2652b8868ac435eb8861118e70f76c4e54 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Fri, 19 Jun 2020 11:31:40 +0100 Subject: [PATCH 1/5] fix missing connect timeout and make tests safer --- Sources/AsyncHTTPClient/Utils.swift | 7 ++++- .../HTTPClientNIOTSTests.swift | 31 ++++++++++--------- .../HTTPClientTests.swift | 23 ++++++++++++++ 3 files changed, 45 insertions(+), 16 deletions(-) diff --git a/Sources/AsyncHTTPClient/Utils.swift b/Sources/AsyncHTTPClient/Utils.swift index 7da957b07..dd30e0393 100644 --- a/Sources/AsyncHTTPClient/Utils.swift +++ b/Sources/AsyncHTTPClient/Utils.swift @@ -79,7 +79,7 @@ extension NIOClientTCPBootstrap { requiresTLS: Bool, configuration: HTTPClient.Configuration ) throws -> NIOClientTCPBootstrap { - let bootstrap: NIOClientTCPBootstrap + var bootstrap: NIOClientTCPBootstrap #if canImport(Network) // if eventLoop is compatible with NIOTransportServices create a NIOTSConnectionBootstrap if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *), let tsBootstrap = NIOTSConnectionBootstrap(validatingGroup: eventLoop) { @@ -106,10 +106,15 @@ extension NIOClientTCPBootstrap { } #endif + if let timeout = configuration.timeout.connect { + bootstrap = bootstrap.connectTimeout(timeout) + } + // don't enable TLS if we have a proxy, this will be enabled later on if requiresTLS, configuration.proxy == nil { return bootstrap.enableTLS() } + return bootstrap } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift index d37ecae2a..a3213751b 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift @@ -74,19 +74,25 @@ class HTTPClientNIOTSTests: XCTestCase { func testConnectionFailError() { guard isTestingNIOTS() else { return } let httpBin = HTTPBin(ssl: true) - let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) + let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + configuration: .init(timeout: .init(connect: .milliseconds(100), + read: .milliseconds(100)))) + defer { XCTAssertNoThrow(try httpClient.syncShutdown(requiresCleanClose: true)) } + let port = httpBin.port XCTAssertNoThrow(try httpBin.shutdown()) - do { - _ = try httpClient.get(url: "https://localhost:\(port)/get").wait() - XCTFail("This should have failed") - } catch ChannelError.connectTimeout { - } catch { - XCTFail("Error should have been ChannelError.connectTimeout not \(type(of: error))") + XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(port)/get").wait()) { error in + switch error { + case ChannelError.connectTimeout(let timeout): + XCTAssertLessThanOrEqual(timeout, .milliseconds(150)) + break + default: + XCTFail("Unexpected error: \(error)") + } } } @@ -103,14 +109,9 @@ class HTTPClientNIOTSTests: XCTestCase { XCTAssertNoThrow(try httpBin.shutdown()) } - do { - _ = try httpClient.get(url: "https://localhost:\(httpBin.port)/get").wait() - XCTFail("This should have failed") - } catch let error as HTTPClient.NWTLSError { - XCTAssertEqual(error.status, errSSLHandshakeFail) - } catch { - XCTFail("Error should have been NWTLSError not \(type(of: error))") - } + XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/get").wait()) { error in + XCTAssertEqual((error as? HTTPClient.NWTLSError)?.status, errSSLHandshakeFail) + } #endif } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index ec8eb92fe..6f0c49ba3 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -422,6 +422,29 @@ class HTTPClientTests: XCTestCase { } } + func testConnectTimeout() throws { + let httpBin = HTTPBin(ssl: false) + let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + configuration: .init(timeout: .init(connect: .milliseconds(100), read: .milliseconds(150)))) + + defer { + XCTAssertNoThrow(try httpClient.syncShutdown()) + } + + let port = httpBin.port + XCTAssertNoThrow(try httpBin.shutdown()) + + XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(port)/get").wait()) { error in + switch error { + case ChannelError.connectTimeout(let timeout): + XCTAssertLessThanOrEqual(timeout, .milliseconds(150)) + break + default: + XCTFail("Unexpected error: \(error)") + } + } + } + func testDeadline() throws { XCTAssertThrowsError(try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "wait", deadline: .now() + .milliseconds(150)).wait(), "Should fail") { error in guard case let error = error as? HTTPClientError, error == .readTimeout else { From f099d6fcac8fbb2890ef4b5914a98d2b4d416c5a Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Fri, 19 Jun 2020 11:39:36 +0100 Subject: [PATCH 2/5] swiftformat and linux tests --- Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift | 7 +++---- Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift | 1 + Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 3 +-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift index a3213751b..63ea7cd86 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift @@ -89,7 +89,6 @@ class HTTPClientNIOTSTests: XCTestCase { switch error { case ChannelError.connectTimeout(let timeout): XCTAssertLessThanOrEqual(timeout, .milliseconds(150)) - break default: XCTFail("Unexpected error: \(error)") } @@ -109,9 +108,9 @@ class HTTPClientNIOTSTests: XCTestCase { XCTAssertNoThrow(try httpBin.shutdown()) } - XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/get").wait()) { error in - XCTAssertEqual((error as? HTTPClient.NWTLSError)?.status, errSSLHandshakeFail) - } + XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/get").wait()) { error in + XCTAssertEqual((error as? HTTPClient.NWTLSError)?.status, errSSLHandshakeFail) + } #endif } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 83efb6f0d..dc82aba4b 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -46,6 +46,7 @@ extension HTTPClientTests { ("testStreaming", testStreaming), ("testRemoteClose", testRemoteClose), ("testReadTimeout", testReadTimeout), + ("testConnectTimeout", testConnectTimeout), ("testDeadline", testDeadline), ("testCancel", testCancel), ("testStressCancel", testStressCancel), diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 6f0c49ba3..07e56ecd3 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -425,7 +425,7 @@ class HTTPClientTests: XCTestCase { func testConnectTimeout() throws { let httpBin = HTTPBin(ssl: false) let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), - configuration: .init(timeout: .init(connect: .milliseconds(100), read: .milliseconds(150)))) + configuration: .init(timeout: .init(connect: .milliseconds(100), read: .milliseconds(150)))) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) @@ -438,7 +438,6 @@ class HTTPClientTests: XCTestCase { switch error { case ChannelError.connectTimeout(let timeout): XCTAssertLessThanOrEqual(timeout, .milliseconds(150)) - break default: XCTFail("Unexpected error: \(error)") } From ee1d5a81e5930bc5ece249d2f7dc525e6261df2a Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Fri, 19 Jun 2020 12:15:42 +0100 Subject: [PATCH 3/5] fix timeout test --- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 07e56ecd3..da353fb3d 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -423,7 +423,6 @@ class HTTPClientTests: XCTestCase { } func testConnectTimeout() throws { - let httpBin = HTTPBin(ssl: false) let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), configuration: .init(timeout: .init(connect: .milliseconds(100), read: .milliseconds(150)))) @@ -431,10 +430,8 @@ class HTTPClientTests: XCTestCase { XCTAssertNoThrow(try httpClient.syncShutdown()) } - let port = httpBin.port - XCTAssertNoThrow(try httpBin.shutdown()) - - XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(port)/get").wait()) { error in + // This must throw as 198.51.100.254 is reserved for documentation only + XCTAssertThrowsError(try httpClient.get(url: "http://198.51.100.254:65535/get").wait()) { error in switch error { case ChannelError.connectTimeout(let timeout): XCTAssertLessThanOrEqual(timeout, .milliseconds(150)) From 8eeae12c927ad1da41fd20ea1df5282ad4b9e22e Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Fri, 19 Jun 2020 15:02:04 +0100 Subject: [PATCH 4/5] speedup another test --- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index da353fb3d..5c80b8bb5 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -1680,7 +1680,7 @@ class HTTPClientTests: XCTestCase { } func testAvoidLeakingTLSHandshakeCompletionPromise() { - let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) + let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), configuration: .init(timeout: .init(connect: .milliseconds(100)))) let localHTTPBin = HTTPBin() let port = localHTTPBin.port XCTAssertNoThrow(try localHTTPBin.shutdown()) From 5a438926fc63b4d5c35983ff7943329447c9ca7f Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Mon, 22 Jun 2020 15:46:31 +0100 Subject: [PATCH 5/5] make tests safer --- Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift | 7 +------ Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 7 +------ 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift index 63ea7cd86..ce71c5fab 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift @@ -86,12 +86,7 @@ class HTTPClientNIOTSTests: XCTestCase { XCTAssertNoThrow(try httpBin.shutdown()) XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(port)/get").wait()) { error in - switch error { - case ChannelError.connectTimeout(let timeout): - XCTAssertLessThanOrEqual(timeout, .milliseconds(150)) - default: - XCTFail("Unexpected error: \(error)") - } + XCTAssertEqual(.connectTimeout(.milliseconds(100)), error as? ChannelError) } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 5c80b8bb5..573096575 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -432,12 +432,7 @@ class HTTPClientTests: XCTestCase { // This must throw as 198.51.100.254 is reserved for documentation only XCTAssertThrowsError(try httpClient.get(url: "http://198.51.100.254:65535/get").wait()) { error in - switch error { - case ChannelError.connectTimeout(let timeout): - XCTAssertLessThanOrEqual(timeout, .milliseconds(150)) - default: - XCTFail("Unexpected error: \(error)") - } + XCTAssertEqual(.connectTimeout(.milliseconds(100)), error as? ChannelError) } }