From e63c16ebe8af5dd268a549807fc71c24024f3983 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Tue, 16 Aug 2022 12:30:29 +0100 Subject: [PATCH 1/2] Accurately apply the connect timeout in async code Motivation We should apply the connect timeout to the complete set of connection attempts, rather than the request deadline. This allows users fine-grained control over how long we attempt to connect for. This is also the behaviour of our old-school interface. Modifications - Changed the connect deadline calculation for async/await to match that of the future-based code. - Added a connect timeout test. Result Connect timeouts are properly handled --- .../AsyncAwait/HTTPClient+execute.swift | 2 +- Sources/AsyncHTTPClient/HTTPClient.swift | 2 +- .../AsyncAwaitEndToEndTests.swift | 80 +++++++++++++++++++ 3 files changed, 82 insertions(+), 2 deletions(-) diff --git a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift index 043ad510b..318edbff9 100644 --- a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift +++ b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift @@ -134,7 +134,7 @@ extension HTTPClient { request: request, requestOptions: .init(idleReadTimeout: nil), logger: logger, - connectionDeadline: deadline, + connectionDeadline: .now() + (self.configuration.timeout.connectionCreationTimeout), preferredEventLoop: eventLoop, responseContinuation: continuation ) diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 3fbdb1366..9cf84a2cb 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -604,7 +604,7 @@ public class HTTPClient { eventLoopPreference: eventLoopPreference, task: task, redirectHandler: redirectHandler, - connectionDeadline: .now() + (self.configuration.timeout.connect ?? .seconds(10)), + connectionDeadline: .now() + (self.configuration.timeout.connectionCreationTimeout), requestOptions: .fromClientConfiguration(self.configuration), delegate: delegate ) diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift index 41a2d0798..68457d4ff 100644 --- a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift +++ b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift @@ -34,6 +34,27 @@ private func makeDefaultHTTPClient( } final class AsyncAwaitEndToEndTests: XCTestCase { + var clientGroup: EventLoopGroup! + var serverGroup: EventLoopGroup! + + override func setUp() { + XCTAssertNil(self.clientGroup) + XCTAssertNil(self.serverGroup) + + self.clientGroup = getDefaultEventLoopGroup(numberOfThreads: 1) + self.serverGroup = MultiThreadedEventLoopGroup(numberOfThreads: 1) + } + + override func tearDown() { + XCTAssertNotNil(self.clientGroup) + XCTAssertNoThrow(try self.clientGroup.syncShutdownGracefully()) + self.clientGroup = nil + + XCTAssertNotNil(self.serverGroup) + XCTAssertNoThrow(try self.serverGroup.syncShutdownGracefully()) + self.serverGroup = nil + } + func testSimpleGet() { #if compiler(>=5.5.2) && canImport(_Concurrency) guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { return } @@ -394,6 +415,65 @@ final class AsyncAwaitEndToEndTests: XCTestCase { #endif } + func testConnectTimeout() { + #if compiler(>=5.5.2) && canImport(_Concurrency) + guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { return } + XCTAsyncTest(timeout: 60) { + #if os(Linux) + // 198.51.100.254 is reserved for documentation only and therefore should not accept any TCP connection + let url = "http://198.51.100.254/get" + #else + // on macOS we can use the TCP backlog behaviour when the queue is full to simulate a non reachable server. + // this makes this test a bit more stable if `198.51.100.254` actually responds to connection attempt. + // The backlog behaviour on Linux can not be used to simulate a non-reachable server. + // Linux sends a `SYN/ACK` back even if the `backlog` queue is full as it has two queues. + // The second queue is not limit by `ChannelOptions.backlog` but by `/proc/sys/net/ipv4/tcp_max_syn_backlog`. + + let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) + defer { + XCTAssertNoThrow(try group.syncShutdownGracefully()) + } + + let serverChannel = try await ServerBootstrap(group: self.serverGroup) + .serverChannelOption(ChannelOptions.backlog, value: 1) + .serverChannelOption(ChannelOptions.autoRead, value: false) + .bind(host: "127.0.0.1", port: 0) + .get() + defer { + XCTAssertNoThrow(try serverChannel.close().wait()) + } + let port = serverChannel.localAddress!.port! + let firstClientChannel = try ClientBootstrap(group: self.serverGroup) + .connect(host: "127.0.0.1", port: port) + .wait() + defer { + XCTAssertNoThrow(try firstClientChannel.close().wait()) + } + let url = "http://localhost:\(port)/get" + #endif + + let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + configuration: .init(timeout: .init(connect: .milliseconds(100), read: .milliseconds(150)))) + + defer { + XCTAssertNoThrow(try httpClient.syncShutdown()) + } + + let request = HTTPClientRequest(url: url) + let start = NIODeadline.now() + await XCTAssertThrowsError(try await httpClient.execute(request, deadline: .now() + .seconds(30))) { + XCTAssertEqualTypeAndValue($0, HTTPClientError.connectTimeout) + let end = NIODeadline.now() + let duration = end - start + + // We give ourselves 10x slack in order to be confident that even on slow machines this assertion passes. + // It's 30x smaller than our other timeout though. + XCTAssertLessThan(duration, .seconds(1)) + } + } + #endif + } + func testSelfSignedCertificateIsRejectedWithCorrectErrorIfRequestDeadlineIsExceeded() { #if compiler(>=5.5.2) && canImport(_Concurrency) guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { return } From 14b8460d4e5b9987ef0cf1bc4c9eb7d44c6065ea Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Tue, 16 Aug 2022 13:00:16 +0100 Subject: [PATCH 2/2] Add missing Linux test --- Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests+XCTest.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests+XCTest.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests+XCTest.swift index fc6de5480..397071e08 100644 --- a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests+XCTest.swift @@ -38,6 +38,7 @@ extension AsyncAwaitEndToEndTests { ("testCanceling", testCanceling), ("testDeadline", testDeadline), ("testImmediateDeadline", testImmediateDeadline), + ("testConnectTimeout", testConnectTimeout), ("testSelfSignedCertificateIsRejectedWithCorrectErrorIfRequestDeadlineIsExceeded", testSelfSignedCertificateIsRejectedWithCorrectErrorIfRequestDeadlineIsExceeded), ("testInvalidURL", testInvalidURL), ("testRedirectChangesHostHeader", testRedirectChangesHostHeader),