Code review

Author: fabianfett

Diff for: Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/HTTP1ProxyConnectHandler.swift

@@ -82,10 +82,8 @@ final class HTTP1ProxyConnectHandler: ChannelDuplexHandler, RemovableChannelHand
             preconditionFailure("How can we receive a channelInactive before a channelActive?")
         case .connectSent(let timeout), .headReceived(let timeout):
             timeout.cancel()
-            let error = HTTPClientError.remoteConnectionClosed
-            self.state = .failed(error)
-            self.proxyEstablishedPromise?.fail(error)
-            context.fireErrorCaught(error)
+            self.failWithError(HTTPClientError.remoteConnectionClosed, context: context, closeConnection: false)
+
         case .failed, .completed:
             break
         }
@@ -98,7 +96,7 @@ final class HTTP1ProxyConnectHandler: ChannelDuplexHandler, RemovableChannelHand
     func channelRead(context: ChannelHandlerContext, data: NIOAny) {
         switch self.unwrapInboundIn(data) {
         case .head(let head):
-            guard case .connectSent(let promise) = self.state else {
+            guard case .connectSent(let scheduled) = self.state else {
                 preconditionFailure("HTTPDecoder should throw an error, if we have not send a request")
             }
 
@@ -107,34 +105,21 @@ final class HTTP1ProxyConnectHandler: ChannelDuplexHandler, RemovableChannelHand
                 // Any 2xx (Successful) response indicates that the sender (and all
                 // inbound proxies) will switch to tunnel mode immediately after the
                 // blank line that concludes the successful response's header section
-                self.state = .headReceived(promise)
+                self.state = .headReceived(scheduled)
             case 407:
-                let error = HTTPClientError.proxyAuthenticationRequired
-                self.state = .failed(error)
-                self.proxyEstablishedPromise?.fail(error)
-                context.fireErrorCaught(error)
-                context.close(mode: .all, promise: nil)
+                self.failWithError(HTTPClientError.proxyAuthenticationRequired, context: context)
 
             default:
-                // Any response other than a successful response
-                // indicates that the tunnel has not yet been formed and that the
-                // connection remains governed by HTTP.
-                let error = HTTPClientError.invalidProxyResponse
-                self.state = .failed(error)
-                self.proxyEstablishedPromise?.fail(error)
-                context.fireErrorCaught(error)
-                context.close(mode: .all, promise: nil)
+                // Any response other than a successful response indicates that the tunnel
+                // has not yet been formed and that the connection remains governed by HTTP.
+                self.failWithError(HTTPClientError.invalidProxyResponse, context: context)
             }
         case .body:
             switch self.state {
             case .headReceived(let timeout):
                 timeout.cancel()
                 // we don't expect a body
-                let error = HTTPClientError.invalidProxyResponse
-                self.state = .failed(error)
-                self.proxyEstablishedPromise?.fail(error)
-                context.fireErrorCaught(error)
-                context.close(mode: .all, promise: nil)
+                self.failWithError(HTTPClientError.invalidProxyResponse, context: context)
 
             case .failed:
                 // ran into an error before... ignore this one
@@ -171,11 +156,7 @@ final class HTTP1ProxyConnectHandler: ChannelDuplexHandler, RemovableChannelHand
                 preconditionFailure("How can we have a scheduled timeout, if the connection is not even up?")
 
             case .connectSent, .headReceived:
-                let error = HTTPClientError.httpProxyHandshakeTimeout
-                self.state = .failed(error)
-                self.proxyEstablishedPromise?.fail(error)
-                context.fireErrorCaught(error)
-                context.close(mode: .all, promise: nil)
+                self.failWithError(HTTPClientError.httpProxyHandshakeTimeout, context: context)
 
             case .failed, .completed:
                 break
@@ -196,4 +177,13 @@ final class HTTP1ProxyConnectHandler: ChannelDuplexHandler, RemovableChannelHand
         context.write(self.wrapOutboundOut(.end(nil)), promise: nil)
         context.flush()
     }
+
+    private func failWithError(_ error: Error, context: ChannelHandlerContext, closeConnection: Bool = true) {
+        self.state = .failed(error)
+        self.proxyEstablishedPromise?.fail(error)
+        context.fireErrorCaught(error)
+        if closeConnection {
+            context.close(mode: .all, promise: nil)
+        }
+    }
 }

Diff for: Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift

@@ -41,7 +41,7 @@ extension HTTPConnectionPool {
             self.key = key
             self.clientConfiguration = clientConfiguration
             self.sslContextCache = sslContextCache
-            self.tlsConfiguration = tlsConfiguration ?? clientConfiguration.tlsConfiguration ?? .forClient()
+            self.tlsConfiguration = tlsConfiguration ?? clientConfiguration.tlsConfiguration ?? .makeClientConfiguration()
         }
     }
 }

Diff for: Tests/AsyncHTTPClientTests/HTTPConnectionPool+FactoryTests.swift

@@ -16,6 +16,7 @@
 import Logging
 import NIO
 import NIOSOCKS
+import NIOSSL
 import XCTest
 
 class HTTPConnectionPool_FactoryTests: XCTestCase {
@@ -141,10 +142,12 @@ class HTTPConnectionPool_FactoryTests: XCTestCase {
 
         let request = try! HTTPClient.Request(url: "https://localhost:\(server!.localAddress!.port!)")
 
+        var tlsConfig = TLSConfiguration.makeClientConfiguration()
+        tlsConfig.certificateVerification = .none
         let factory = HTTPConnectionPool.ConnectionFactory(
             key: .init(request),
             tlsConfiguration: nil,
-            clientConfiguration: .init(tlsConfiguration: .forClient(certificateVerification: .none)),
+            clientConfiguration: .init(tlsConfiguration: tlsConfig),
             sslContextCache: .init()
         )