Code review: We use a deadline now for connection creation.

fabianfett · fabianfett · commit 7a3d7b4893a9 · 2021-06-23T16:24:20.000+02:00
diff --git a/Sources/AsyncHTTPClient/ConnectionPool.swift b/Sources/AsyncHTTPClient/ConnectionPool.swift
@@ -455,7 +455,13 @@ class HTTP1ConnectionProvider {
                              logger: Logger) -> EventLoopFuture<Channel> {
         let connectionID = HTTPConnectionPool.Connection.ID.globalGenerator.next()
         let eventLoop = preference.bestEventLoop ?? self.eventLoop
-        return self.factory.makeChannel(connectionID: connectionID, eventLoop: eventLoop, logger: logger).flatMapThrowing {
+        let deadline = NIODeadline.now() + (self.configuration.timeout.connect ?? .seconds(10))
+        return self.factory.makeChannel(
+            connectionID: connectionID,
+            deadline: deadline,
+            eventLoop: eventLoop,
+            logger: logger
+        ).flatMapThrowing {
             (channel, _) -> Channel in
 
             // add the http1.1 channel handlers
diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift
@@ -29,74 +29,83 @@ extension HTTPConnectionPool {
     }
 
     final class ConnectionFactory {
-        struct Timeouts {
-            /// the `TimeAmount` waited before the TCP connection creation is failed with a timeout error
-            var connect: TimeAmount = .seconds(10)
-
-            /// the `TimeAmount` waited before the SOCKS proxy connection creation is failed with a timeout error
-            var socksProxyHandshake: TimeAmount = .seconds(10)
-
-            /// the `TimeAmount` waited before the HTTP proxy connection creation is failed with a timeout error
-            var httpProxyHandshake: TimeAmount = .seconds(10)
-
-            /// the `TimeAmount` waited before we the TLS handshake is failed with a timeout error
-            var tlsHandshake: TimeAmount = .seconds(10)
-        }
-
         let key: ConnectionPool.Key
         let clientConfiguration: HTTPClient.Configuration
         let tlsConfiguration: TLSConfiguration
         let sslContextCache: SSLContextCache
-        let timeouts: Timeouts
 
         init(key: ConnectionPool.Key,
              tlsConfiguration: TLSConfiguration?,
              clientConfiguration: HTTPClient.Configuration,
-             sslContextCache: SSLContextCache,
-             timeouts: Timeouts = .init()) {
+             sslContextCache: SSLContextCache) {
             self.key = key
             self.clientConfiguration = clientConfiguration
             self.sslContextCache = sslContextCache
             self.tlsConfiguration = tlsConfiguration ?? clientConfiguration.tlsConfiguration ?? .forClient()
-
-            var timeouts = timeouts
-            if let connect = clientConfiguration.timeout.connect {
-                timeouts.connect = connect
-            }
-            self.timeouts = timeouts
         }
     }
 }
 
 extension HTTPConnectionPool.ConnectionFactory {
-    func makeChannel(connectionID: HTTPConnectionPool.Connection.ID, eventLoop: EventLoop, logger: Logger) -> EventLoopFuture<(Channel, HTTPVersion)> {
+    func makeChannel(
+        connectionID: HTTPConnectionPool.Connection.ID,
+        deadline: NIODeadline,
+        eventLoop: EventLoop,
+        logger: Logger
+    ) -> EventLoopFuture<(Channel, HTTPVersion)> {
+        let channelFuture: EventLoopFuture<(Channel, HTTPVersion)>
+        
         if self.key.scheme.isProxyable, let proxy = self.clientConfiguration.proxy {
             switch proxy.type {
             case .socks:
-                return self.makeSOCKSProxyChannel(proxy, connectionID: connectionID, eventLoop: eventLoop, logger: logger)
+                channelFuture = self.makeSOCKSProxyChannel(
+                    proxy,
+                    connectionID: connectionID,
+                    deadline: deadline,
+                    eventLoop: eventLoop,
+                    logger: logger)
             case .http:
-                return self.makeHTTPProxyChannel(proxy, connectionID: connectionID, eventLoop: eventLoop, logger: logger)
+                channelFuture = self.makeHTTPProxyChannel(
+                    proxy,
+                    connectionID: connectionID,
+                    deadline: deadline,
+                    eventLoop: eventLoop,
+                    logger: logger)
             }
         } else {
-            return self.makeNonProxiedChannel(eventLoop: eventLoop, logger: logger)
+            channelFuture = self.makeNonProxiedChannel(deadline: deadline, eventLoop: eventLoop, logger: logger)
+        }
+        
+        // let's map `ChannelError.connectTimeout` into a `HTTPClientError.connectTimeout`
+        return channelFuture.flatMapErrorThrowing { error throws -> (Channel, HTTPVersion) in
+            switch error {
+            case ChannelError.connectTimeout:
+                throw HTTPClientError.connectTimeout
+            default:
+                throw error
+            }
         }
     }
 
-    private func makeNonProxiedChannel(eventLoop: EventLoop, logger: Logger) -> EventLoopFuture<(Channel, HTTPVersion)> {
+    private func makeNonProxiedChannel(
+        deadline: NIODeadline,
+        eventLoop: EventLoop,
+        logger: Logger
+    ) -> EventLoopFuture<(Channel, HTTPVersion)> {
         switch self.key.scheme {
         case .http, .http_unix, .unix:
-            return self.makePlainChannel(eventLoop: eventLoop).map { ($0, .http1_1) }
+            return self.makePlainChannel(deadline: deadline, eventLoop: eventLoop).map { ($0, .http1_1) }
         case .https, .https_unix:
-            return self.makeTLSChannel(eventLoop: eventLoop, logger: logger).flatMapThrowing {
+            return self.makeTLSChannel(deadline: deadline, eventLoop: eventLoop, logger: logger).flatMapThrowing {
                 channel, negotiated in
 
                 (channel, try self.matchALPNToHTTPVersion(negotiated))
             }
         }
     }
 
-    private func makePlainChannel(eventLoop: EventLoop) -> EventLoopFuture<Channel> {
-        let bootstrap = self.makePlainBootstrap(eventLoop: eventLoop)
+    private func makePlainChannel(deadline: NIODeadline, eventLoop: EventLoop) -> EventLoopFuture<Channel> {
+        let bootstrap = self.makePlainBootstrap(deadline: deadline, eventLoop: eventLoop)
 
         switch self.key.scheme {
         case .http:
@@ -111,21 +120,22 @@ extension HTTPConnectionPool.ConnectionFactory {
     private func makeHTTPProxyChannel(
         _ proxy: HTTPClient.Configuration.Proxy,
         connectionID: HTTPConnectionPool.Connection.ID,
+        deadline: NIODeadline,
         eventLoop: EventLoop,
         logger: Logger
     ) -> EventLoopFuture<(Channel, HTTPVersion)> {
         // A proxy connection starts with a plain text connection to the proxy server. After
         // the connection has been established with the proxy server, the connection might be
         // upgraded to TLS before we send our first request.
-        let bootstrap = self.makePlainBootstrap(eventLoop: eventLoop)
+        let bootstrap = self.makePlainBootstrap(deadline: deadline, eventLoop: eventLoop)
         return bootstrap.connect(host: proxy.host, port: proxy.port).flatMap { channel in
             let encoder = HTTPRequestEncoder()
             let decoder = ByteToMessageHandler(HTTPResponseDecoder(leftOverBytesStrategy: .dropBytes))
             let proxyHandler = HTTP1ProxyConnectHandler(
                 targetHost: self.key.host,
                 targetPort: self.key.port,
                 proxyAuthorization: proxy.authorization,
-                deadline: .now() + self.timeouts.httpProxyHandshake
+                deadline: deadline
             )
 
             do {
@@ -145,24 +155,25 @@ extension HTTPConnectionPool.ConnectionFactory {
                     }
                 }
             }.flatMap {
-                self.setupTLSInProxyConnectionIfNeeded(channel, logger: logger)
+                self.setupTLSInProxyConnectionIfNeeded(channel, deadline: deadline, logger: logger)
             }
         }
     }
 
     private func makeSOCKSProxyChannel(
         _ proxy: HTTPClient.Configuration.Proxy,
         connectionID: HTTPConnectionPool.Connection.ID,
+        deadline: NIODeadline,
         eventLoop: EventLoop,
         logger: Logger
     ) -> EventLoopFuture<(Channel, HTTPVersion)> {
         // A proxy connection starts with a plain text connection to the proxy server. After
         // the connection has been established with the proxy server, the connection might be
         // upgraded to TLS before we send our first request.
-        let bootstrap = self.makePlainBootstrap(eventLoop: eventLoop)
+        let bootstrap = self.makePlainBootstrap(deadline: deadline, eventLoop: eventLoop)
         return bootstrap.connect(host: proxy.host, port: proxy.port).flatMap { channel in
             let socksConnectHandler = SOCKSClientHandler(targetAddress: .domain(self.key.host, port: self.key.port))
-            let socksEventHandler = SOCKSEventsHandler(deadline: .now() + self.timeouts.socksProxyHandshake)
+            let socksEventHandler = SOCKSEventsHandler(deadline: deadline)
 
             do {
                 try channel.pipeline.syncOperations.addHandler(socksConnectHandler)
@@ -178,12 +189,16 @@ extension HTTPConnectionPool.ConnectionFactory {
                     channel.pipeline.removeHandler(socksConnectHandler)
                 }
             }.flatMap {
-                self.setupTLSInProxyConnectionIfNeeded(channel, logger: logger)
+                self.setupTLSInProxyConnectionIfNeeded(channel, deadline: deadline, logger: logger)
             }
         }
     }
 
-    private func setupTLSInProxyConnectionIfNeeded(_ channel: Channel, logger: Logger) -> EventLoopFuture<(Channel, HTTPVersion)> {
+    private func setupTLSInProxyConnectionIfNeeded(
+        _ channel: Channel,
+        deadline: NIODeadline,
+        logger: Logger
+    ) -> EventLoopFuture<(Channel, HTTPVersion)> {
         switch self.key.scheme {
         case .unix, .http_unix, .https_unix:
             preconditionFailure("Unexpected scheme. Not supported for proxy!")
@@ -193,7 +208,7 @@ extension HTTPConnectionPool.ConnectionFactory {
             var tlsConfig = self.tlsConfiguration
             // since we can support h2, we need to advertise this in alpn
             tlsConfig.applicationProtocols = ["http/1.1" /* , "h2" */ ]
-            let tlsEventHandler = TLSEventsHandler(deadline: .now() + self.timeouts.tlsHandshake)
+            let tlsEventHandler = TLSEventsHandler(deadline: deadline)
 
             let sslContextFuture = self.sslContextCache.sslContext(
                 tlsConfiguration: tlsConfig,
@@ -224,11 +239,11 @@ extension HTTPConnectionPool.ConnectionFactory {
         }
     }
 
-    private func makePlainBootstrap(eventLoop: EventLoop) -> NIOClientTCPBootstrapProtocol {
+    private func makePlainBootstrap(deadline: NIODeadline, eventLoop: EventLoop) -> NIOClientTCPBootstrapProtocol {
         #if canImport(Network)
             if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *), let tsBootstrap = NIOTSConnectionBootstrap(validatingGroup: eventLoop) {
                 return tsBootstrap
-                    .connectTimeout(self.timeouts.connect)
+                    .connectTimeout(deadline - NIODeadline.now())
                     .channelInitializer { channel in
                         do {
                             try channel.pipeline.syncOperations.addHandler(HTTPClient.NWErrorHandler())
@@ -242,14 +257,15 @@ extension HTTPConnectionPool.ConnectionFactory {
 
         if let nioBootstrap = ClientBootstrap(validatingGroup: eventLoop) {
             return nioBootstrap
-                .connectTimeout(self.timeouts.connect)
+                .connectTimeout(deadline - NIODeadline.now())
         }
 
         preconditionFailure("No matching bootstrap found")
     }
 
-    private func makeTLSChannel(eventLoop: EventLoop, logger: Logger) -> EventLoopFuture<(Channel, String?)> {
+    private func makeTLSChannel(deadline: NIODeadline, eventLoop: EventLoop, logger: Logger) -> EventLoopFuture<(Channel, String?)> {
         let bootstrapFuture = self.makeTLSBootstrap(
+            deadline: deadline,
             eventLoop: eventLoop,
             logger: logger
         )
@@ -285,7 +301,7 @@ extension HTTPConnectionPool.ConnectionFactory {
         return channelFuture
     }
 
-    private func makeTLSBootstrap(eventLoop: EventLoop, logger: Logger)
+    private func makeTLSBootstrap(deadline: NIODeadline, eventLoop: EventLoop, logger: Logger)
         -> EventLoopFuture<NIOClientTCPBootstrapProtocol> {
         // since we can support h2, we need to advertise this in alpn
         var tlsConfig = self.tlsConfiguration
@@ -298,7 +314,7 @@ extension HTTPConnectionPool.ConnectionFactory {
                     options -> NIOClientTCPBootstrapProtocol in
 
                     tsBootstrap
-                        .connectTimeout(self.timeouts.connect)
+                        .connectTimeout(deadline - NIODeadline.now())
                         .tlsOptions(options)
                         .channelInitializer { channel in
                             do {
@@ -327,7 +343,7 @@ extension HTTPConnectionPool.ConnectionFactory {
         )
 
         let bootstrap = ClientBootstrap(group: eventLoop)
-            .connectTimeout(self.timeouts.connect)
+            .connectTimeout(deadline - NIODeadline.now())
             .channelInitializer { channel in
                 sslContextFuture.flatMap { (sslContext) -> EventLoopFuture<Void> in
                     do {
@@ -336,7 +352,7 @@ extension HTTPConnectionPool.ConnectionFactory {
                             context: sslContext,
                             serverHostname: hostname
                         )
-                        let tlsEventHandler = TLSEventsHandler(deadline: .now() + self.timeouts.tlsHandshake)
+                        let tlsEventHandler = TLSEventsHandler(deadline: deadline)
 
                         try sync.addHandler(sslHandler)
                         try sync.addHandler(tlsEventHandler)
diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift
@@ -912,6 +912,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
         case bodyLengthMismatch
         case writeAfterRequestSent
         case incompatibleHeaders
+        case connectTimeout
         case socksHandshakeTimeout
         case httpProxyHandshakeTimeout
         case tlsHandshakeTimeout
@@ -972,6 +973,8 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
     public static let writeAfterRequestSent = HTTPClientError(code: .writeAfterRequestSent)
     /// Incompatible headers specified, for example `Transfer-Encoding` and `Content-Length`.
     public static let incompatibleHeaders = HTTPClientError(code: .incompatibleHeaders)
+    /// Creating a new tcp connection timed out
+    public static let connectTimeout = HTTPClientError(code: .connectTimeout)
     /// The socks handshake timed out.
     public static let socksHandshakeTimeout = HTTPClientError(code: .socksHandshakeTimeout)
     /// The http proxy connection creation timed out.
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift
@@ -88,8 +88,8 @@ class HTTPClientNIOTSTests: XCTestCase {
         let port = httpBin.port
         XCTAssertNoThrow(try httpBin.shutdown())
 
-        XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(port)/get").wait()) { error in
-            XCTAssertEqual(.connectTimeout(.milliseconds(100)), error as? ChannelError)
+        XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(port)/get").wait()) {
+            XCTAssertEqual($0 as? HTTPClientError, .connectTimeout)
         }
     }
 
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
@@ -587,8 +587,8 @@ 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
-            XCTAssertEqual(.connectTimeout(.milliseconds(100)), error as? ChannelError)
+        XCTAssertThrowsError(try httpClient.get(url: "http://198.51.100.254:65535/get").wait()) {
+            XCTAssertEqual($0 as? HTTPClientError, .connectTimeout)
         }
     }
 
@@ -1841,15 +1841,9 @@ class HTTPClientTests: XCTestCase {
 
         XCTAssertThrowsError(try localClient.get(url: "http://localhost:\(port)").wait()) { error in
             if isTestingNIOTS() {
-                guard case ChannelError.connectTimeout = error else {
-                    XCTFail("Unexpected error: \(error)")
-                    return
-                }
+                XCTAssertEqual(error as? HTTPClientError, .connectTimeout)
             } else {
-                guard error is NIOConnectionError else {
-                    XCTFail("Unexpected error: \(error)")
-                    return
-                }
+                XCTAssert(error is NIOConnectionError, "Unexpected error: \(error)")
             }
         }
     }
@@ -2505,9 +2499,9 @@ class HTTPClientTests: XCTestCase {
         let request = try HTTPClient.Request(url: "http://198.51.100.254:65535/get")
         let delegate = TestDelegate()
 
-        XCTAssertThrowsError(try httpClient.execute(request: request, delegate: delegate).wait()) { error in
-            XCTAssertEqual(.connectTimeout(.milliseconds(10)), error as? ChannelError)
-            XCTAssertEqual(.connectTimeout(.milliseconds(10)), delegate.error as? ChannelError)
+        XCTAssertThrowsError(try httpClient.execute(request: request, delegate: delegate).wait()) {
+            XCTAssertEqual(.connectTimeout, $0 as? HTTPClientError)
+            XCTAssertEqual(.connectTimeout, delegate.error as? HTTPClientError)
         }
     }
 
@@ -2733,7 +2727,7 @@ class HTTPClientTests: XCTestCase {
 
         XCTAssertThrowsError(try task.wait()) { error in
             if isTestingNIOTS() {
-                XCTAssertEqual(error as? ChannelError, .connectTimeout(.milliseconds(100)))
+                XCTAssertEqual(error as? HTTPClientError, .connectTimeout)
             } else {
                 switch error as? NIOSSLError {
                 case .some(.handshakeFailed(.sslError(_))): break
@@ -2780,7 +2774,7 @@ class HTTPClientTests: XCTestCase {
 
         XCTAssertThrowsError(try task.wait()) { error in
             if isTestingNIOTS() {
-                XCTAssertEqual(error as? ChannelError, .connectTimeout(.milliseconds(200)))
+                XCTAssertEqual(error as? HTTPClientError, .connectTimeout)
             } else {
                 switch error as? NIOSSLError {
                 case .some(.handshakeFailed(.sslError(_))): break
diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+FactoryTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+FactoryTests.swift

Original file line number	Diff line number	Diff line change
`@@ -88,8 +88,8 @@ class HTTPClientNIOTSTests: XCTestCase {`
`88`	`88`	`let port = httpBin.port`
`89`	`89`	`XCTAssertNoThrow(try httpBin.shutdown())`
`90`	`90`
`91`		`- XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(port)/get").wait()) { error in`
`92`		`- XCTAssertEqual(.connectTimeout(.milliseconds(100)), error as? ChannelError)`
	`91`	`+ XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(port)/get").wait()) {`
	`92`	`+ XCTAssertEqual($0 as? HTTPClientError, .connectTimeout)`
`93`	`93`	`}`
`94`	`94`	`}`
`95`	`95`