Skip to content

Commit 390a790

Browse files
fabianfettfabianfett
committed
Code review
1 parent ee6a452 commit 390a790

File tree

4 files changed

+72
-68
lines changed

4 files changed

+72
-68
lines changed

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

+66-66
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,8 @@ final class HTTP1ProxyConnectHandler: ChannelDuplexHandler, RemovableChannelHand
8282
preconditionFailure("How can we receive a channelInactive before a channelActive?")
8383
case .connectSent(let timeout), .headReceived(let timeout):
8484
timeout.cancel()
85-
let error = HTTPClientError.remoteConnectionClosed
86-
self.state = .failed(error)
87-
self.proxyEstablishedPromise?.fail(error)
88-
context.fireErrorCaught(error)
85+
self.failWithError(HTTPClientError.remoteConnectionClosed, context: context, closeConnection: false)
86+
8987
case .failed, .completed:
9088
break
9189
}
@@ -98,68 +96,15 @@ final class HTTP1ProxyConnectHandler: ChannelDuplexHandler, RemovableChannelHand
9896
func channelRead(context: ChannelHandlerContext, data: NIOAny) {
9997
switch self.unwrapInboundIn(data) {
10098
case .head(let head):
101-
guard case .connectSent(let promise) = self.state else {
102-
preconditionFailure("HTTPDecoder should throw an error, if we have not send a request")
103-
}
104-
105-
switch head.status.code {
106-
case 200..<300:
107-
// Any 2xx (Successful) response indicates that the sender (and all
108-
// inbound proxies) will switch to tunnel mode immediately after the
109-
// blank line that concludes the successful response's header section
110-
self.state = .headReceived(promise)
111-
case 407:
112-
let error = HTTPClientError.proxyAuthenticationRequired
113-
self.state = .failed(error)
114-
self.proxyEstablishedPromise?.fail(error)
115-
context.fireErrorCaught(error)
116-
context.close(mode: .all, promise: nil)
117-
118-
default:
119-
// Any response other than a successful response
120-
// indicates that the tunnel has not yet been formed and that the
121-
// connection remains governed by HTTP.
122-
let error = HTTPClientError.invalidProxyResponse
123-
self.state = .failed(error)
124-
self.proxyEstablishedPromise?.fail(error)
125-
context.fireErrorCaught(error)
126-
context.close(mode: .all, promise: nil)
127-
}
99+
self.handleHTTPHeadReceived(head, context: context)
128100
case .body:
129-
switch self.state {
130-
case .headReceived(let timeout):
131-
timeout.cancel()
132-
// we don't expect a body
133-
let error = HTTPClientError.invalidProxyResponse
134-
self.state = .failed(error)
135-
self.proxyEstablishedPromise?.fail(error)
136-
context.fireErrorCaught(error)
137-
context.close(mode: .all, promise: nil)
138-
139-
case .failed:
140-
// ran into an error before... ignore this one
141-
break
142-
case .completed, .connectSent, .initialized:
143-
preconditionFailure("Invalid state: \(self.state)")
144-
}
145-
101+
self.handleHTTPBodyReceived(context: context)
146102
case .end:
147-
switch self.state {
148-
case .headReceived(let timeout):
149-
timeout.cancel()
150-
self.state = .completed
151-
self.proxyEstablishedPromise?.succeed(())
152-
153-
case .failed:
154-
// ran into an error before... ignore this one
155-
break
156-
case .initialized, .connectSent, .completed:
157-
preconditionFailure("Invalid state: \(self.state)")
158-
}
103+
self.handleHTTPEndReceived(context: context)
159104
}
160105
}
161106

162-
func sendConnect(context: ChannelHandlerContext) {
107+
private func sendConnect(context: ChannelHandlerContext) {
163108
guard case .initialized = self.state else {
164109
// we might run into this handler twice, once in handlerAdded and once in channelActive.
165110
return
@@ -171,11 +116,7 @@ final class HTTP1ProxyConnectHandler: ChannelDuplexHandler, RemovableChannelHand
171116
preconditionFailure("How can we have a scheduled timeout, if the connection is not even up?")
172117

173118
case .connectSent, .headReceived:
174-
let error = HTTPClientError.httpProxyHandshakeTimeout
175-
self.state = .failed(error)
176-
self.proxyEstablishedPromise?.fail(error)
177-
context.fireErrorCaught(error)
178-
context.close(mode: .all, promise: nil)
119+
self.failWithError(HTTPClientError.httpProxyHandshakeTimeout, context: context)
179120

180121
case .failed, .completed:
181122
break
@@ -196,4 +137,63 @@ final class HTTP1ProxyConnectHandler: ChannelDuplexHandler, RemovableChannelHand
196137
context.write(self.wrapOutboundOut(.end(nil)), promise: nil)
197138
context.flush()
198139
}
140+
141+
private func handleHTTPHeadReceived(_ head: HTTPResponseHead, context: ChannelHandlerContext) {
142+
guard case .connectSent(let scheduled) = self.state else {
143+
preconditionFailure("HTTPDecoder should throw an error, if we have not send a request")
144+
}
145+
146+
switch head.status.code {
147+
case 200..<300:
148+
// Any 2xx (Successful) response indicates that the sender (and all
149+
// inbound proxies) will switch to tunnel mode immediately after the
150+
// blank line that concludes the successful response's header section
151+
self.state = .headReceived(scheduled)
152+
case 407:
153+
self.failWithError(HTTPClientError.proxyAuthenticationRequired, context: context)
154+
155+
default:
156+
// Any response other than a successful response indicates that the tunnel
157+
// has not yet been formed and that the connection remains governed by HTTP.
158+
self.failWithError(HTTPClientError.invalidProxyResponse, context: context)
159+
}
160+
}
161+
162+
private func handleHTTPBodyReceived(context: ChannelHandlerContext) {
163+
switch self.state {
164+
case .headReceived(let timeout):
165+
timeout.cancel()
166+
// we don't expect a body
167+
self.failWithError(HTTPClientError.invalidProxyResponse, context: context)
168+
case .failed:
169+
// ran into an error before... ignore this one
170+
break
171+
case .completed, .connectSent, .initialized:
172+
preconditionFailure("Invalid state: \(self.state)")
173+
}
174+
}
175+
176+
private func handleHTTPEndReceived(context: ChannelHandlerContext) {
177+
switch self.state {
178+
case .headReceived(let timeout):
179+
timeout.cancel()
180+
self.state = .completed
181+
self.proxyEstablishedPromise?.succeed(())
182+
183+
case .failed:
184+
// ran into an error before... ignore this one
185+
break
186+
case .initialized, .connectSent, .completed:
187+
preconditionFailure("Invalid state: \(self.state)")
188+
}
189+
}
190+
191+
private func failWithError(_ error: Error, context: ChannelHandlerContext, closeConnection: Bool = true) {
192+
self.state = .failed(error)
193+
self.proxyEstablishedPromise?.fail(error)
194+
context.fireErrorCaught(error)
195+
if closeConnection {
196+
context.close(mode: .all, promise: nil)
197+
}
198+
}
199199
}

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ extension HTTPConnectionPool {
4141
self.key = key
4242
self.clientConfiguration = clientConfiguration
4343
self.sslContextCache = sslContextCache
44-
self.tlsConfiguration = tlsConfiguration ?? clientConfiguration.tlsConfiguration ?? .forClient()
44+
self.tlsConfiguration = tlsConfiguration ?? clientConfiguration.tlsConfiguration ?? .makeClientConfiguration()
4545
}
4646
}
4747
}

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

+1
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import XCTest
2525
extension HTTPConnectionPool_FactoryTests {
2626
static var allTests: [(String, (HTTPConnectionPool_FactoryTests) -> () throws -> Void)] {
2727
return [
28+
("testConnectionCreationTimesoutIfDeadlineIsInThePast", testConnectionCreationTimesoutIfDeadlineIsInThePast),
2829
("testSOCKSConnectionCreationTimesoutIfRemoteIsUnresponsive", testSOCKSConnectionCreationTimesoutIfRemoteIsUnresponsive),
2930
("testHTTPProxyConnectionCreationTimesoutIfRemoteIsUnresponsive", testHTTPProxyConnectionCreationTimesoutIfRemoteIsUnresponsive),
3031
("testTLSConnectionCreationTimesoutIfRemoteIsUnresponsive", testTLSConnectionCreationTimesoutIfRemoteIsUnresponsive),

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

+4-1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import Logging
1717
import NIO
1818
import NIOSOCKS
19+
import NIOSSL
1920
import XCTest
2021

2122
class HTTPConnectionPool_FactoryTests: XCTestCase {
@@ -141,10 +142,12 @@ class HTTPConnectionPool_FactoryTests: XCTestCase {
141142

142143
let request = try! HTTPClient.Request(url: "https://localhost:\(server!.localAddress!.port!)")
143144

145+
var tlsConfig = TLSConfiguration.makeClientConfiguration()
146+
tlsConfig.certificateVerification = .none
144147
let factory = HTTPConnectionPool.ConnectionFactory(
145148
key: .init(request),
146149
tlsConfiguration: nil,
147-
clientConfiguration: .init(tlsConfiguration: .forClient(certificateVerification: .none)),
150+
clientConfiguration: .init(tlsConfiguration: tlsConfig),
148151
sslContextCache: .init()
149152
)
150153

0 commit comments

Comments
 (0)