From 939aa2b94735fa219dd40e694bedca813e76573a Mon Sep 17 00:00:00 2001
From: Artem Redkin <aredkin@apple.com>
Date: Mon, 23 Sep 2019 17:03:24 +0100
Subject: [PATCH 01/10] add support for redirect limits
---
Sources/AsyncHTTPClient/HTTPClient.swift | 56 ++++++++++++++++---
Sources/AsyncHTTPClient/HTTPHandler.swift | 23 +++++++-
.../HTTPClientTestUtils.swift | 10 ++++
.../HTTPClientTests.swift | 34 ++++++++++-
4 files changed, 110 insertions(+), 13 deletions(-)
diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift
index 76f59c9ac..09846ce87 100644
--- a/Sources/AsyncHTTPClient/HTTPClient.swift
+++ b/Sources/AsyncHTTPClient/HTTPClient.swift
@@ -221,14 +221,16 @@ public class HTTPClient {
}
private func execute<Delegate: HTTPClientResponseDelegate>(request: Request,
+ redirectLimit: Configuration.RedirectLimit.Next? = nil,
delegate: Delegate,
eventLoop delegateEL: EventLoop,
channelEL: EventLoop? = nil,
deadline: NIODeadline? = nil) -> Task<Delegate.Response> {
let redirectHandler: RedirectHandler<Delegate.Response>?
- if self.configuration.followRedirects {
- redirectHandler = RedirectHandler<Delegate.Response>(request: request) { newRequest in
+ if let next = redirectLimit ?? self.configuration.followRedirects.next {
+ redirectHandler = RedirectHandler<Delegate.Response>(limit: next, request: request) { newRequest, limit in
self.execute(request: newRequest,
+ redirectLimit: limit,
delegate: delegate,
eventLoop: delegateEL,
channelEL: channelEL,
@@ -317,7 +319,7 @@ public class HTTPClient {
/// - `305: Use Proxy`
/// - `307: Temporary Redirect`
/// - `308: Permanent Redirect`
- public var followRedirects: Bool
+ public var followRedirects: FollowRedirects
/// Default client timeout, defaults to no timeouts.
public var timeout: Timeout
/// Upstream proxy, defaults to no proxy.
@@ -325,11 +327,11 @@ public class HTTPClient {
/// Ignore TLS unclean shutdown error, defaults to `false`.
public var ignoreUncleanSSLShutdown: Bool
- public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
+ public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: FollowRedirects = .disabled, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
self.init(tlsConfiguration: tlsConfiguration, followRedirects: followRedirects, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
}
- public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
+ public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: FollowRedirects = .disabled, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
self.tlsConfiguration = tlsConfiguration
self.followRedirects = followRedirects
self.timeout = timeout
@@ -337,11 +339,11 @@ public class HTTPClient {
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
}
- public init(certificateVerification: CertificateVerification, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
+ public init(certificateVerification: CertificateVerification, followRedirects: FollowRedirects = .disabled, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
self.init(certificateVerification: certificateVerification, followRedirects: followRedirects, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
}
- public init(certificateVerification: CertificateVerification, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
+ public init(certificateVerification: CertificateVerification, followRedirects: FollowRedirects = .disabled, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
self.tlsConfiguration = TLSConfiguration.forClient(certificateVerification: certificateVerification)
self.followRedirects = followRedirects
self.timeout = timeout
@@ -423,6 +425,41 @@ extension HTTPClient.Configuration {
self.read = read
}
}
+
+ /// Specifies redirect limit.
+ public struct RedirectLimit {
+ enum Next {
+ case none
+ case loop(visited: Set<URL>)
+ case count(left: Int)
+ }
+
+ var next: Next
+
+ /// No redirect limit.
+ public static let none = RedirectLimit(next: .none)
+ /// Request execution will be stopped if redirect loop is detected.
+ public static let detectLoop = RedirectLimit(next: .loop(visited: Set()))
+ /// Specifies maximum number of redirects for a single request.
+ public static func count(_ value: Int) -> RedirectLimit { return RedirectLimit(next: .count(left: value)) }
+ }
+
+ /// Specifies redirect processing settings.
+ public enum FollowRedirects {
+ /// Redirects are not followed.
+ case disabled
+ /// Redirecets are followed with a specified limit.
+ case enabled(limit: RedirectLimit)
+
+ var next: RedirectLimit.Next? {
+ switch self {
+ case .disabled:
+ return nil
+ case .enabled(let limit):
+ return limit.next
+ }
+ }
+ }
}
private extension ChannelPipeline {
@@ -472,6 +509,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
case invalidProxyResponse
case contentLengthMissing
case proxyAuthenticationRequired
+ case redirectLimitReached
}
private var code: Code
@@ -508,6 +546,8 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
public static let invalidProxyResponse = HTTPClientError(code: .invalidProxyResponse)
/// Request does not contain `Content-Length` header.
public static let contentLengthMissing = HTTPClientError(code: .contentLengthMissing)
- /// Proxy Authentication Required
+ /// Proxy Authentication Required.
public static let proxyAuthenticationRequired = HTTPClientError(code: .proxyAuthenticationRequired)
+ /// Redirect Limit reached.
+ public static let redirectLimitReached = HTTPClientError(code: .redirectLimitReached)
}
diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift
index 3913389a1..6f26c8e61 100644
--- a/Sources/AsyncHTTPClient/HTTPHandler.swift
+++ b/Sources/AsyncHTTPClient/HTTPHandler.swift
@@ -787,8 +787,9 @@ extension TaskHandler: ChannelDuplexHandler {
// MARK: - RedirectHandler
internal struct RedirectHandler<ResponseType> {
+ let limit: HTTPClient.Configuration.RedirectLimit.Next
let request: HTTPClient.Request
- let execute: (HTTPClient.Request) -> HTTPClient.Task<ResponseType>
+ let execute: (HTTPClient.Request, HTTPClient.Configuration.RedirectLimit.Next) -> HTTPClient.Task<ResponseType>
func redirectTarget(status: HTTPResponseStatus, headers: HTTPHeaders) -> URL? {
switch status {
@@ -818,6 +819,24 @@ internal struct RedirectHandler<ResponseType> {
}
func redirect(status: HTTPResponseStatus, to redirectURL: URL, promise: EventLoopPromise<ResponseType>) {
+ let nextLimit: HTTPClient.Configuration.RedirectLimit.Next
+ switch self.limit {
+ case .none:
+ nextLimit = .none
+ case .count(let left):
+ if left == 0 {
+ return promise.fail(HTTPClientError.redirectLimitReached)
+ }
+ nextLimit = .count(left: left - 1)
+ case .loop(let visited):
+ if visited.contains(redirectURL) {
+ return promise.fail(HTTPClientError.redirectLimitReached)
+ }
+ var visited = visited
+ visited.insert(redirectURL)
+ nextLimit = .loop(visited: visited)
+ }
+
let originalRequest = self.request
var convertToGet = false
@@ -847,7 +866,7 @@ internal struct RedirectHandler<ResponseType> {
do {
let newRequest = try HTTPClient.Request(url: redirectURL, method: method, headers: headers, body: body)
- return self.execute(newRequest).futureResult.cascade(to: promise)
+ return self.execute(newRequest, nextLimit).futureResult.cascade(to: promise)
} catch {
return promise.fail(error)
}
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
index 9dd207079..54cdf721d 100644
--- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
+++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
@@ -273,6 +273,16 @@ internal final class HttpBinHandler: ChannelInboundHandler {
headers.add(name: "Location", value: "http://127.0.0.1:\(port)/echohostheader")
self.resps.append(HTTPResponseBuilder(status: .found, headers: headers))
return
+ case "/redirect/infinite1":
+ var headers = HTTPHeaders()
+ headers.add(name: "Location", value: "/redirect/infinite2")
+ self.resps.append(HTTPResponseBuilder(status: .found, headers: headers))
+ return
+ case "/redirect/infinite2":
+ var headers = HTTPHeaders()
+ headers.add(name: "Location", value: "/redirect/infinite1")
+ self.resps.append(HTTPResponseBuilder(status: .found, headers: headers))
+ return
// Since this String is taken from URL.path, the percent encoding has been removed
case "/percent encoded":
if req.method != .GET {
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
index fb8a35fe4..3e40dc5f7 100644
--- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
+++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
@@ -130,7 +130,7 @@ class HTTPClientTests: XCTestCase {
let httpBin = HTTPBin(ssl: false)
let httpsBin = HTTPBin(ssl: true)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
- configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: true))
+ configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: .enabled(limit: .none)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
@@ -148,7 +148,7 @@ class HTTPClientTests: XCTestCase {
func testHttpHostRedirect() throws {
let httpBin = HTTPBin(ssl: false)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
- configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: true))
+ configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: .enabled(limit: .none)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
@@ -525,7 +525,7 @@ class HTTPClientTests: XCTestCase {
let httpBin = HTTPBin()
let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 5)
let httpClient = HTTPClient(eventLoopGroupProvider: .shared(eventLoopGroup),
- configuration: HTTPClient.Configuration(followRedirects: true))
+ configuration: HTTPClient.Configuration(followRedirects: .enabled(limit: .none)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
XCTAssertNoThrow(try eventLoopGroup.syncShutdownGracefully())
@@ -563,4 +563,32 @@ class HTTPClientTests: XCTestCase {
response = try httpClient.execute(request: request, delegate: delegate, eventLoop: .delegate(on: eventLoop)).wait()
XCTAssertEqual(true, response)
}
+
+ func testLoopDetectionRedirectLimit() throws {
+ let httpBin = HTTPBin(ssl: true)
+ let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
+ configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: .enabled(limit: .detectLoop)))
+ defer {
+ XCTAssertNoThrow(try httpClient.syncShutdown())
+ XCTAssertNoThrow(try httpBin.shutdown())
+ }
+
+ XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/redirect/infinite1").wait(), "Should fail with redirect limit") { error in
+ XCTAssertEqual(error as! HTTPClientError, HTTPClientError.redirectLimitReached)
+ }
+ }
+
+ func testCountRedirectLimit() throws {
+ let httpBin = HTTPBin(ssl: true)
+ let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
+ configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: .enabled(limit: .count(5))))
+ defer {
+ XCTAssertNoThrow(try httpClient.syncShutdown())
+ XCTAssertNoThrow(try httpBin.shutdown())
+ }
+
+ XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/redirect/infinite1").wait(), "Should fail with redirect limit") { error in
+ XCTAssertEqual(error as! HTTPClientError, HTTPClientError.redirectLimitReached)
+ }
+ }
}
From b4371119656840df1959e776fb90d8354622f1ee Mon Sep 17 00:00:00 2001
From: Artem Redkin <aredkin@apple.com>
Date: Mon, 30 Sep 2019 14:38:18 +0100
Subject: [PATCH 02/10] add tests on linux
---
Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
index a06c148f1..d03b62ae3 100644
--- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
+++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
@@ -57,6 +57,8 @@ extension HTTPClientTests {
("testWrongContentLengthForSSLUncleanShutdown", testWrongContentLengthForSSLUncleanShutdown),
("testWrongContentLengthWithIgnoreErrorForSSLUncleanShutdown", testWrongContentLengthWithIgnoreErrorForSSLUncleanShutdown),
("testEventLoopArgument", testEventLoopArgument),
+ ("testLoopDetectionRedirectLimit", testLoopDetectionRedirectLimit),
+ ("testCountRedirectLimit", testCountRedirectLimit),
]
}
}
From 99096c5e8f948fd0b598aa5511cc7423594403ed Mon Sep 17 00:00:00 2001
From: Artem Redkin <aredkin@apple.com>
Date: Mon, 30 Sep 2019 16:48:15 +0100
Subject: [PATCH 03/10] review fix: guard instead of if
---
Sources/AsyncHTTPClient/HTTPHandler.swift | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift
index 6f26c8e61..b1f093e53 100644
--- a/Sources/AsyncHTTPClient/HTTPHandler.swift
+++ b/Sources/AsyncHTTPClient/HTTPHandler.swift
@@ -824,7 +824,7 @@ internal struct RedirectHandler<ResponseType> {
case .none:
nextLimit = .none
case .count(let left):
- if left == 0 {
+ guard left > 0 else {
return promise.fail(HTTPClientError.redirectLimitReached)
}
nextLimit = .count(left: left - 1)
From b3ffde6865d592a8ba12f2fe646dd445733ba283 Mon Sep 17 00:00:00 2001
From: Artem Redkin <aredkin@apple.com>
Date: Tue, 1 Oct 2019 12:30:19 +0100
Subject: [PATCH 04/10] review fixes
---
Sources/AsyncHTTPClient/HTTPClient.swift | 68 +++++++------------
Sources/AsyncHTTPClient/HTTPHandler.swift | 30 ++++----
.../HTTPClientTests.swift | 10 +--
3 files changed, 43 insertions(+), 65 deletions(-)
diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift
index 09846ce87..9ff7db684 100644
--- a/Sources/AsyncHTTPClient/HTTPClient.swift
+++ b/Sources/AsyncHTTPClient/HTTPClient.swift
@@ -221,22 +221,24 @@ public class HTTPClient {
}
private func execute<Delegate: HTTPClientResponseDelegate>(request: Request,
- redirectLimit: Configuration.RedirectLimit.Next? = nil,
+ redirectState: Configuration.RedirectPolicy.State? = nil,
delegate: Delegate,
eventLoop delegateEL: EventLoop,
channelEL: EventLoop? = nil,
deadline: NIODeadline? = nil) -> Task<Delegate.Response> {
let redirectHandler: RedirectHandler<Delegate.Response>?
- if let next = redirectLimit ?? self.configuration.followRedirects.next {
- redirectHandler = RedirectHandler<Delegate.Response>(limit: next, request: request) { newRequest, limit in
+ switch self.configuration.redirects {
+ case .allow(let max, let allowCycles):
+ let state = redirectState ?? .init(count: max, visited: allowCycles ? nil : Set())
+ redirectHandler = RedirectHandler<Delegate.Response>(state: state, request: request) { newRequest, state in
self.execute(request: newRequest,
- redirectLimit: limit,
+ redirectState: state,
delegate: delegate,
eventLoop: delegateEL,
channelEL: channelEL,
deadline: deadline)
}
- } else {
+ case .disallow:
redirectHandler = nil
}
@@ -319,7 +321,7 @@ public class HTTPClient {
/// - `305: Use Proxy`
/// - `307: Temporary Redirect`
/// - `308: Permanent Redirect`
- public var followRedirects: FollowRedirects
+ public var redirects: RedirectPolicy
/// Default client timeout, defaults to no timeouts.
public var timeout: Timeout
/// Upstream proxy, defaults to no proxy.
@@ -327,25 +329,25 @@ public class HTTPClient {
/// Ignore TLS unclean shutdown error, defaults to `false`.
public var ignoreUncleanSSLShutdown: Bool
- public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: FollowRedirects = .disabled, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
- self.init(tlsConfiguration: tlsConfiguration, followRedirects: followRedirects, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
+ public init(tlsConfiguration: TLSConfiguration? = nil, redirects: RedirectPolicy = .allow(max: 5, allowCycles: false), timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
+ self.init(tlsConfiguration: tlsConfiguration, redirects: redirects, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
}
- public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: FollowRedirects = .disabled, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
+ public init(tlsConfiguration: TLSConfiguration? = nil, redirects: RedirectPolicy = .allow(max: 5, allowCycles: false), timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
self.tlsConfiguration = tlsConfiguration
- self.followRedirects = followRedirects
+ self.redirects = redirects
self.timeout = timeout
self.proxy = proxy
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
}
- public init(certificateVerification: CertificateVerification, followRedirects: FollowRedirects = .disabled, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
- self.init(certificateVerification: certificateVerification, followRedirects: followRedirects, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
+ public init(certificateVerification: CertificateVerification, redirects: RedirectPolicy = .allow(max: 5, allowCycles: false), timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
+ self.init(certificateVerification: certificateVerification, redirects: redirects, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
}
- public init(certificateVerification: CertificateVerification, followRedirects: FollowRedirects = .disabled, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
+ public init(certificateVerification: CertificateVerification, redirects: RedirectPolicy = .allow(max: 5, allowCycles: false), timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
self.tlsConfiguration = TLSConfiguration.forClient(certificateVerification: certificateVerification)
- self.followRedirects = followRedirects
+ self.redirects = redirects
self.timeout = timeout
self.proxy = proxy
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
@@ -426,38 +428,16 @@ extension HTTPClient.Configuration {
}
}
- /// Specifies redirect limit.
- public struct RedirectLimit {
- enum Next {
- case none
- case loop(visited: Set<URL>)
- case count(left: Int)
- }
-
- var next: Next
-
- /// No redirect limit.
- public static let none = RedirectLimit(next: .none)
- /// Request execution will be stopped if redirect loop is detected.
- public static let detectLoop = RedirectLimit(next: .loop(visited: Set()))
- /// Specifies maximum number of redirects for a single request.
- public static func count(_ value: Int) -> RedirectLimit { return RedirectLimit(next: .count(left: value)) }
- }
-
/// Specifies redirect processing settings.
- public enum FollowRedirects {
+ public enum RedirectPolicy {
/// Redirects are not followed.
- case disabled
- /// Redirecets are followed with a specified limit.
- case enabled(limit: RedirectLimit)
-
- var next: RedirectLimit.Next? {
- switch self {
- case .disabled:
- return nil
- case .enabled(let limit):
- return limit.next
- }
+ case disallow
+ /// Redirects are followed with a specified limit. Cycle detection reqiures that all visited URL's are kept in memory.
+ case allow(max: Int, allowCycles: Bool)
+
+ struct State {
+ var count: Int
+ var visited: Set<URL>?
}
}
}
diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift
index b1f093e53..efb8230a6 100644
--- a/Sources/AsyncHTTPClient/HTTPHandler.swift
+++ b/Sources/AsyncHTTPClient/HTTPHandler.swift
@@ -787,9 +787,9 @@ extension TaskHandler: ChannelDuplexHandler {
// MARK: - RedirectHandler
internal struct RedirectHandler<ResponseType> {
- let limit: HTTPClient.Configuration.RedirectLimit.Next
+ let state: HTTPClient.Configuration.RedirectPolicy.State
let request: HTTPClient.Request
- let execute: (HTTPClient.Request, HTTPClient.Configuration.RedirectLimit.Next) -> HTTPClient.Task<ResponseType>
+ let execute: (HTTPClient.Request, HTTPClient.Configuration.RedirectPolicy.State) -> HTTPClient.Task<ResponseType>
func redirectTarget(status: HTTPResponseStatus, headers: HTTPHeaders) -> URL? {
switch status {
@@ -819,22 +819,20 @@ internal struct RedirectHandler<ResponseType> {
}
func redirect(status: HTTPResponseStatus, to redirectURL: URL, promise: EventLoopPromise<ResponseType>) {
- let nextLimit: HTTPClient.Configuration.RedirectLimit.Next
- switch self.limit {
- case .none:
- nextLimit = .none
- case .count(let left):
- guard left > 0 else {
- return promise.fail(HTTPClientError.redirectLimitReached)
- }
- nextLimit = .count(left: left - 1)
- case .loop(let visited):
- if visited.contains(redirectURL) {
+ var newState = self.state
+ guard newState.count > 0 else {
+ return promise.fail(HTTPClientError.redirectLimitReached)
+ }
+
+ newState.count -= 1
+
+ if var visited = newState.visited {
+ guard !visited.contains(redirectURL) else {
return promise.fail(HTTPClientError.redirectLimitReached)
}
- var visited = visited
+
visited.insert(redirectURL)
- nextLimit = .loop(visited: visited)
+ newState.visited = visited
}
let originalRequest = self.request
@@ -866,7 +864,7 @@ internal struct RedirectHandler<ResponseType> {
do {
let newRequest = try HTTPClient.Request(url: redirectURL, method: method, headers: headers, body: body)
- return self.execute(newRequest, nextLimit).futureResult.cascade(to: promise)
+ return self.execute(newRequest, newState).futureResult.cascade(to: promise)
} catch {
return promise.fail(error)
}
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
index 3e40dc5f7..615269e14 100644
--- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
+++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
@@ -130,7 +130,7 @@ class HTTPClientTests: XCTestCase {
let httpBin = HTTPBin(ssl: false)
let httpsBin = HTTPBin(ssl: true)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
- configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: .enabled(limit: .none)))
+ configuration: HTTPClient.Configuration(certificateVerification: .none, redirects: .allow(max: 10, allowCycles: true)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
@@ -148,7 +148,7 @@ class HTTPClientTests: XCTestCase {
func testHttpHostRedirect() throws {
let httpBin = HTTPBin(ssl: false)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
- configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: .enabled(limit: .none)))
+ configuration: HTTPClient.Configuration(certificateVerification: .none, redirects: .allow(max: 10, allowCycles: true)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
@@ -525,7 +525,7 @@ class HTTPClientTests: XCTestCase {
let httpBin = HTTPBin()
let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 5)
let httpClient = HTTPClient(eventLoopGroupProvider: .shared(eventLoopGroup),
- configuration: HTTPClient.Configuration(followRedirects: .enabled(limit: .none)))
+ configuration: HTTPClient.Configuration(redirects: .allow(max: 10, allowCycles: true)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
XCTAssertNoThrow(try eventLoopGroup.syncShutdownGracefully())
@@ -567,7 +567,7 @@ class HTTPClientTests: XCTestCase {
func testLoopDetectionRedirectLimit() throws {
let httpBin = HTTPBin(ssl: true)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
- configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: .enabled(limit: .detectLoop)))
+ configuration: HTTPClient.Configuration(certificateVerification: .none, redirects: .allow(max: 5, allowCycles: false)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
XCTAssertNoThrow(try httpBin.shutdown())
@@ -581,7 +581,7 @@ class HTTPClientTests: XCTestCase {
func testCountRedirectLimit() throws {
let httpBin = HTTPBin(ssl: true)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
- configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: .enabled(limit: .count(5))))
+ configuration: HTTPClient.Configuration(certificateVerification: .none, redirects: .allow(max: 5, allowCycles: true)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
XCTAssertNoThrow(try httpBin.shutdown())
From bbad11ffcbfc42aa955118c119f0c1ed4e71e836 Mon Sep 17 00:00:00 2001
From: Artem Redkin <aredkin@apple.com>
Date: Tue, 8 Oct 2019 13:18:02 +0100
Subject: [PATCH 05/10] review fix: make redirect state be part for request
---
Sources/AsyncHTTPClient/HTTPClient.swift | 14 +++-----
Sources/AsyncHTTPClient/HTTPHandler.swift | 41 +++++++++++++++--------
2 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift
index 9ff7db684..272b35065 100644
--- a/Sources/AsyncHTTPClient/HTTPClient.swift
+++ b/Sources/AsyncHTTPClient/HTTPClient.swift
@@ -221,7 +221,6 @@ public class HTTPClient {
}
private func execute<Delegate: HTTPClientResponseDelegate>(request: Request,
- redirectState: Configuration.RedirectPolicy.State? = nil,
delegate: Delegate,
eventLoop delegateEL: EventLoop,
channelEL: EventLoop? = nil,
@@ -229,10 +228,12 @@ public class HTTPClient {
let redirectHandler: RedirectHandler<Delegate.Response>?
switch self.configuration.redirects {
case .allow(let max, let allowCycles):
- let state = redirectState ?? .init(count: max, visited: allowCycles ? nil : Set())
- redirectHandler = RedirectHandler<Delegate.Response>(state: state, request: request) { newRequest, state in
+ var request = request
+ if request.redirectState == nil {
+ request.redirectState = .init(count: max, visited: allowCycles ? nil : Set())
+ }
+ redirectHandler = RedirectHandler<Delegate.Response>(request: request) { newRequest in
self.execute(request: newRequest,
- redirectState: state,
delegate: delegate,
eventLoop: delegateEL,
channelEL: channelEL,
@@ -434,11 +435,6 @@ extension HTTPClient.Configuration {
case disallow
/// Redirects are followed with a specified limit. Cycle detection reqiures that all visited URL's are kept in memory.
case allow(max: Int, allowCycles: Bool)
-
- struct State {
- var count: Int
- var visited: Set<URL>?
- }
}
}
diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift
index efb8230a6..0e6529d8b 100644
--- a/Sources/AsyncHTTPClient/HTTPHandler.swift
+++ b/Sources/AsyncHTTPClient/HTTPHandler.swift
@@ -100,6 +100,13 @@ extension HTTPClient {
/// Request body, defaults to no body.
public var body: Body?
+ struct RedirectState {
+ var count: Int
+ var visited: Set<URL>?
+ }
+
+ var redirectState: RedirectState?
+
/// Create HTTP request.
///
/// - parameters:
@@ -152,6 +159,8 @@ extension HTTPClient {
self.host = host
self.headers = headers
self.body = body
+
+ self.redirectState = nil
}
/// Whether request will be executed using secure socket.
@@ -787,9 +796,8 @@ extension TaskHandler: ChannelDuplexHandler {
// MARK: - RedirectHandler
internal struct RedirectHandler<ResponseType> {
- let state: HTTPClient.Configuration.RedirectPolicy.State
let request: HTTPClient.Request
- let execute: (HTTPClient.Request, HTTPClient.Configuration.RedirectPolicy.State) -> HTTPClient.Task<ResponseType>
+ let execute: (HTTPClient.Request) -> HTTPClient.Task<ResponseType>
func redirectTarget(status: HTTPResponseStatus, headers: HTTPHeaders) -> URL? {
switch status {
@@ -819,20 +827,24 @@ internal struct RedirectHandler<ResponseType> {
}
func redirect(status: HTTPResponseStatus, to redirectURL: URL, promise: EventLoopPromise<ResponseType>) {
- var newState = self.state
- guard newState.count > 0 else {
- return promise.fail(HTTPClientError.redirectLimitReached)
- }
+ var nextState: HTTPClient.Request.RedirectState?
+ if var state = request.redirectState {
+ guard state.count > 0 else {
+ return promise.fail(HTTPClientError.redirectLimitReached)
+ }
- newState.count -= 1
+ state.count -= 1
- if var visited = newState.visited {
- guard !visited.contains(redirectURL) else {
- return promise.fail(HTTPClientError.redirectLimitReached)
+ if var visited = state.visited {
+ guard !visited.contains(redirectURL) else {
+ return promise.fail(HTTPClientError.redirectLimitReached)
+ }
+
+ visited.insert(redirectURL)
+ state.visited = visited
}
- visited.insert(redirectURL)
- newState.visited = visited
+ nextState = state
}
let originalRequest = self.request
@@ -863,8 +875,9 @@ internal struct RedirectHandler<ResponseType> {
}
do {
- let newRequest = try HTTPClient.Request(url: redirectURL, method: method, headers: headers, body: body)
- return self.execute(newRequest, newState).futureResult.cascade(to: promise)
+ var newRequest = try HTTPClient.Request(url: redirectURL, method: method, headers: headers, body: body)
+ newRequest.redirectState = nextState
+ return self.execute(newRequest).futureResult.cascade(to: promise)
} catch {
return promise.fail(error)
}
From 8b0e613b00c40016a4550f7fcb738e88a6380fa0 Mon Sep 17 00:00:00 2001
From: Artem Redkin <aredkin@apple.com>
Date: Tue, 8 Oct 2019 13:22:23 +0100
Subject: [PATCH 06/10] review fix: add a separate error for redirect cycles
---
Sources/AsyncHTTPClient/HTTPClient.swift | 3 +++
Sources/AsyncHTTPClient/HTTPHandler.swift | 2 +-
Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 2 +-
3 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift
index 272b35065..5bbefa6dc 100644
--- a/Sources/AsyncHTTPClient/HTTPClient.swift
+++ b/Sources/AsyncHTTPClient/HTTPClient.swift
@@ -486,6 +486,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
case contentLengthMissing
case proxyAuthenticationRequired
case redirectLimitReached
+ case redirectCycleDetected
}
private var code: Code
@@ -526,4 +527,6 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
public static let proxyAuthenticationRequired = HTTPClientError(code: .proxyAuthenticationRequired)
/// Redirect Limit reached.
public static let redirectLimitReached = HTTPClientError(code: .redirectLimitReached)
+ /// Redirect Cycle detected.
+ public static let redirectCycleDetected = HTTPClientError(code: .redirectCycleDetected)
}
diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift
index 0e6529d8b..47a85def2 100644
--- a/Sources/AsyncHTTPClient/HTTPHandler.swift
+++ b/Sources/AsyncHTTPClient/HTTPHandler.swift
@@ -837,7 +837,7 @@ internal struct RedirectHandler<ResponseType> {
if var visited = state.visited {
guard !visited.contains(redirectURL) else {
- return promise.fail(HTTPClientError.redirectLimitReached)
+ return promise.fail(HTTPClientError.redirectCycleDetected)
}
visited.insert(redirectURL)
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
index 615269e14..41e797ad7 100644
--- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
+++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
@@ -574,7 +574,7 @@ class HTTPClientTests: XCTestCase {
}
XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/redirect/infinite1").wait(), "Should fail with redirect limit") { error in
- XCTAssertEqual(error as! HTTPClientError, HTTPClientError.redirectLimitReached)
+ XCTAssertEqual(error as! HTTPClientError, HTTPClientError.redirectCycleDetected)
}
}
From aa843dea4465fd4ba660fc6b361d7677b35c17ec Mon Sep 17 00:00:00 2001
From: Artem Redkin <aredkin@apple.com>
Date: Fri, 11 Oct 2019 13:19:26 +0100
Subject: [PATCH 07/10] Update Sources/AsyncHTTPClient/HTTPClient.swift
Co-Authored-By: Johannes Weiss <johannesweiss@apple.com>
---
Sources/AsyncHTTPClient/HTTPClient.swift | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift
index 5bbefa6dc..bb5a1340a 100644
--- a/Sources/AsyncHTTPClient/HTTPClient.swift
+++ b/Sources/AsyncHTTPClient/HTTPClient.swift
@@ -433,7 +433,13 @@ extension HTTPClient.Configuration {
public enum RedirectPolicy {
/// Redirects are not followed.
case disallow
- /// Redirects are followed with a specified limit. Cycle detection reqiures that all visited URL's are kept in memory.
+ /// Redirects are followed with a specified limit.
+ ///
+ /// - parameters:
+ /// - max: The maximum number of allowed redirects.
+ /// - allowCycles: Whether cycles are allowed.
+ ///
+ /// - warning: Cycle detection will keep all visited URLs in memory which means a malicious server could use this as a denial-of-service vector.
case allow(max: Int, allowCycles: Bool)
}
}
From 7e2edf88aa77f401c277533fbbdf023062f15f9f Mon Sep 17 00:00:00 2001
From: Artem Redkin <aredkin@apple.com>
Date: Fri, 11 Oct 2019 14:54:08 +0100
Subject: [PATCH 08/10] review fixes
---
Sources/AsyncHTTPClient/HTTPClient.swift | 46 +++++++++++++------
.../HTTPClientTests.swift | 10 ++--
2 files changed, 37 insertions(+), 19 deletions(-)
diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift
index bb5a1340a..245738936 100644
--- a/Sources/AsyncHTTPClient/HTTPClient.swift
+++ b/Sources/AsyncHTTPClient/HTTPClient.swift
@@ -226,8 +226,8 @@ public class HTTPClient {
channelEL: EventLoop? = nil,
deadline: NIODeadline? = nil) -> Task<Delegate.Response> {
let redirectHandler: RedirectHandler<Delegate.Response>?
- switch self.configuration.redirects {
- case .allow(let max, let allowCycles):
+ switch self.configuration.redirectConfiguration.configuration {
+ case .follow(let max, let allowCycles):
var request = request
if request.redirectState == nil {
request.redirectState = .init(count: max, visited: allowCycles ? nil : Set())
@@ -322,7 +322,7 @@ public class HTTPClient {
/// - `305: Use Proxy`
/// - `307: Temporary Redirect`
/// - `308: Permanent Redirect`
- public var redirects: RedirectPolicy
+ public var redirectConfiguration: RedirectConfiguration
/// Default client timeout, defaults to no timeouts.
public var timeout: Timeout
/// Upstream proxy, defaults to no proxy.
@@ -330,25 +330,25 @@ public class HTTPClient {
/// Ignore TLS unclean shutdown error, defaults to `false`.
public var ignoreUncleanSSLShutdown: Bool
- public init(tlsConfiguration: TLSConfiguration? = nil, redirects: RedirectPolicy = .allow(max: 5, allowCycles: false), timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
- self.init(tlsConfiguration: tlsConfiguration, redirects: redirects, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
+ public init(tlsConfiguration: TLSConfiguration? = nil, redirectConfiguration: RedirectConfiguration? = nil, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
+ self.init(tlsConfiguration: tlsConfiguration, redirectConfiguration: redirectConfiguration, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
}
- public init(tlsConfiguration: TLSConfiguration? = nil, redirects: RedirectPolicy = .allow(max: 5, allowCycles: false), timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
+ public init(tlsConfiguration: TLSConfiguration? = nil, redirectConfiguration: RedirectConfiguration? = nil, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
self.tlsConfiguration = tlsConfiguration
- self.redirects = redirects
+ self.redirectConfiguration = redirectConfiguration ?? RedirectConfiguration()
self.timeout = timeout
self.proxy = proxy
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
}
- public init(certificateVerification: CertificateVerification, redirects: RedirectPolicy = .allow(max: 5, allowCycles: false), timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
- self.init(certificateVerification: certificateVerification, redirects: redirects, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
+ public init(certificateVerification: CertificateVerification, redirectConfiguration: RedirectConfiguration? = nil, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
+ self.init(certificateVerification: certificateVerification, redirectConfiguration: redirectConfiguration, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
}
- public init(certificateVerification: CertificateVerification, redirects: RedirectPolicy = .allow(max: 5, allowCycles: false), timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
+ public init(certificateVerification: CertificateVerification, redirectConfiguration: RedirectConfiguration? = nil, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
self.tlsConfiguration = TLSConfiguration.forClient(certificateVerification: certificateVerification)
- self.redirects = redirects
+ self.redirectConfiguration = redirectConfiguration ?? RedirectConfiguration()
self.timeout = timeout
self.proxy = proxy
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
@@ -430,9 +430,27 @@ extension HTTPClient.Configuration {
}
/// Specifies redirect processing settings.
- public enum RedirectPolicy {
+ public struct RedirectConfiguration {
+ enum Configuration {
+ /// Redirects are not followed.
+ case disallow
+ /// Redirects are followed with a specified limit.
+ case follow(max: Int, allowCycles: Bool)
+ }
+
+ var configuration: Configuration
+
+ init() {
+ self.configuration = .follow(max: 5, allowCycles: false)
+ }
+
+ init(configuration: Configuration) {
+ self.configuration = configuration
+ }
+
/// Redirects are not followed.
- case disallow
+ public static let disallow = RedirectConfiguration(configuration: .disallow)
+
/// Redirects are followed with a specified limit.
///
/// - parameters:
@@ -440,7 +458,7 @@ extension HTTPClient.Configuration {
/// - allowCycles: Whether cycles are allowed.
///
/// - warning: Cycle detection will keep all visited URLs in memory which means a malicious server could use this as a denial-of-service vector.
- case allow(max: Int, allowCycles: Bool)
+ public static func follow(max: Int, allowCycles: Bool) -> RedirectConfiguration { return .init(configuration: .follow(max: max, allowCycles: allowCycles)) }
}
}
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
index 41e797ad7..9e1931a0c 100644
--- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
+++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
@@ -130,7 +130,7 @@ class HTTPClientTests: XCTestCase {
let httpBin = HTTPBin(ssl: false)
let httpsBin = HTTPBin(ssl: true)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
- configuration: HTTPClient.Configuration(certificateVerification: .none, redirects: .allow(max: 10, allowCycles: true)))
+ configuration: HTTPClient.Configuration(certificateVerification: .none, redirectConfiguration: .follow(max: 10, allowCycles: true)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
@@ -148,7 +148,7 @@ class HTTPClientTests: XCTestCase {
func testHttpHostRedirect() throws {
let httpBin = HTTPBin(ssl: false)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
- configuration: HTTPClient.Configuration(certificateVerification: .none, redirects: .allow(max: 10, allowCycles: true)))
+ configuration: HTTPClient.Configuration(certificateVerification: .none, redirectConfiguration: .follow(max: 10, allowCycles: true)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
@@ -525,7 +525,7 @@ class HTTPClientTests: XCTestCase {
let httpBin = HTTPBin()
let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 5)
let httpClient = HTTPClient(eventLoopGroupProvider: .shared(eventLoopGroup),
- configuration: HTTPClient.Configuration(redirects: .allow(max: 10, allowCycles: true)))
+ configuration: HTTPClient.Configuration(redirectConfiguration: .follow(max: 10, allowCycles: true)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
XCTAssertNoThrow(try eventLoopGroup.syncShutdownGracefully())
@@ -567,7 +567,7 @@ class HTTPClientTests: XCTestCase {
func testLoopDetectionRedirectLimit() throws {
let httpBin = HTTPBin(ssl: true)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
- configuration: HTTPClient.Configuration(certificateVerification: .none, redirects: .allow(max: 5, allowCycles: false)))
+ configuration: HTTPClient.Configuration(certificateVerification: .none, redirectConfiguration: .follow(max: 5, allowCycles: false)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
XCTAssertNoThrow(try httpBin.shutdown())
@@ -581,7 +581,7 @@ class HTTPClientTests: XCTestCase {
func testCountRedirectLimit() throws {
let httpBin = HTTPBin(ssl: true)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
- configuration: HTTPClient.Configuration(certificateVerification: .none, redirects: .allow(max: 5, allowCycles: true)))
+ configuration: HTTPClient.Configuration(certificateVerification: .none, redirectConfiguration: .follow(max: 5, allowCycles: true)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
XCTAssertNoThrow(try httpBin.shutdown())
From 5209dc38e7dff0d8a9633cdf12e73c97ae48975f Mon Sep 17 00:00:00 2001
From: Artem Redkin <aredkin@apple.com>
Date: Mon, 14 Oct 2019 11:08:10 +0100
Subject: [PATCH 09/10] rewrite backpressure test
---
Sources/AsyncHTTPClient/HTTPHandler.swift | 2 +-
.../HTTPClientInternalTests.swift | 51 ++++++++++++++-----
.../HTTPClientTestUtils.swift | 2 +-
3 files changed, 41 insertions(+), 14 deletions(-)
diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift
index 47a85def2..79c9cafe4 100644
--- a/Sources/AsyncHTTPClient/HTTPHandler.swift
+++ b/Sources/AsyncHTTPClient/HTTPHandler.swift
@@ -456,7 +456,7 @@ extension HTTPClient {
public let eventLoop: EventLoop
let promise: EventLoopPromise<Response>
- private var channel: Channel?
+ var channel: Channel?
private var cancelled: Bool
private let lock: Lock
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift
index f75bbb087..614d6afa1 100644
--- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift
+++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift
@@ -155,17 +155,29 @@ class HTTPClientInternalTests: XCTestCase {
XCTAssertThrowsError(try httpClient.post(url: "http://localhost:\(httpBin.port)/post", body: body).wait())
}
+ // In order to test backpressure we need to make sure that reads will not happen
+ // until the backpressure promise is succeeded. Since we cannot guarantee when
+ // messages will be delivered to a client pipeline and we need this test to be
+ // fast (no waiting for arbitrary amounts of time), we do the following.
+ // First, we enforce NIO to send us only 1 byte at a time. Then we send a message
+ // of 4 bytes. This will guarantee that if we see first byte of the message, other
+ // bytes a ready to be read as well. This will allow us to test if subsequent reads
+ // are waiting for backpressure promise.
func testUploadStreamingBackpressure() throws {
class BackpressureTestDelegate: HTTPClientResponseDelegate {
typealias Response = Void
var _reads = 0
let lock: Lock
- let promise: EventLoopPromise<Void>
+ let backpressurePromise: EventLoopPromise<Void>
+ let optionsApplied: EventLoopPromise<Void>
+ let messageReceived: EventLoopPromise<Void>
- init(promise: EventLoopPromise<Void>) {
+ init(eventLoop: EventLoop) {
self.lock = Lock()
- self.promise = promise
+ self.backpressurePromise = eventLoop.makePromise()
+ self.optionsApplied = eventLoop.makePromise()
+ self.messageReceived = eventLoop.makePromise()
}
var reads: Int {
@@ -174,18 +186,30 @@ class HTTPClientInternalTests: XCTestCase {
}
}
+ func didReceiveHead(task: HTTPClient.Task<Void>, _ head: HTTPResponseHead) -> EventLoopFuture<Void> {
+ // This is to force NIO to send only 1 byte at a time.
+ let future = task.channel!.setOption(ChannelOptions.maxMessagesPerRead, value: 1).flatMap {
+ task.channel!.setOption(ChannelOptions.recvAllocator, value: FixedSizeRecvByteBufferAllocator(capacity: 1))
+ }
+ future.cascade(to: self.optionsApplied)
+ return future
+ }
+
func didReceiveBodyPart(task: HTTPClient.Task<Response>, _ buffer: ByteBuffer) -> EventLoopFuture<Void> {
+ // We count a number of reads received.
self.lock.withLockVoid {
self._reads += 1
}
- return self.promise.futureResult
+ // We need to notify the test when first byte of the message is arrived.
+ self.messageReceived.succeed(())
+ return self.backpressurePromise.futureResult
}
func didFinishRequest(task: HTTPClient.Task<Response>) throws {}
}
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew)
- let promise: EventLoopPromise<Channel> = httpClient.eventLoopGroup.next().makePromise()
+ let promise = httpClient.eventLoopGroup.next().makePromise(of: Channel.self)
let httpBin = HTTPBin(channelPromise: promise)
defer {
@@ -194,27 +218,30 @@ class HTTPClientInternalTests: XCTestCase {
}
let request = try Request(url: "http://localhost:\(httpBin.port)/custom")
- let delegate = BackpressureTestDelegate(promise: httpClient.eventLoopGroup.next().makePromise())
+ let delegate = BackpressureTestDelegate(eventLoop: httpClient.eventLoopGroup.next())
let future = httpClient.execute(request: request, delegate: delegate).futureResult
let channel = try promise.futureResult.wait()
+ // We need to wait for channel options that limit NIO to sending only one byte at a time.
+ try delegate.optionsApplied.futureResult.wait()
- // Send 3 parts, but only one should be received until the future is complete
+ // Send 4 bytes, but only one should be received until the backpressure promise is succeeded.
let buffer = ByteBuffer.of(string: "1234")
try channel.writeAndFlush(HTTPServerResponsePart.body(.byteBuffer(buffer))).wait()
- try channel.writeAndFlush(HTTPServerResponsePart.body(.byteBuffer(buffer))).wait()
- try channel.writeAndFlush(HTTPServerResponsePart.body(.byteBuffer(buffer))).wait()
+ // Now we wait until message is delivered to client channel pipeline
+ try delegate.messageReceived.futureResult.wait()
XCTAssertEqual(delegate.reads, 1)
- delegate.promise.succeed(())
+ // Succeed the backpressure promise.
+ delegate.backpressurePromise.succeed(())
try channel.writeAndFlush(HTTPServerResponsePart.end(nil)).wait()
try future.wait()
- XCTAssertEqual(delegate.reads, 3)
+ // At this point all other bytes should be delivered.
+ XCTAssertEqual(delegate.reads, 4)
}
-
func testRequestURITrailingSlash() throws {
let request1 = try Request(url: "https://someserver.com:8888/some/path?foo=bar#ref")
XCTAssertEqual(request1.url.uri, "/some/path?foo=bar")
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
index 54cdf721d..1a51f4fe1 100644
--- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
+++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
@@ -305,7 +305,7 @@ internal final class HttpBinHandler: ChannelInboundHandler {
context.close(promise: nil)
return
case "/custom":
- context.write(wrapOutboundOut(.head(HTTPResponseHead(version: HTTPVersion(major: 1, minor: 1), status: .ok))), promise: nil)
+ context.writeAndFlush(wrapOutboundOut(.head(HTTPResponseHead(version: HTTPVersion(major: 1, minor: 1), status: .ok))), promise: nil)
return
case "/events/10/1": // TODO: parse path
context.write(wrapOutboundOut(.head(HTTPResponseHead(version: HTTPVersion(major: 1, minor: 1), status: .ok))), promise: nil)
From cf2d4d615f9e2b3bd6ea6f7f4b2cd8c7d206cee9 Mon Sep 17 00:00:00 2001
From: Artem Redkin <aredkin@apple.com>
Date: Mon, 14 Oct 2019 11:12:39 +0100
Subject: [PATCH 10/10] fix formatting
---
Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift | 1 +
1 file changed, 1 insertion(+)
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift
index 614d6afa1..40ebad75e 100644
--- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift
+++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift
@@ -242,6 +242,7 @@ class HTTPClientInternalTests: XCTestCase {
// At this point all other bytes should be delivered.
XCTAssertEqual(delegate.reads, 4)
}
+
func testRequestURITrailingSlash() throws {
let request1 = try Request(url: "https://someserver.com:8888/some/path?foo=bar#ref")
XCTAssertEqual(request1.url.uri, "/some/path?foo=bar")