Skip to content

Add TestBackpressure test #273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ extension HTTPClientTests {
("testDelegateCallinsTolerateRandomEL", testDelegateCallinsTolerateRandomEL),
("testContentLengthTooLongFails", testContentLengthTooLongFails),
("testContentLengthTooShortFails", testContentLengthTooShortFails),
("testBackpressure", testBackpressure),
]
}
}
55 changes: 55 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2533,4 +2533,59 @@ class HTTPClientTests: XCTestCase {
XCTAssertEqual(info.connectionNumber, 1)
XCTAssertEqual(info.requestNumber, 1)
}

func testBackpressure() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should maybe name it testDownloadBackpressure? That'd match the existing test.

Asl

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

class BackpressureResponseDelegate: HTTPClientResponseDelegate {
typealias Response = Void
var count = 0
var processingBodyPart = false
var didntWait = false
var lock = Lock()

init() {}

func didReceiveHead(task: HTTPClient.Task<Response>, _ head: HTTPResponseHead) -> EventLoopFuture<Void> {
return task.eventLoop.makeSucceededFuture(())
}

func didReceiveBodyPart(task: HTTPClient.Task<Response>, _ part: ByteBuffer) -> EventLoopFuture<Void> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are we making sure that didReceiveBodyPart is actually invoked multiple times? Because if it's only invoked once, then we don't know if download streaming works or not I think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We aren’t at the moment. I can add a test to see if it did. Is there a way we can force it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adam-fowler From the NIO level: Yes, you could set ChannelOptions.maxMessagesPerRead to 1 and set the allocator to FixedSizeRecvAllocator(1) which means that NIO will read everything byte to byte. That'll cause AHC to call out multiple times.

The issue here is that through AHC's API, you can't easily mess with the underlying Channel... You could make it an internal test and force it that way.

The other (probably easier) option is:

  • Download say 1 MB of data. NIO's default allocator will never send you more than 64k. Given that AHC doesn't change it, you can't get 1 MB in one big chunk.
  • Add XCTAssertGreaterOrEqual(numberOfCalls, 2) or so.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of interest: What about XCTAssertEqual doesn't work here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XCTAssertEqual doesn’t return an error while assert does not totally sure why

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adam-fowler yes, it continues the execution but it will still fail the test run once it's complete. So I think you can just use XCTAssert...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this situation it doesn't work.

if processingBodyPart == true {
didntWait = true
}
processingBodyPart = true
count += 1
}
// wait one second before returning a successful future
return task.eventLoop.scheduleTask(in: .milliseconds(200)) {
self.lock.withLock {
self.processingBodyPart = false
self.count -= 1
}
}.futureResult
}

func didReceiveError(task: HTTPClient.Task<Response>, _ error: Error) {}
func didFinishRequest(task: HTTPClient.Task<Response>) throws {}
}

let elg = MultiThreadedEventLoopGroup(numberOfThreads: 5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we use the self.defaultHTTPClient?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we don't need a group or a client

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a client that uses an elg with more than one thread is more likely to fail

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should get #177 in for that tbh. But your call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until #177 is merged I leave this using the local client

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)
}
}