From 7b7690993e6a479699f6ef2fa977368ff847717a Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 13 Oct 2021 15:57:36 +0200 Subject: [PATCH 1/4] add internal http2 configuration --- .../HTTPConnectionPool+Factory.swift | 21 ++--- Sources/AsyncHTTPClient/HTTPClient.swift | 84 +++++++++++++++++-- .../HTTP2ConnectionTests.swift | 12 +-- 3 files changed, 92 insertions(+), 25 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift index 5cc49c994..67433b3d6 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift @@ -31,19 +31,14 @@ extension HTTPConnectionPool { let tlsConfiguration: TLSConfiguration let sslContextCache: SSLContextCache - // This property can be removed once we enable true http/2 support - let allowHTTP2Connections: Bool - init(key: ConnectionPool.Key, tlsConfiguration: TLSConfiguration?, clientConfiguration: HTTPClient.Configuration, - sslContextCache: SSLContextCache, - allowHTTP2Connections: Bool = false) { + sslContextCache: SSLContextCache) { self.key = key self.clientConfiguration = clientConfiguration self.sslContextCache = sslContextCache self.tlsConfiguration = tlsConfiguration ?? clientConfiguration.tlsConfiguration ?? .makeClientConfiguration() - self.allowHTTP2Connections = allowHTTP2Connections } } } @@ -303,13 +298,14 @@ extension HTTPConnectionPool.ConnectionFactory { return channel.eventLoop.makeSucceededFuture(.http1_1(channel)) case .https: var tlsConfig = self.tlsConfiguration - // since we can support h2, we need to advertise this in alpn - if self.allowHTTP2Connections { + switch self.clientConfiguration.http2.configuration { + case .automatic: + // since we can support h2, we need to advertise this in alpn // "ProtocolNameList" contains the list of protocols advertised by the // client, in descending order of preference. // https://datatracker.ietf.org/doc/html/rfc7301#section-3.1 tlsConfig.applicationProtocols = ["h2", "http/1.1"] - } else { + case .disabled: tlsConfig.applicationProtocols = ["http/1.1"] } let tlsEventHandler = TLSEventsHandler(deadline: deadline) @@ -407,14 +403,15 @@ extension HTTPConnectionPool.ConnectionFactory { private func makeTLSBootstrap(deadline: NIODeadline, eventLoop: EventLoop, logger: Logger) -> EventLoopFuture { - // since we can support h2, we need to advertise this in alpn var tlsConfig = self.tlsConfiguration - if self.allowHTTP2Connections { + switch self.clientConfiguration.http2.configuration { + case .automatic: + // since we can support h2, we need to advertise this in alpn // "ProtocolNameList" contains the list of protocols advertised by the // client, in descending order of preference. // https://datatracker.ietf.org/doc/html/rfc7301#section-3.1 tlsConfig.applicationProtocols = ["h2", "http/1.1"] - } else { + case .disabled: tlsConfig.applicationProtocols = ["http/1.1"] } diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 3e45d4dfb..0f095d5b8 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -603,13 +603,66 @@ public class HTTPClient { /// Ignore TLS unclean shutdown error, defaults to `false`. public var ignoreUncleanSSLShutdown: Bool - public init(tlsConfiguration: TLSConfiguration? = nil, - redirectConfiguration: RedirectConfiguration? = nil, - timeout: Timeout = Timeout(), - connectionPool: ConnectionPool = ConnectionPool(), - proxy: Proxy? = nil, - ignoreUncleanSSLShutdown: Bool = false, - decompression: Decompression = .disabled) { + // TODO: make public + // TODO: set to automatic by default + /// HTTP/2 is by default disabled + internal var http2: HTTP2 + + // TODO: make public + internal init( + certificateVerification: CertificateVerification, + redirectConfiguration: RedirectConfiguration? = nil, + timeout: Timeout = Timeout(), + connectionPool: TimeAmount = .seconds(60), + proxy: Proxy? = nil, + ignoreUncleanSSLShutdown: Bool = false, + decompression: Decompression = .disabled, + backgroundActivityLogger: Logger? = nil, + http2: HTTP2 + ) { + self.init( + certificateVerification: certificateVerification, + redirectConfiguration: redirectConfiguration, + timeout: timeout, + connectionPool: connectionPool, + proxy: proxy, + ignoreUncleanSSLShutdown: ignoreUncleanSSLShutdown, + decompression: decompression, + backgroundActivityLogger: backgroundActivityLogger + ) + } + + public init( + tlsConfiguration: TLSConfiguration? = nil, + redirectConfiguration: RedirectConfiguration? = nil, + timeout: Timeout = Timeout(), + connectionPool: ConnectionPool = ConnectionPool(), + proxy: Proxy? = nil, + ignoreUncleanSSLShutdown: Bool = false, + decompression: Decompression = .disabled + ) { + self.init( + tlsConfiguration: tlsConfiguration, + redirectConfiguration: redirectConfiguration, + timeout: timeout, connectionPool: connectionPool, + proxy: proxy, + ignoreUncleanSSLShutdown: ignoreUncleanSSLShutdown, + decompression: decompression, + // TODO: set to automatic by default + http2: .disabled + ) + } + + internal init( + tlsConfiguration: TLSConfiguration? = nil, + redirectConfiguration: RedirectConfiguration? = nil, + timeout: Timeout = Timeout(), + connectionPool: ConnectionPool = ConnectionPool(), + proxy: Proxy? = nil, + ignoreUncleanSSLShutdown: Bool = false, + decompression: Decompression = .disabled, + http2: HTTP2 + ) { self.tlsConfiguration = tlsConfiguration self.redirectConfiguration = redirectConfiguration ?? RedirectConfiguration() self.timeout = timeout @@ -617,6 +670,7 @@ public class HTTPClient { self.proxy = proxy self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown self.decompression = decompression + self.http2 = http2 } public init(tlsConfiguration: TLSConfiguration? = nil, @@ -830,6 +884,22 @@ extension HTTPClient.Configuration { self.concurrentHTTP1ConnectionsPerHostSoftLimit = concurrentHTTP1ConnectionsPerHostSoftLimit } } + + // TODO: make this struct and its static properties public + internal struct HTTP2 { + internal enum Configuration { + case disabled + case automatic + } + + /// HTTP/2 is completely disabled + internal static let disabled: Self = .init(configuration: .disabled) + + /// HTTP/2 is used if we connect to a server with HTTPS and the server supports HTTP/2 + internal static let automatic: Self = .init(configuration: .automatic) + + internal var configuration: Configuration + } } /// Possible client errors. diff --git a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift index d940f01f6..e5ecb915a 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift @@ -253,9 +253,9 @@ class TestConnectionCreator { let factory = HTTPConnectionPool.ConnectionFactory( key: .init(request), tlsConfiguration: tlsConfiguration, - clientConfiguration: .init(), - sslContextCache: .init(), - allowHTTP2Connections: true + // TODO: use default client configuration once http/2 is enabled by default + clientConfiguration: .init(http2: .automatic), + sslContextCache: .init() ) let promise = try self.lock.withLock { () -> EventLoopPromise in @@ -295,9 +295,9 @@ class TestConnectionCreator { let factory = HTTPConnectionPool.ConnectionFactory( key: .init(request), tlsConfiguration: tlsConfiguration, - clientConfiguration: .init(), - sslContextCache: .init(), - allowHTTP2Connections: true + // TODO: use default client configuration once http/2 is enabled by default + clientConfiguration: .init(http2: .automatic), + sslContextCache: .init() ) let promise = try self.lock.withLock { () -> EventLoopPromise in From d0a78de71fe8db9fcf28490db44c1ec0719480d3 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 2 Nov 2021 18:06:26 +0000 Subject: [PATCH 2/4] rename HTTP2 to HTTPVersion to be more future proof --- .../HTTPConnectionPool+Factory.swift | 8 ++-- Sources/AsyncHTTPClient/HTTPClient.swift | 45 +++++-------------- .../HTTP2ConnectionTests.swift | 4 +- 3 files changed, 17 insertions(+), 40 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift index 67433b3d6..87c5255c9 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift @@ -298,14 +298,14 @@ extension HTTPConnectionPool.ConnectionFactory { return channel.eventLoop.makeSucceededFuture(.http1_1(channel)) case .https: var tlsConfig = self.tlsConfiguration - switch self.clientConfiguration.http2.configuration { + switch self.clientConfiguration.httpVersion.configuration { case .automatic: // since we can support h2, we need to advertise this in alpn // "ProtocolNameList" contains the list of protocols advertised by the // client, in descending order of preference. // https://datatracker.ietf.org/doc/html/rfc7301#section-3.1 tlsConfig.applicationProtocols = ["h2", "http/1.1"] - case .disabled: + case .http1Only: tlsConfig.applicationProtocols = ["http/1.1"] } let tlsEventHandler = TLSEventsHandler(deadline: deadline) @@ -404,14 +404,14 @@ extension HTTPConnectionPool.ConnectionFactory { private func makeTLSBootstrap(deadline: NIODeadline, eventLoop: EventLoop, logger: Logger) -> EventLoopFuture { var tlsConfig = self.tlsConfiguration - switch self.clientConfiguration.http2.configuration { + switch self.clientConfiguration.httpVersion.configuration { case .automatic: // since we can support h2, we need to advertise this in alpn // "ProtocolNameList" contains the list of protocols advertised by the // client, in descending order of preference. // https://datatracker.ietf.org/doc/html/rfc7301#section-3.1 tlsConfig.applicationProtocols = ["h2", "http/1.1"] - case .disabled: + case .http1Only: tlsConfig.applicationProtocols = ["http/1.1"] } diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 0f095d5b8..8a7b6a3b5 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -606,31 +606,7 @@ public class HTTPClient { // TODO: make public // TODO: set to automatic by default /// HTTP/2 is by default disabled - internal var http2: HTTP2 - - // TODO: make public - internal init( - certificateVerification: CertificateVerification, - redirectConfiguration: RedirectConfiguration? = nil, - timeout: Timeout = Timeout(), - connectionPool: TimeAmount = .seconds(60), - proxy: Proxy? = nil, - ignoreUncleanSSLShutdown: Bool = false, - decompression: Decompression = .disabled, - backgroundActivityLogger: Logger? = nil, - http2: HTTP2 - ) { - self.init( - certificateVerification: certificateVerification, - redirectConfiguration: redirectConfiguration, - timeout: timeout, - connectionPool: connectionPool, - proxy: proxy, - ignoreUncleanSSLShutdown: ignoreUncleanSSLShutdown, - decompression: decompression, - backgroundActivityLogger: backgroundActivityLogger - ) - } + internal var httpVersion: HTTPVersion public init( tlsConfiguration: TLSConfiguration? = nil, @@ -649,10 +625,11 @@ public class HTTPClient { ignoreUncleanSSLShutdown: ignoreUncleanSSLShutdown, decompression: decompression, // TODO: set to automatic by default - http2: .disabled + httpVersion: .http1Only ) } - + + // TODO: make public internal init( tlsConfiguration: TLSConfiguration? = nil, redirectConfiguration: RedirectConfiguration? = nil, @@ -661,7 +638,7 @@ public class HTTPClient { proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false, decompression: Decompression = .disabled, - http2: HTTP2 + httpVersion: HTTPVersion ) { self.tlsConfiguration = tlsConfiguration self.redirectConfiguration = redirectConfiguration ?? RedirectConfiguration() @@ -670,7 +647,7 @@ public class HTTPClient { self.proxy = proxy self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown self.decompression = decompression - self.http2 = http2 + self.httpVersion = httpVersion } public init(tlsConfiguration: TLSConfiguration? = nil, @@ -886,16 +863,16 @@ extension HTTPClient.Configuration { } // TODO: make this struct and its static properties public - internal struct HTTP2 { + internal struct HTTPVersion { internal enum Configuration { - case disabled + case http1Only case automatic } - /// HTTP/2 is completely disabled - internal static let disabled: Self = .init(configuration: .disabled) + /// we only use HTTP/1, even if the server would supports HTTP/2 + internal static let http1Only: Self = .init(configuration: .http1Only) - /// HTTP/2 is used if we connect to a server with HTTPS and the server supports HTTP/2 + /// HTTP/2 is used if we connect to a server with HTTPS and the server supports HTTP/2, otherwise we use HTTP/1 internal static let automatic: Self = .init(configuration: .automatic) internal var configuration: Configuration diff --git a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift index e5ecb915a..a5ee1694d 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift @@ -254,7 +254,7 @@ class TestConnectionCreator { key: .init(request), tlsConfiguration: tlsConfiguration, // TODO: use default client configuration once http/2 is enabled by default - clientConfiguration: .init(http2: .automatic), + clientConfiguration: .init(httpVersion: .automatic), sslContextCache: .init() ) @@ -296,7 +296,7 @@ class TestConnectionCreator { key: .init(request), tlsConfiguration: tlsConfiguration, // TODO: use default client configuration once http/2 is enabled by default - clientConfiguration: .init(http2: .automatic), + clientConfiguration: .init(httpVersion: .automatic), sslContextCache: .init() ) From 3afa4d72dca6881e56346f16b74a0ea0e27db91c Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 2 Nov 2021 18:14:52 +0000 Subject: [PATCH 3/4] remove TODOs as requested by Fabian --- Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift | 2 -- 1 file changed, 2 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift index a5ee1694d..4bbe013d0 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift @@ -253,7 +253,6 @@ class TestConnectionCreator { let factory = HTTPConnectionPool.ConnectionFactory( key: .init(request), tlsConfiguration: tlsConfiguration, - // TODO: use default client configuration once http/2 is enabled by default clientConfiguration: .init(httpVersion: .automatic), sslContextCache: .init() ) @@ -295,7 +294,6 @@ class TestConnectionCreator { let factory = HTTPConnectionPool.ConnectionFactory( key: .init(request), tlsConfiguration: tlsConfiguration, - // TODO: use default client configuration once http/2 is enabled by default clientConfiguration: .init(httpVersion: .automatic), sslContextCache: .init() ) From edf332351a5568defc02124cc083db69c1812d56 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 2 Nov 2021 18:19:34 +0000 Subject: [PATCH 4/4] swift format --- Sources/AsyncHTTPClient/HTTPClient.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 8a7b6a3b5..e140a8bc8 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -628,7 +628,7 @@ public class HTTPClient { httpVersion: .http1Only ) } - + // TODO: make public internal init( tlsConfiguration: TLSConfiguration? = nil,