Add TestBackpressure test #273

adam-fowler · 2020-06-23T21:57:29Z

Add response delegate backpressure test. Creates delegate that returns a futureResult from a scheduleTask that will finish after one second has passed. The processingBodyPart flag is set at the beginning of didReceiveBodyPart and cleared when the scheduled task runs. Add check to see if this flag is set when entering didReceiveBodyPart. If so then the backpressure wasn't applied and the previous didReceiveBodyPart task is still running.

Also added count which increments at entry to didReceiveBodyPart and decrements on scheduled task being run.

Lukasa

Hey @adam-fowler, I'm about to do a full review but I wanted to let you know early that you'll need to run the test generation script to get the CI working.

adam-fowler · 2020-06-24T10:40:50Z

Hey @adam-fowler, I'm about to do a full review but I wanted to let you know early that you'll need to run the test generation script to get the CI working.

sorry Cory forgot about that. Sorted now

Lukasa

Cool, broadly looks good! A few notes in the diff.

Lukasa · 2020-06-24T10:40:19Z

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

@@ -2533,4 +2533,59 @@ class HTTPClientTests: XCTestCase {
        XCTAssertEqual(info.connectionNumber, 1)
        XCTAssertEqual(info.requestNumber, 1)
    }
+
+    func testBackpressue() {


Nit:

Suggested change

func testBackpressue() {

func testBackpressure() {

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

Lukasa · 2020-06-24T10:42:37Z

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

+                    count += 1
+                }
+                // wait one second before returning a successful future
+                return task.eventLoop.scheduleTask(in: .milliseconds(1000) ) {


Waiting a second does force this test to take quite a long time. Is there any reason we think this test wouldn't work equally well with a delay of, say, 200ms instead of the full second?

Reduced to 200ms

Lukasa · 2020-06-24T10:43:24Z

Hey @adam-fowler, I'm about to do a full review but I wanted to let you know early that you'll need to run the test generation script to get the CI working.

sorry Cory forgot about that. Sorted now

No need to apologise! Easily done. 😁

Lukasa · 2020-06-24T10:53:14Z

Nice, looks exactly right. Test failures seem to back up #274 as well, so this is a good leaping off point for providing a fix.

weissi

Looks good, couple of questions.

weissi · 2020-06-24T11:22:35Z

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

+            func didFinishRequest(task: HTTPClient.Task<Response>) throws {}
+        }
+
+        let elg = MultiThreadedEventLoopGroup(numberOfThreads: 5)


can't we use the self.defaultHTTPClient?

Then we don't need a group or a client

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

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

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

weissi · 2020-06-24T11:22:38Z

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

@@ -2533,4 +2533,59 @@ class HTTPClientTests: XCTestCase {
        XCTAssertEqual(info.connectionNumber, 1)
        XCTAssertEqual(info.requestNumber, 1)
    }
+
+    func testBackpressure() {


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

Asl

weissi · 2020-06-24T11:26:43Z

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

+            func didReceiveBodyPart(task: HTTPClient.Task<Response>, _ part: ByteBuffer) -> EventLoopFuture<Void> {
+                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


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

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

@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...

In this situation it doesn't work.

weissi · 2020-06-24T11:29:02Z

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

+                return task.eventLoop.makeSucceededFuture(())
+            }
+
+            func didReceiveBodyPart(task: HTTPClient.Task<Response>, _ part: ByteBuffer) -> EventLoopFuture<Void> {


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?

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

@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.

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

Added HTTPBin path "/zeros/100000"

so we can guarantee at least 3 body parts

ktoso · 2020-08-20T01:34:31Z

Just as a heads up, the main development branch has been changed to main, following the Swift policy on this.

This PR has been re-targeted to main and should just work. However when performing rebases etc please keep this in mind -- you may want to fetch the main branch and rebase onto the main branch from now on since master is not up-to-date anymore and is going to be deleted shortly.

adam-fowler · 2020-08-24T14:12:19Z

Just as a heads up, the main development branch has been changed to main, following the Swift policy on this.

This PR has been re-targeted to main and should just work. However when performing rebases etc please keep this in mind -- you may want to fetch the main branch and rebase onto the main branch from now on since master is not up-to-date anymore and is going to be deleted shortly.

All for this, but the default branch is still master here and recent PRs have been merged into master and not main

adam-fowler · 2021-04-21T17:45:13Z

@Lukasa This looks like you fixed this and added tests in #352. Shall I close

Lukasa · 2021-04-21T18:29:42Z

Sure thing.

Add TestBackpressure test

9d8aa24

adam-fowler force-pushed the test-backpressure branch from 0cb4954 to 9d8aa24 Compare June 23, 2020 22:00

adam-fowler mentioned this pull request Jun 23, 2020

ResponseDelegate can invoke didReceiveBodyPart before the previous run's future returned #274

Closed

Lukasa requested a review from weissi June 24, 2020 10:37

Lukasa requested changes Jun 24, 2020

View reviewed changes

Include tests, Sanity fixes

df778c5

Lukasa requested changes Jun 24, 2020

View reviewed changes

Reduce wait time in testBackpressure

19accd7

weissi reviewed Jun 24, 2020

View reviewed changes

adam-fowler added 2 commits June 24, 2020 15:13

Changes from review

8611b61

Added HTTPBin path "/zeros/100000"

Upped amount downloaded in testDownloadBackpressure

84622f5

so we can guarantee at least 3 body parts

ktoso changed the base branch from master to main August 20, 2020 01:30

adam-fowler closed this Apr 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TestBackpressure test #273

Add TestBackpressure test #273

adam-fowler commented Jun 23, 2020 •

edited

Loading

Lukasa left a comment

adam-fowler commented Jun 24, 2020

Lukasa left a comment

Lukasa Jun 24, 2020

adam-fowler Jun 24, 2020

Lukasa Jun 24, 2020

adam-fowler Jun 24, 2020

Lukasa commented Jun 24, 2020

Lukasa commented Jun 24, 2020

weissi left a comment

weissi Jun 24, 2020

weissi Jun 24, 2020

adam-fowler Jun 24, 2020

weissi Jun 24, 2020

adam-fowler Jun 24, 2020

weissi Jun 24, 2020

adam-fowler Jun 24, 2020

weissi Jun 24, 2020

adam-fowler Jun 24, 2020

weissi Jun 24, 2020

adam-fowler Jun 24, 2020

weissi Jun 24, 2020

adam-fowler Jun 24, 2020

weissi Jun 24, 2020

ktoso commented Aug 20, 2020

adam-fowler commented Aug 24, 2020

adam-fowler commented Apr 21, 2021

Lukasa commented Apr 21, 2021

Add TestBackpressure test #273

Add TestBackpressure test #273

Conversation

adam-fowler commented Jun 23, 2020 • edited Loading

Lukasa left a comment

Choose a reason for hiding this comment

adam-fowler commented Jun 24, 2020

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented Jun 24, 2020

Lukasa commented Jun 24, 2020

weissi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso commented Aug 20, 2020

adam-fowler commented Aug 24, 2020

adam-fowler commented Apr 21, 2021

Lukasa commented Apr 21, 2021

adam-fowler commented Jun 23, 2020 •

edited

Loading