Skip to content

Commit f4ce479

Browse files
fabianfettfabianfett
committed
Code review
1 parent 81895c3 commit f4ce479

8 files changed

+62
-40
lines changed

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ final class HTTP1ProxyConnectHandler: ChannelDuplexHandler, RemovableChannelHand
4141
let deadline: NIODeadline
4242

4343
private var proxyEstablishedPromise: EventLoopPromise<Void>?
44-
var proxyEstablishedFuture: EventLoopFuture<Void>! {
44+
var proxyEstablishedFuture: EventLoopFuture<Void>? {
4545
return self.proxyEstablishedPromise?.futureResult
4646
}
4747

Diff for: ‎Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/SOCKSEventsHandler.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ final class SOCKSEventsHandler: ChannelInboundHandler, RemovableChannelHandler {
3030
}
3131

3232
private var socksEstablishedPromise: EventLoopPromise<Void>?
33-
var socksEstablishedFuture: EventLoopFuture<Void>! {
33+
var socksEstablishedFuture: EventLoopFuture<Void>? {
3434
return self.socksEstablishedPromise?.futureResult
3535
}
3636

Diff for: ‎Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/TLSEventsHandler.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ final class TLSEventsHandler: ChannelInboundHandler, RemovableChannelHandler {
3030
}
3131

3232
private var tlsEstablishedPromise: EventLoopPromise<String?>?
33-
var tlsEstablishedFuture: EventLoopFuture<String?>! {
33+
var tlsEstablishedFuture: EventLoopFuture<String?>? {
3434
return self.tlsEstablishedPromise?.futureResult
3535
}
3636

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

+38-21
Original file line numberDiff line numberDiff line change
@@ -29,21 +29,20 @@ extension HTTPConnectionPool {
2929
}
3030

3131
final class ConnectionFactory {
32-
3332
struct Timeouts {
3433
/// the `TimeAmount` waited before the TCP connection creation is failed with a timeout error
3534
var connect: TimeAmount = .seconds(10)
36-
35+
3736
/// the `TimeAmount` waited before the SOCKS proxy connection creation is failed with a timeout error
3837
var socksProxyHandshake: TimeAmount = .seconds(10)
39-
38+
4039
/// the `TimeAmount` waited before the HTTP proxy connection creation is failed with a timeout error
4140
var httpProxyHandshake: TimeAmount = .seconds(10)
42-
41+
4342
/// the `TimeAmount` waited before we the TLS handshake is failed with a timeout error
4443
var tlsHandshake: TimeAmount = .seconds(10)
4544
}
46-
45+
4746
let key: ConnectionPool.Key
4847
let clientConfiguration: HTTPClient.Configuration
4948
let tlsConfiguration: TLSConfiguration
@@ -59,7 +58,7 @@ extension HTTPConnectionPool {
5958
self.clientConfiguration = clientConfiguration
6059
self.sslContextCache = sslContextCache
6160
self.tlsConfiguration = tlsConfiguration ?? clientConfiguration.tlsConfiguration ?? .forClient()
62-
61+
6362
var timeouts = timeouts
6463
if let connect = clientConfiguration.timeout.connect {
6564
timeouts.connect = connect
@@ -88,10 +87,10 @@ extension HTTPConnectionPool.ConnectionFactory {
8887
case .http, .http_unix, .unix:
8988
return self.makePlainChannel(eventLoop: eventLoop).map { ($0, .http1_1) }
9089
case .https, .https_unix:
91-
return self.makeTLSChannel(eventLoop: eventLoop, logger: logger).map {
92-
(channel, negotiated) -> (Channel, HTTPVersion) in
93-
let version = negotiated == "h2" ? HTTPVersion.http2 : HTTPVersion.http1_1
94-
return (channel, version)
90+
return self.makeTLSChannel(eventLoop: eventLoop, logger: logger).flatMapThrowing {
91+
channel, negotiated in
92+
93+
(channel, try self.matchALPNToHTTPVersion(negotiated))
9594
}
9695
}
9796
}
@@ -137,7 +136,9 @@ extension HTTPConnectionPool.ConnectionFactory {
137136
return channel.eventLoop.makeFailedFuture(error)
138137
}
139138

140-
return proxyHandler.proxyEstablishedFuture.flatMap {
139+
// The proxyEstablishedFuture is set as soon as the HTTP1ProxyConnectHandler is in a
140+
// pipeline. It is created in HTTP1ProxyConnectHandler's handlerAdded method.
141+
return proxyHandler.proxyEstablishedFuture!.flatMap {
141142
channel.pipeline.removeHandler(proxyHandler).flatMap {
142143
channel.pipeline.removeHandler(decoder).flatMap {
143144
channel.pipeline.removeHandler(encoder)
@@ -170,7 +171,9 @@ extension HTTPConnectionPool.ConnectionFactory {
170171
return channel.eventLoop.makeFailedFuture(error)
171172
}
172173

173-
return socksEventHandler.socksEstablishedFuture.flatMap {
174+
// The socksEstablishedFuture is set as soon as the SOCKSEventsHandler is in a
175+
// pipeline. It is created in SOCKSEventsHandler's handlerAdded method.
176+
return socksEventHandler.socksEstablishedFuture!.flatMap {
174177
channel.pipeline.removeHandler(socksEventHandler).flatMap {
175178
channel.pipeline.removeHandler(socksConnectHandler)
176179
}
@@ -206,18 +209,16 @@ extension HTTPConnectionPool.ConnectionFactory {
206209
)
207210
try channel.pipeline.syncOperations.addHandler(sslHandler)
208211
try channel.pipeline.syncOperations.addHandler(tlsEventHandler)
209-
return tlsEventHandler.tlsEstablishedFuture
212+
213+
// The tlsEstablishedFuture is set as soon as the TLSEventsHandler is in a
214+
// pipeline. It is created in TLSEventsHandler's handlerAdded method.
215+
return tlsEventHandler.tlsEstablishedFuture!
210216
} catch {
211217
return channel.eventLoop.makeFailedFuture(error)
212218
}
213219
}.flatMap { negotiated -> EventLoopFuture<(Channel, HTTPVersion)> in
214-
channel.pipeline.removeHandler(tlsEventHandler).map {
215-
switch negotiated {
216-
case "h2":
217-
return (channel, .http2)
218-
default:
219-
return (channel, .http1_1)
220-
}
220+
channel.pipeline.removeHandler(tlsEventHandler).flatMapThrowing {
221+
(channel, try self.matchALPNToHTTPVersion(negotiated))
221222
}
222223
}
223224
}
@@ -263,8 +264,13 @@ extension HTTPConnectionPool.ConnectionFactory {
263264
preconditionFailure("Unexpected scheme")
264265
}
265266
}.flatMap { channel -> EventLoopFuture<(Channel, String?)> in
267+
// It is save to use `try!` here, since we are sure, that a `TLSEventsHandler` exists
268+
// within the pipeline. It is added in `makeTLSBootstrap`.
266269
let tlsEventHandler = try! channel.pipeline.syncOperations.handler(type: TLSEventsHandler.self)
267-
return tlsEventHandler.tlsEstablishedFuture.flatMap { negotiated in
270+
271+
// The tlsEstablishedFuture is set as soon as the TLSEventsHandler is in a
272+
// pipeline. It is created in TLSEventsHandler's handlerAdded method.
273+
return tlsEventHandler.tlsEstablishedFuture!.flatMap { negotiated in
268274
channel.pipeline.removeHandler(tlsEventHandler).map { (channel, negotiated) }
269275
}
270276
}
@@ -343,6 +349,17 @@ extension HTTPConnectionPool.ConnectionFactory {
343349

344350
return eventLoop.makeSucceededFuture(bootstrap)
345351
}
352+
353+
private func matchALPNToHTTPVersion(_ negotiated: String?) throws -> HTTPVersion {
354+
switch negotiated {
355+
case .none, .some("http/1.1"):
356+
return .http1_1
357+
case .some("h2"):
358+
return .http2
359+
case .some(let unsupported):
360+
throw HTTPClientError.serverOfferedUnsupportedApplicationProtocol(unsupported)
361+
}
362+
}
346363
}
347364

348365
extension ConnectionPool.Key.Scheme {

Diff for: ‎Sources/AsyncHTTPClient/HTTPClient.swift

+5
Original file line numberDiff line numberDiff line change
@@ -911,6 +911,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
911911
case socksHandshakeTimeout
912912
case httpProxyHandshakeTimeout
913913
case tlsHandshakeTimeout
914+
case serverOfferedUnsupportedApplicationProtocol(String)
914915
}
915916

916917
private var code: Code
@@ -973,4 +974,8 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
973974
public static let httpProxyHandshakeTimeout = HTTPClientError(code: .httpProxyHandshakeTimeout)
974975
/// The tls handshake timed out.
975976
public static let tlsHandshakeTimeout = HTTPClientError(code: .tlsHandshakeTimeout)
977+
/// The remote server only offered an unsupported application protocol
978+
public static func serverOfferedUnsupportedApplicationProtocol(_ proto: String) -> HTTPClientError {
979+
return HTTPClientError(code: .serverOfferedUnsupportedApplicationProtocol(proto))
980+
}
976981
}

Diff for: ‎Tests/AsyncHTTPClientTests/HTTP1ProxyConnectHandlerTests.swift

+7-7
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ class HTTP1ProxyConnectHandlerTests: XCTestCase {
4949
XCTAssertNoThrow(try embedded.writeInbound(HTTPClientResponsePart.head(responseHead)))
5050
XCTAssertNoThrow(try embedded.writeInbound(HTTPClientResponsePart.end(nil)))
5151

52-
XCTAssertNoThrow(try proxyConnectHandler.proxyEstablishedFuture.wait())
52+
XCTAssertNoThrow(try XCTUnwrap(proxyConnectHandler.proxyEstablishedFuture).wait())
5353
}
5454

5555
func testProxyConnectWithAuthorization() {
@@ -82,7 +82,7 @@ class HTTP1ProxyConnectHandlerTests: XCTestCase {
8282
XCTAssertNoThrow(try embedded.writeInbound(HTTPClientResponsePart.head(responseHead)))
8383
XCTAssertNoThrow(try embedded.writeInbound(HTTPClientResponsePart.end(nil)))
8484

85-
XCTAssertNoThrow(try proxyConnectHandler.proxyEstablishedFuture.wait())
85+
XCTAssertNoThrow(try XCTUnwrap(proxyConnectHandler.proxyEstablishedFuture).wait())
8686
}
8787

8888
func testProxyConnectWithoutAuthorizationFailure500() {
@@ -119,7 +119,7 @@ class HTTP1ProxyConnectHandlerTests: XCTestCase {
119119
XCTAssertFalse(embedded.isActive, "Channel should be closed in response to the error")
120120
XCTAssertNoThrow(try embedded.writeInbound(HTTPClientResponsePart.end(nil)))
121121

122-
XCTAssertThrowsError(try proxyConnectHandler.proxyEstablishedFuture.wait()) {
122+
XCTAssertThrowsError(try XCTUnwrap(proxyConnectHandler.proxyEstablishedFuture).wait()) {
123123
XCTAssertEqual($0 as? HTTPClientError, .invalidProxyResponse)
124124
}
125125
}
@@ -158,8 +158,8 @@ class HTTP1ProxyConnectHandlerTests: XCTestCase {
158158
XCTAssertFalse(embedded.isActive, "Channel should be closed in response to the error")
159159
XCTAssertNoThrow(try embedded.writeInbound(HTTPClientResponsePart.end(nil)))
160160

161-
XCTAssertThrowsError(try proxyConnectHandler.proxyEstablishedFuture.wait()) { error in
162-
XCTAssertEqual(error as? HTTPClientError, .proxyAuthenticationRequired)
161+
XCTAssertThrowsError(try XCTUnwrap(proxyConnectHandler.proxyEstablishedFuture).wait()) {
162+
XCTAssertEqual($0 as? HTTPClientError, .proxyAuthenticationRequired)
163163
}
164164
}
165165

@@ -197,8 +197,8 @@ class HTTP1ProxyConnectHandlerTests: XCTestCase {
197197
XCTAssertEqual(embedded.isActive, false)
198198
XCTAssertNoThrow(try embedded.writeInbound(HTTPClientResponsePart.end(nil)))
199199

200-
XCTAssertThrowsError(try proxyConnectHandler.proxyEstablishedFuture.wait()) { error in
201-
XCTAssertEqual(error as? HTTPClientError, .invalidProxyResponse)
200+
XCTAssertThrowsError(try XCTUnwrap(proxyConnectHandler.proxyEstablishedFuture).wait()) {
201+
XCTAssertEqual($0 as? HTTPClientError, .invalidProxyResponse)
202202
}
203203
}
204204
}

Diff for: ‎Tests/AsyncHTTPClientTests/SOCKSEventsHandlerTests.swift

+5-5
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ class SOCKSEventsHandlerTests: XCTestCase {
2727
XCTAssertNoThrow(try embedded.connect(to: .makeAddressResolvingHost("localhost", port: 0)).wait())
2828

2929
embedded.pipeline.fireUserInboundEventTriggered(SOCKSProxyEstablishedEvent())
30-
XCTAssertNoThrow(try socksEventsHandler.socksEstablishedFuture.wait())
30+
XCTAssertNoThrow(try XCTUnwrap(socksEventsHandler.socksEstablishedFuture).wait())
3131
}
3232

3333
func testHandlerFailsFutureWhenRemovedWithoutEvent() {
@@ -37,7 +37,7 @@ class SOCKSEventsHandlerTests: XCTestCase {
3737
XCTAssertNotNil(socksEventsHandler.socksEstablishedFuture)
3838

3939
XCTAssertNoThrow(try embedded.pipeline.removeHandler(socksEventsHandler).wait())
40-
XCTAssertThrowsError(try socksEventsHandler.socksEstablishedFuture.wait())
40+
XCTAssertThrowsError(try XCTUnwrap(socksEventsHandler.socksEstablishedFuture).wait())
4141
}
4242

4343
func testHandlerFailsFutureWhenHandshakeFails() {
@@ -48,7 +48,7 @@ class SOCKSEventsHandlerTests: XCTestCase {
4848

4949
let error = SOCKSError.InvalidReservedByte(actual: 19)
5050
embedded.pipeline.fireErrorCaught(error)
51-
XCTAssertThrowsError(try socksEventsHandler.socksEstablishedFuture.wait()) {
51+
XCTAssertThrowsError(try XCTUnwrap(socksEventsHandler.socksEstablishedFuture).wait()) {
5252
XCTAssertEqual($0 as? SOCKSError.InvalidReservedByte, error)
5353
}
5454
}
@@ -64,7 +64,7 @@ class SOCKSEventsHandlerTests: XCTestCase {
6464

6565
embedded.embeddedEventLoop.advanceTime(by: .milliseconds(20))
6666

67-
XCTAssertThrowsError(try socksEventsHandler.socksEstablishedFuture.wait()) {
67+
XCTAssertThrowsError(try XCTUnwrap(socksEventsHandler.socksEstablishedFuture).wait()) {
6868
XCTAssertEqual($0 as? HTTPClientError, .socksHandshakeTimeout)
6969
}
7070
XCTAssertFalse(embedded.isActive, "The timeout shall close the connection")
@@ -81,7 +81,7 @@ class SOCKSEventsHandlerTests: XCTestCase {
8181

8282
// schedules execute only on the next tick
8383
embedded.embeddedEventLoop.run()
84-
XCTAssertThrowsError(try socksEventsHandler.socksEstablishedFuture.wait()) {
84+
XCTAssertThrowsError(try XCTUnwrap(socksEventsHandler.socksEstablishedFuture).wait()) {
8585
XCTAssertEqual($0 as? HTTPClientError, .socksHandshakeTimeout)
8686
}
8787
XCTAssertFalse(embedded.isActive, "The timeout shall close the connection")

Diff for: ‎Tests/AsyncHTTPClientTests/TLSEventsHandlerTests.swift

+4-4
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ class TLSEventsHandlerTests: XCTestCase {
2828
XCTAssertNoThrow(try embedded.connect(to: .makeAddressResolvingHost("localhost", port: 0)).wait())
2929

3030
embedded.pipeline.fireUserInboundEventTriggered(TLSUserEvent.handshakeCompleted(negotiatedProtocol: "abcd1234"))
31-
XCTAssertEqual(try tlsEventsHandler.tlsEstablishedFuture.wait(), "abcd1234")
31+
XCTAssertEqual(try XCTUnwrap(tlsEventsHandler.tlsEstablishedFuture).wait(), "abcd1234")
3232
}
3333

3434
func testHandlerFailsFutureWhenRemovedWithoutEvent() {
@@ -38,7 +38,7 @@ class TLSEventsHandlerTests: XCTestCase {
3838
XCTAssertNotNil(tlsEventsHandler.tlsEstablishedFuture)
3939

4040
XCTAssertNoThrow(try embedded.pipeline.removeHandler(tlsEventsHandler).wait())
41-
XCTAssertThrowsError(try tlsEventsHandler.tlsEstablishedFuture.wait())
41+
XCTAssertThrowsError(try XCTUnwrap(tlsEventsHandler.tlsEstablishedFuture).wait())
4242
}
4343

4444
func testHandlerFailsFutureWhenHandshakeFails() {
@@ -48,7 +48,7 @@ class TLSEventsHandlerTests: XCTestCase {
4848
XCTAssertNotNil(tlsEventsHandler.tlsEstablishedFuture)
4949

5050
embedded.pipeline.fireErrorCaught(NIOSSLError.handshakeFailed(BoringSSLError.wantConnect))
51-
XCTAssertThrowsError(try tlsEventsHandler.tlsEstablishedFuture.wait()) {
51+
XCTAssertThrowsError(try XCTUnwrap(tlsEventsHandler.tlsEstablishedFuture).wait()) {
5252
XCTAssertEqual($0 as? NIOSSLError, .handshakeFailed(BoringSSLError.wantConnect))
5353
}
5454
}
@@ -65,6 +65,6 @@ class TLSEventsHandlerTests: XCTestCase {
6565
embedded.pipeline.fireUserInboundEventTriggered(TLSUserEvent.shutdownCompleted)
6666

6767
embedded.pipeline.fireUserInboundEventTriggered(TLSUserEvent.handshakeCompleted(negotiatedProtocol: "alpn"))
68-
XCTAssertEqual(try tlsEventsHandler.tlsEstablishedFuture.wait(), "alpn")
68+
XCTAssertEqual(try XCTUnwrap(tlsEventsHandler.tlsEstablishedFuture).wait(), "alpn")
6969
}
7070
}

0 commit comments

Comments
 (0)