From 9d8aa243f207a9d7dce2ebf8b9f4def9fbf9e8ae Mon Sep 17 00:00:00 2001 From: Adam Fowler Date: Tue, 23 Jun 2020 22:51:27 +0100 Subject: [PATCH 1/5] Add TestBackpressure test --- .../HTTPClientTests.swift | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index df144b70b..61aa1312c 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2533,4 +2533,59 @@ class HTTPClientTests: XCTestCase { XCTAssertEqual(info.connectionNumber, 1) XCTAssertEqual(info.requestNumber, 1) } + + func testBackpressue() { + class BackpressureResponseDelegate: HTTPClientResponseDelegate { + typealias Response = Void + var count = 0 + var processingBodyPart = false + var didntWait = false + var lock = Lock() + + init() { } + + func didReceiveHead(task: HTTPClient.Task, _ head: HTTPResponseHead) -> EventLoopFuture { + return task.eventLoop.makeSucceededFuture(()) + } + + func didReceiveBodyPart(task: HTTPClient.Task, _ part: ByteBuffer) -> EventLoopFuture { + lock.withLock { + // if processingBodyPart is true then previous body part is still being processed + // XCTAssertEqual doesn't work here so store result to test later + if processingBodyPart == true { + didntWait = true + } + processingBodyPart = true + count += 1 + } + // wait one second before returning a successful future + return task.eventLoop.scheduleTask(in: .milliseconds(1000) ) { + self.lock.withLock { + self.processingBodyPart = false + self.count -= 1 + } + }.futureResult + } + + func didReceiveError(task: HTTPClient.Task, _ error: Error) { } + func didFinishRequest(task: HTTPClient.Task) throws { } + } + + let elg = MultiThreadedEventLoopGroup(numberOfThreads: 5) + let client = HTTPClient(eventLoopGroupProvider: .shared(elg)) + defer { + XCTAssertNoThrow(try client.syncShutdown()) + XCTAssertNoThrow(try elg.syncShutdownGracefully()) + } + + let data = Data(count: 65273) + let backpressureResponseDelegate = BackpressureResponseDelegate() + guard let request = try? HTTPClient.Request(url: self.defaultHTTPBinURLPrefix + "get", body: .data(data)) else { + XCTFail("Failed to init Request") + return + } + XCTAssertNoThrow(try client.execute(request: request, delegate: backpressureResponseDelegate).wait()) + XCTAssertEqual(backpressureResponseDelegate.didntWait, false) + XCTAssertEqual(backpressureResponseDelegate.count, 0) + } } From df778c582b970d638ac49cb6bd9678b2db48cc18 Mon Sep 17 00:00:00 2001 From: Adam Fowler Date: Wed, 24 Jun 2020 11:40:03 +0100 Subject: [PATCH 2/5] Include tests, Sanity fixes --- .../AsyncHTTPClientTests/HTTPClientTests+XCTest.swift | 1 + Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 10 +++++----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 5fe7b2417..957117c8a 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -119,6 +119,7 @@ extension HTTPClientTests { ("testDelegateCallinsTolerateRandomEL", testDelegateCallinsTolerateRandomEL), ("testContentLengthTooLongFails", testContentLengthTooLongFails), ("testContentLengthTooShortFails", testContentLengthTooShortFails), + ("testBackpressue", testBackpressue), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 61aa1312c..8a986c678 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2542,14 +2542,14 @@ class HTTPClientTests: XCTestCase { var didntWait = false var lock = Lock() - init() { } + init() {} func didReceiveHead(task: HTTPClient.Task, _ head: HTTPResponseHead) -> EventLoopFuture { return task.eventLoop.makeSucceededFuture(()) } func didReceiveBodyPart(task: HTTPClient.Task, _ part: ByteBuffer) -> EventLoopFuture { - lock.withLock { + self.lock.withLock { // if processingBodyPart is true then previous body part is still being processed // XCTAssertEqual doesn't work here so store result to test later if processingBodyPart == true { @@ -2559,7 +2559,7 @@ class HTTPClientTests: XCTestCase { count += 1 } // wait one second before returning a successful future - return task.eventLoop.scheduleTask(in: .milliseconds(1000) ) { + return task.eventLoop.scheduleTask(in: .milliseconds(1000)) { self.lock.withLock { self.processingBodyPart = false self.count -= 1 @@ -2567,8 +2567,8 @@ class HTTPClientTests: XCTestCase { }.futureResult } - func didReceiveError(task: HTTPClient.Task, _ error: Error) { } - func didFinishRequest(task: HTTPClient.Task) throws { } + func didReceiveError(task: HTTPClient.Task, _ error: Error) {} + func didFinishRequest(task: HTTPClient.Task) throws {} } let elg = MultiThreadedEventLoopGroup(numberOfThreads: 5) From 19accd7d4188f0dda0bfe814501cea0ed8d887eb Mon Sep 17 00:00:00 2001 From: Adam Fowler Date: Wed, 24 Jun 2020 11:47:23 +0100 Subject: [PATCH 3/5] Reduce wait time in testBackpressure --- Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift | 2 +- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 957117c8a..8df1206f5 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -119,7 +119,7 @@ extension HTTPClientTests { ("testDelegateCallinsTolerateRandomEL", testDelegateCallinsTolerateRandomEL), ("testContentLengthTooLongFails", testContentLengthTooLongFails), ("testContentLengthTooShortFails", testContentLengthTooShortFails), - ("testBackpressue", testBackpressue), + ("testBackpressure", testBackpressure), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 8a986c678..a2e6e0b70 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2534,7 +2534,7 @@ class HTTPClientTests: XCTestCase { XCTAssertEqual(info.requestNumber, 1) } - func testBackpressue() { + func testBackpressure() { class BackpressureResponseDelegate: HTTPClientResponseDelegate { typealias Response = Void var count = 0 @@ -2559,7 +2559,7 @@ class HTTPClientTests: XCTestCase { count += 1 } // wait one second before returning a successful future - return task.eventLoop.scheduleTask(in: .milliseconds(1000)) { + return task.eventLoop.scheduleTask(in: .milliseconds(200)) { self.lock.withLock { self.processingBodyPart = false self.count -= 1 From 8611b6191072bd59c82abdfc60243b31aa1ea89c Mon Sep 17 00:00:00 2001 From: Adam Fowler Date: Wed, 24 Jun 2020 15:13:57 +0100 Subject: [PATCH 4/5] Changes from review Added HTTPBin path "/zeros/100000" --- Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift | 7 +++++++ .../AsyncHTTPClientTests/HTTPClientTests+XCTest.swift | 2 +- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 10 ++++++---- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 6beba8938..5ab4432a2 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -541,6 +541,13 @@ internal final class HttpBinHandler: ChannelInboundHandler { } context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil) return + case "/zeros/100000": + let buf = context.channel.allocator.buffer(repeating: 0, count: 100_000) + context.write(wrapOutboundOut(.head(HTTPResponseHead(version: HTTPVersion(major: 1, minor: 1), status: .ok))), promise: nil) + context.writeAndFlush(wrapOutboundOut(.body(.byteBuffer(buf))), promise: nil) + context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil) + return + default: context.write(wrapOutboundOut(.head(HTTPResponseHead(version: HTTPVersion(major: 1, minor: 1), status: .notFound))), promise: nil) context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 8df1206f5..51da398a9 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -119,7 +119,7 @@ extension HTTPClientTests { ("testDelegateCallinsTolerateRandomEL", testDelegateCallinsTolerateRandomEL), ("testContentLengthTooLongFails", testContentLengthTooLongFails), ("testContentLengthTooShortFails", testContentLengthTooShortFails), - ("testBackpressure", testBackpressure), + ("testDownloadBackpressure", testDownloadBackpressure), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index a2e6e0b70..3e4f4d8c9 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2534,10 +2534,11 @@ class HTTPClientTests: XCTestCase { XCTAssertEqual(info.requestNumber, 1) } - func testBackpressure() { + func testDownloadBackpressure() { class BackpressureResponseDelegate: HTTPClientResponseDelegate { typealias Response = Void var count = 0 + var totalCount = 0 var processingBodyPart = false var didntWait = false var lock = Lock() @@ -2557,6 +2558,7 @@ class HTTPClientTests: XCTestCase { } processingBodyPart = true count += 1 + totalCount += 1 } // wait one second before returning a successful future return task.eventLoop.scheduleTask(in: .milliseconds(200)) { @@ -2571,21 +2573,21 @@ class HTTPClientTests: XCTestCase { func didFinishRequest(task: HTTPClient.Task) throws {} } - let elg = MultiThreadedEventLoopGroup(numberOfThreads: 5) + let elg = MultiThreadedEventLoopGroup(numberOfThreads: 3) let client = HTTPClient(eventLoopGroupProvider: .shared(elg)) defer { XCTAssertNoThrow(try client.syncShutdown()) XCTAssertNoThrow(try elg.syncShutdownGracefully()) } - let data = Data(count: 65273) let backpressureResponseDelegate = BackpressureResponseDelegate() - guard let request = try? HTTPClient.Request(url: self.defaultHTTPBinURLPrefix + "get", body: .data(data)) else { + guard let request = try? HTTPClient.Request(url: self.defaultHTTPBinURLPrefix + "zeros/100000") else { XCTFail("Failed to init Request") return } XCTAssertNoThrow(try client.execute(request: request, delegate: backpressureResponseDelegate).wait()) XCTAssertEqual(backpressureResponseDelegate.didntWait, false) + XCTAssertGreaterThan(backpressureResponseDelegate.totalCount, 1) XCTAssertEqual(backpressureResponseDelegate.count, 0) } } From 84622f5f95272c97cc587ec915ec9756ab072414 Mon Sep 17 00:00:00 2001 From: Adam Fowler Date: Wed, 24 Jun 2020 15:16:21 +0100 Subject: [PATCH 5/5] Upped amount downloaded in testDownloadBackpressure so we can guarantee at least 3 body parts --- Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift | 4 ++-- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 5ab4432a2..384db82d8 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -541,8 +541,8 @@ internal final class HttpBinHandler: ChannelInboundHandler { } context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil) return - case "/zeros/100000": - let buf = context.channel.allocator.buffer(repeating: 0, count: 100_000) + case "/zeros/150000": + let buf = context.channel.allocator.buffer(repeating: 0, count: 150_000) context.write(wrapOutboundOut(.head(HTTPResponseHead(version: HTTPVersion(major: 1, minor: 1), status: .ok))), promise: nil) context.writeAndFlush(wrapOutboundOut(.body(.byteBuffer(buf))), promise: nil) context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 3e4f4d8c9..8c0506c80 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2581,13 +2581,13 @@ class HTTPClientTests: XCTestCase { } let backpressureResponseDelegate = BackpressureResponseDelegate() - guard let request = try? HTTPClient.Request(url: self.defaultHTTPBinURLPrefix + "zeros/100000") else { + guard let request = try? HTTPClient.Request(url: self.defaultHTTPBinURLPrefix + "zeros/150000") else { XCTFail("Failed to init Request") return } XCTAssertNoThrow(try client.execute(request: request, delegate: backpressureResponseDelegate).wait()) XCTAssertEqual(backpressureResponseDelegate.didntWait, false) - XCTAssertGreaterThan(backpressureResponseDelegate.totalCount, 1) + XCTAssertGreaterThan(backpressureResponseDelegate.totalCount, 2) XCTAssertEqual(backpressureResponseDelegate.count, 0) } }