Upload streaming #37

artemredkin · 2019-05-21T16:05:25Z

Addressing comments in the client proposal discussion, I would like to propose the following change:

HTTPResponseDelegate.didReceivePart now returns an optional future that could be used to indicate to the client that reads should be stopped until this future is resolved (backpressure)

HTTPClient.Body is now a struct with optional length and a callback for upload streaming. In addition, there are static methods to keep API the same as before.

Motivation: Users may want to optimize memory usage when executing request with big bodies, same as streaming download Modifications: Added ChunkProvider typealias in HTTPHandler.swift Added new Body enum case - stream in HTTPHandler.swift Extracted body processing in HTTPHandler.swift to a separate method Added .stream enum processing in HTTPHandler.swift Added upload streaming test to SwiftNIOHTTPTests.swift Result: HTTPClient library now provides methods to stream Request body

… of files

weissi

Thanks, that looks like a great start. I think we need to invest a bunch more in testing this functionality though.

Tests/NIOHTTPClientTests/SwiftNIOHTTPTests.swift

weissi · 2019-05-22T09:59:44Z

Tests/NIOHTTPClientTests/SwiftNIOHTTPTests.swift

@@ -322,4 +322,65 @@ class SwiftHTTPTests: XCTestCase {
        let res = try httpClient.get(url: "https://test/ok").wait()
        XCTAssertEqual(res.status, .ok)
    }
+


we need a bunch of other tests here:

upload/download interrupted

failed future returned from uploading & downloading

a test that shows that the back-pressure is working

Added failure test and backpressure test. What do you mean by interruption? External cancellation?

README.md

Sources/NIOHTTPClient/HTTPHandler.swift

Sources/NIOHTTPClient/Utils.swift

ianpartridge

I must be missing something... I thought didReceivePart() was for streaming incoming body data (e.g. GET response). Are you intending to also use this callback for streaming outgoing body data (e.g. POST)? If so, I think that is confusing to the user...

weissi · 2019-05-31T13:54:25Z

I must be missing something... I thought didReceivePart() was for streaming incoming body data (e.g. GET response). Are you intending to also use this callback for streaming outgoing body data (e.g. POST)? If so, I think that is confusing to the user...

@ianpartridge no, you're not missing anything but it's important to propagate back-pressure. Let's assume we have three different nodes A, B, and C with two active HTTP connections:

A --> B --> C

where A --> B is B download streaming a large file from A (eg. through a GET) and B --> C is B upload streaming the same file to C (eg. through a POST).

Now let's assume we're B and we prefer not to be killed by the OOM killer. We must now protect against A delivering us the bytes faster than C will accept them (or else we would need to buffer the bytes that C didn't accept yet in memory which would eventually lead to the OOM killer coming to find us).

How can we (as B) achieve this? We need to propagate the back-pressure that C is giving us back to A. For this HTTP API this would mean that we need to be able to tell the didReceivePart callback when we're ready to accept the next part. For blocking IO, back-pressure is easy by doing:

func didReceivePart(data: ...) {
    otherConnection.sendData(data) // blocks until `data` is fully sent
}

for asynchronous programming we can express the same concept as

func didReceivePart(data: ...) -> EventLoopFuture<Void> {
    return otherConnection.sendData(data) // returns a future that is fulfilled when the data has been sent
}

in other words: The whole thing totally works iff the API allows us to indicate when we're ready for the next chunk and that's done with the change to have didReceivePart return a future.

And yes, this isn't actually related to upload streaming but when I was looking at the back-pressure for the upload streaming I noticed that download streaming doesn't actually support back-pressure and therefore it's in this PR :).

ianpartridge · 2019-05-31T14:13:54Z

OK, thanks so much for the awesome explanation. So, if I'm understanding correctly, the new EventLoopFuture<Void>? return value of didReceivePart() means as follows:

nil: I've finished handling the part, you can send me another whenever you like.
An EventLoopFuture<Void>: I'm still handling the part, you can send me another when this future is fulfilled.

Thoughts...

Is the reason the future is optional because it avoids the cost of creating an already succeeded future in the case where the user doesn't care about propagating backpressure? Otherwise the future could be non-optional and we just ask the user to create a succeeded future.

Would returning an enum with two cases (case done and case pending(EventLoopFuture<Void>)) be clearer to the user?

Lukasa · 2019-05-31T14:16:28Z

I'd missed this, thanks for pointing this out @ianpartridge:

Is the reason the future is optional because it avoids the cost of creating an already succeeded future in the case where the user doesn't care about propagating backpressure? Otherwise the future could be non-optional and we just ask the user to create a succeeded future.

I don't think we should allow a nil return value here. Forgetting to respect back pressure is a common source of nasty bugs that can manifest as OOMs and crashes, so the easier we can make it to propagate the backpressure the better. I think we did a good job of this on NonBlockingFileIO.readChunked, and I'd like to see us continue that here, so I'd be strongly in favour of requiring a future to be returned.

weissi · 2019-05-31T14:18:27Z

Thoughts...

Is the reason the future is optional because it avoids the cost of creating an already succeeded future in the case where the user doesn't care about propagating backpressure? Otherwise the future could be non-optional and we just ask the user to create a succeeded future

Hah, glad you point that out! I had no idea it was optional, I don't think it should be :). @artemredkin was that deliberate?

Would returning an enum with two cases (case done and case pending(EventLoopFuture<Void>)) be clearer to the user?

This would be slightly cheaper at run-time (than always allocating) in the case where we don't need back-pressure (rare!) and as you point out make the API less clear. This feels like premature optimisation to me.

artemredkin · 2019-05-31T14:21:30Z

It was deliberate, yes. I'll change it to non-optional

artemredkin · 2019-05-31T14:23:46Z

Actually, reason I made it's optional is that there is no easy way to create completed future without passing in eventLoopGroup...

Lukasa · 2019-05-31T14:25:09Z

In general I think that's an acceptable limitation. I'm happy with the idea that any reasonable delegate implementation will need access to an event loop.

artemredkin · 2019-05-31T14:25:50Z

@Lukasa @weissi do you think enum may be used here, or should I pass in a non-optional promise as an argument for example?

artemredkin · 2019-05-31T14:27:07Z

@Lukasa I wanted delegates to have simple inits without dependencies...

Lukasa · 2019-05-31T14:27:17Z

Neither: I think both will be misused. If we really think it's unacceptable to require the delegate have access to an event loop then I prefer non-optional promise to the enum unless the empty case is .iAmHappyIfSomeoneDoSesMyServerAndPromiseNotToComplain.

Lukasa · 2019-05-31T14:31:54Z

I wanted delegates to have simple inits without dependencies...

Why?

artemredkin · 2019-05-31T14:35:28Z

I think having something like:

let client = HTTPClient(...)
let delegate = CountingDelegate()
client.execute(request: request, delegate: delegate)

is slightly better than:

let client = HTTPClient(...)
let delegate = CountingDelegate(eventLoopGroup: client.eventLoopGroup)
client.execute(request: request, delegate: delegate)

but I see your point.

Lukasa · 2019-05-31T14:36:33Z

Note that you only need EventLoop, not EventLoopGroup.

Sources/NIOHTTPClient/HTTPHandler.swift

artemredkin · 2019-05-31T15:11:14Z

Task has an optional Channel unfortunately, but its easy to add EventLoop there

artemredkin · 2019-05-31T15:22:17Z

Added EventLoop to Task

Sources/NIOHTTPClient/HTTPHandler.swift

ianpartridge

Sorry, a pile more comments. I hope they are useful.

ianpartridge · 2019-05-31T20:39:06Z

Sources/NIOHTTPClient/HTTPHandler.swift

-            case .string(let string):
-                return string.utf8.count
+    struct Body {
+        typealias ChunkProvider = (@escaping (IOData) -> EventLoopFuture<Void>) -> EventLoopFuture<Void>


Maybe we should call this PartProvider - for symmetry with didReceivePart().

I wonder if the typealias holds its weight actually...

Rewrote without typealias, seems to be fine

Sources/NIOHTTPClient/HTTPHandler.swift

ianpartridge · 2019-05-31T21:03:41Z

Sources/NIOHTTPClient/HTTPHandler.swift

-                return data.count
-            case .string(let string):
-                return string.utf8.count
+    struct Body {


Nit: this is really a personal preference (although I think NIO style might mandate this) but I much prefer having explicit access modifiers, and just extension instead of public extension. I'm not clever enough to remember Swift's access control rules without having to think twice every time...

good idea, fixed, thanks!

yes please, I think there should never be any word in front of extension. public extension for example is really bad because it means that you can add public functions without the word public appearing in a diff and that's an issue for all SemVer stuff.

Sources/NIOHTTPClient/HTTPHandler.swift

ianpartridge · 2019-05-31T21:13:11Z

Sources/NIOHTTPClient/HTTPHandler.swift

@@ -207,13 +224,15 @@ internal extension URL {

 public extension HTTPClient {
    final class Task<Response> {
+        let eventLoop: EventLoop


I think this needs to be public. If I'm correct the tests are only compiling because you have @testable which users won't have...

fixed, thanks!

yeah, if we can it might be good to remove @testable from the test files... But I know it's not always that straightforward. Another option might be to have to test suites: One for the public API and one that does some extra internal stuff. But this is not super high priority ofc.

artemredkin · 2019-06-01T18:24:10Z

Almost forgot, I also wanted to add callback for every body part upload.
@weissi @Lukasa one last question, do you think we need to return EventLoopFuture from didReceiveHead for backpressure purposes as well?

artemredkin · 2019-06-01T18:28:14Z

README.md

@@ -116,26 +116,32 @@ class CountingDelegate: HTTPResponseDelegate {

    var count = 0

-    func didTransmitRequestBody() {
-        // this is executed when request is sent, called once
+    func didTransmitRequestPart(task: HTTPClient.Task<Response>, _ part: IOData) {


@weissi @ianpartridge @tanner0101 do you think we need a callback when request head is sent as well? Also, what do you think about naming pattern (transmit/sent)?

I'd probably err on the side of 'more callbacks', @Lukasa can you think of a good use-case for a callback when the head has been sent?

regarding naming transmit vs. send: I don't mind, think both are fine

not sure about the part: IOData label, part is maybe a bit unclear? I would almost make it _ part: IOData, don't think the label adds a lot of value there given the type IOData? But this is really not super important :)

but it's already _ part: IOData :)

whoops :), sorry

Lukasa · 2019-06-02T16:47:47Z

Yeah, more callbacks is always better, go for it.

artemredkin · 2019-06-04T21:50:07Z

@weissi are you happy with the backpressure/failure tests?
@Lukasa do you think there is any value in returning Future from didReceiveHead for backpressure (maybe like opening file for writing is return code is 2xx or something)?

Lukasa · 2019-06-04T22:09:50Z

I'm in general in favour of being able to exert back pressure at as many places as possible, yeah.

artemredkin · 2019-06-12T16:04:07Z

@weissi @Lukasa @ianpartridge @tanner0101 do you have any additional comments on this PR?

tanner0101 · 2019-06-12T16:26:32Z

Sources/NIOHTTPClient/HTTPHandler.swift

+extension HTTPClient {
+    public struct Body {
+        public var length: Int?
+        public var provider: (@escaping (IOData) -> EventLoopFuture<Void>) -> EventLoopFuture<Void>


Using a struct here would give us more flexibility to add / deprecate methods in the future. It would also allow users of the package to add conformances / extensions.

Something like Body.StreamWriter or Body.Provider:

extension HTTPClient.Body { var writer: (StreamWriter) -> EventLoopFuture<Void> public struct StreamWriter { let closure: (IOData) -> EventLoopFuture<Void> public func write(_ data: IOData) -> EventLoopFuture<Void> { return self.closure(data) } } public static func stream(length: Int? = nil, _ writer: @escaping (StreamWriter) -> EventLoopFuture<Void>) -> ... } } ...

This also reduces the mental load to understand the code a bit.

Here's example usage:

let body: HTTPClient.Body = .stream(length: 50) { stream in var request = try! Request(url: "http://localhost:\(httpBin.port)/events/10/1") request.headers.add(name: "Accept", value: "text/event-stream") let delegate = CopyingDelegate { part in stream.write(.byteBuffer(part)) } return httpClient.execute(request: request, delegate: delegate).future }

good idea, refactored, thanks!

Lukasa

I think this looks good @artemredkin.

weissi

looks good

artemredkin added 7 commits April 18, 2019 22:58

use IOData instead of ByteBuffer to allow for more efficient transfer…

be5594f

… of files

Merge branch 'master' into upload_streaming

b255af6

address some discussion comments

b97a24a

implement backpressure

97a1fc2

Merge branch 'master' into upload_streaming

4ae7f70

Update README

39542ab

artemredkin requested review from weissi, tanner0101 and ianpartridge May 21, 2019 16:05

add missing tests for linux

131fea1

weissi requested changes May 22, 2019

View reviewed changes

tanner0101 reviewed May 29, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

Sources/NIOHTTPClient/HTTPHandler.swift Outdated Show resolved Hide resolved

Sources/NIOHTTPClient/HTTPHandler.swift Outdated Show resolved Hide resolved

Sources/NIOHTTPClient/Utils.swift Outdated Show resolved Hide resolved

tanner0101 added the kind/enhancement Improvements to existing feature. label May 29, 2019

artemredkin added 2 commits May 30, 2019 19:57

review fix: do not use unsafe

b661f08

address review comments

c5a93a8

ianpartridge reviewed May 31, 2019

View reviewed changes

Sources/NIOHTTPClient/HTTPHandler.swift Outdated Show resolved Hide resolved

review fix: fail on future failure and encapsulate eventloop inside Task

6d23788

ianpartridge reviewed May 31, 2019

View reviewed changes

Sources/NIOHTTPClient/HTTPHandler.swift Outdated Show resolved Hide resolved

ianpartridge reviewed May 31, 2019

View reviewed changes

artemredkin added 5 commits June 1, 2019 09:06

review fixes

51b821a

add upload streaming failure cases

3b0e3f9

add upload streaming backpressure test

e0fd437

fix missing linux tests

4a35b34

add callbacks for upload body parts

96d303d

update readme

06d2f2c

artemredkin commented Jun 1, 2019

View reviewed changes

rename didTransim to didSend, added didSendHead callback

634bbd4

add backpressure to response head callback

15d11a8

tanner0101 reviewed Jun 12, 2019

View reviewed changes

artemredkin added 2 commits June 12, 2019 21:23

review fix: extract provider to a separate struct

e1b427e

rename property

9fcf8f0

Lukasa approved these changes Jun 13, 2019

View reviewed changes

tanner0101 approved these changes Jun 13, 2019

View reviewed changes

weissi approved these changes Jun 17, 2019

View reviewed changes

artemredkin merged commit b019e08 into master Jun 17, 2019

artemredkin deleted the upload_streaming branch June 17, 2019 17:21

artemredkin mentioned this pull request Jun 19, 2019

request body streaming #29

Closed

artemredkin mentioned this pull request Aug 15, 2019

Remove eventLoop property from Task #82

Closed

Upload streaming #37

Upload streaming #37

Conversation

artemredkin commented May 21, 2019

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

ianpartridge left a comment

Choose a reason for hiding this comment

weissi commented May 31, 2019

ianpartridge commented May 31, 2019

Lukasa commented May 31, 2019

weissi commented May 31, 2019

artemredkin commented May 31, 2019

artemredkin commented May 31, 2019

Lukasa commented May 31, 2019

artemredkin commented May 31, 2019

artemredkin commented May 31, 2019

Lukasa commented May 31, 2019

Lukasa commented May 31, 2019

artemredkin commented May 31, 2019

Lukasa commented May 31, 2019

artemredkin commented May 31, 2019

artemredkin commented May 31, 2019

ianpartridge 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

artemredkin commented Jun 1, 2019

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 2, 2019

artemredkin commented Jun 4, 2019

Lukasa commented Jun 4, 2019

artemredkin commented Jun 12, 2019

tanner0101 Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

tanner0101 Jun 12, 2019 •

edited

Loading