Add FileDownloadDelegate for simple file downloads #275

MaxDesiatov · 2020-06-24T20:11:29Z

A simple test is included, let me know if you'd like it to be more sophisticated.

Lukasa

Thanks for this!

I'm not best placed to decide if this feature is in-scope or not, so I decided to review this as though it were in scope. I'll let others decide if the feature itself is worth adding.

Sources/AsyncHTTPClient/FileDownloadDelegate.swift

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

MaxDesiatov · 2020-06-25T11:14:59Z

As a quick note, I'd like to get more opinions on whether the feature itself should be included before I address the technical feedback provided by Cory.

artemredkin · 2020-06-26T11:45:05Z

Sources/AsyncHTTPClient/FileDownloadDelegate.swift

+
+    public func didFinishRequest(task: HTTPClient.Task<Response>) throws -> Response {
+        self.writeFuture?.whenComplete { [weak self] _ in
+            try! self?.handle.close()


I don't think we should crash if the close fails

I've made it try! based on this suggestion from @weissi in the Swift Server Slack (#async-http-client channel):

and you want to try! the close. The only reason for the close to fail is that you have already closed it. If you have already closed it, then that's a serious programmer error that need fixing.

MaxDesiatov · 2020-06-30T12:19:47Z

Hi @artemredkin, do you think that this feature makes sense overall in AsyncHTTPClient? Just wondering if it's worth updating the PR to address the feedback, otherwise I'd close it.

artemredkin · 2020-07-14T19:37:31Z

@MaxDesiatov Hi! Sorry I missed your question. I do think this feature would be useful

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

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.

Sources/AsyncHTTPClient/FileDownloadDelegate.swift

Lukasa · 2020-09-07T06:16:43Z

Sources/AsyncHTTPClient/FileDownloadDelegate.swift

+        reportHeaders: ((HTTPHeaders) -> Void)? = nil,
+        reportProgress: ((_ totalBytes: Int?, _ receivedBytes: Int) -> Void)? = nil
+    ) throws {
+        self.handle = try NIOFileHandle(path: path, mode: .write, flags: .allowFileCreation())


I know this is annoying, but open is technically a blocking operation. We should consider making the handle available by a future using self.io.openFile(path:mode:flags:).

Thanks, I've also created apple/swift-nio#1630 to make this more clear in NIOFaileHandle doc comments.

@Lukasa BTW, there's no such thing as openFile(path:mode:flags:), there's only openFile(path:mode:flags:eventLoop:), where would you like the event loop in the initializer to come from?

It'll need to be the event loop that the delegate callbacks fire on.

The event loop for delegate callbacks is passed to every callback separately as an argument, do you think there's a way to get access to that event loop in the initializer? Whoever creates the delegate doesn't have access to that event loop too, it's internal and owned by the HTTPClient instance, to which the delegate doesn't have any direct access to (or even an indirect one, as far as I understand).

Yeah, it's possible that open may have to be delayed until the first operation on the file occurs.

That's a great idea, I've implemented it and it's now ready for review.

Sources/AsyncHTTPClient/HTTPHandler.swift

Lukasa · 2020-09-07T06:18:06Z

Sources/AsyncHTTPClient/HTTPHandler.swift

        }
    }

    func handlerAdded(context: ChannelHandlerContext) {
        guard context.channel.isActive else {
-            self.failTaskAndNotifyDelegate(error: HTTPClientError.remoteConnectionClosed, self.delegate.didReceiveError)
+            self.errorCaught(context: context, error: HTTPClientError.remoteConnectionClosed)


There are a bunch of changes in this file that don't seem well motivated: what are they there for?

These appeared after the base branch was changed from master to main, sorry about that, I'll clean it up.

I'm changing the base branch back from main to master, because main is outdated compared to master in this repo.

@ktoso What happened with the migration to main here?

Something's wrong... I have it noted that we completed the work here but seems master still was the main thing, wat?

Please do not target master @MaxDesiatov, please keep targeting main.
I'll bring main up to date again and re-do the steps about branch protection.
CI is also ticked off to have been changed but I can't confirm that since no admin rights there.

It seems we/I missed to change the default branch so a few PRs were made against master and merged. I've brought main up to date again, fixed that, re targeted this at main and removed master.

Please keep using main from now-on, it should be exactly the same as master just was (it was behind a few commits indeed).

Thanks, I've rebased it on top of main. @Lukasa now ready for review.

Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift

This was noted in review of swift-server/async-http-client#275, but I don't think that this was clearly stated in the documentation.

Lukasa

Cool, I think this looks really good! I've left a few notes in the diff.

Lukasa · 2020-09-08T16:44:08Z

Sources/AsyncHTTPClient/FileDownloadDelegate.swift

+        self.fileHandleFuture = nil
+    }
+
+    public func didFinishRequest(task: HTTPClient.Task<Response>) throws -> Response {


Can we add a test on the error path? I think we're missing cleanup in the didReceiveError case.

Lukasa · 2020-09-08T16:46:56Z

Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift

@@ -531,7 +552,8 @@ internal final class HttpBinHandler: ChannelInboundHandler {
                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)
+                let headers = HTTPHeaders([("Content-Length", "50")])


This changes the behaviour of this endpoint (it previously automatically sent chunked encoding, now it doesn't). I don't think any of the tests rely on that behaviour, but as it's fairly cheap to add new endpoints I think I'd rather we added a new one of these.

We can keep the common code by factoring this out into a method that conditionally adds content-length if we want.

Lukasa

Very nice, I like it a lot!

`FileDownloadDelegate` was introduced in swift-server/async-http-client#275, which almost directly mirrors the implementation in `carton`. Now that it's available in a released version of AHC, let's delete some code here and replace it with the version from that package.

Lukasa requested changes Jun 25, 2020

View reviewed changes

artemredkin reviewed Jun 26, 2020

View reviewed changes

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

MaxDesiatov requested review from artemredkin and Lukasa September 3, 2020 15:14

Lukasa requested changes Sep 7, 2020

View reviewed changes

MaxDesiatov mentioned this pull request Sep 7, 2020

Note that certain NIOFileHandle.init overloads are blocking apple/swift-nio#1630

Merged

MaxDesiatov changed the base branch from main to master September 7, 2020 08:40

MaxDesiatov requested a review from Lukasa September 7, 2020 08:40

ktoso closed this Sep 8, 2020

ktoso reopened this Sep 8, 2020

ktoso changed the base branch from master to main September 8, 2020 02:57

MaxDesiatov added 13 commits September 8, 2020 17:32

Add FileDownloadDelegate for simple file downloads

9e7b767

Add testFileDownload to the allTests array

dce7a89

Fix formatting

cc223ef

Fix compatibility with Swift 5.0

e8b019e

Add doc comments, update README.md

68559ba

Refine FileDownloadDelegate description in README

34ae01c

Bump NIO version, remove weak self, cleanup test

d7c6bb4

Fix formatting issues in a doc comment

e10c6f0

Create separate Progress struct, async open file

b091019

Create an ad-hoc EventLoopGroup for opening a file

43b71ca

Move file opening code to didReceiveBodyPart

f75fb8c

Fix linter error in FileDownloadDelegate.swift

912ca16

Fix wrong future assignment in FileDownloadDelegate

bac8670

MaxDesiatov added 4 commits September 8, 2020 17:32

Fix Swift 5.0 return statement compatibility

e89a351

Fix linter warning

971ef86

Fix Swift 5.0 return statement compatibility

a22ed50

Remove redundant write function

f5b6cdd

Lukasa reviewed Sep 8, 2020

View reviewed changes

MaxDesiatov added 2 commits September 8, 2020 18:14

Add negative test case and separate testing endpoint

7b25a4f

Add missing testFileDownloadError to the manifest

6b041b5

MaxDesiatov requested a review from Lukasa September 8, 2020 17:27

Lukasa approved these changes Sep 10, 2020

View reviewed changes

Lukasa merged commit 49a0d30 into swift-server:main Sep 10, 2020

MaxDesiatov deleted the file-download-delegate branch September 10, 2020 08:21

MaxDesiatov mentioned this pull request Nov 15, 2020

Use FileDownloadDelegate from the AHC package swiftwasm/carton#171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FileDownloadDelegate for simple file downloads #275

Add FileDownloadDelegate for simple file downloads #275

MaxDesiatov commented Jun 24, 2020

Lukasa left a comment

MaxDesiatov commented Jun 25, 2020

artemredkin Jun 26, 2020

MaxDesiatov Jun 26, 2020

MaxDesiatov commented Jun 30, 2020

artemredkin commented Jul 14, 2020

ktoso commented Aug 20, 2020

Lukasa Sep 7, 2020

MaxDesiatov Sep 7, 2020

MaxDesiatov Sep 7, 2020

Lukasa Sep 7, 2020

MaxDesiatov Sep 7, 2020

Lukasa Sep 7, 2020

MaxDesiatov Sep 7, 2020

Lukasa Sep 7, 2020

MaxDesiatov Sep 7, 2020 •

edited

Loading

MaxDesiatov Sep 7, 2020

Lukasa Sep 7, 2020

ktoso Sep 8, 2020

ktoso Sep 8, 2020

MaxDesiatov Sep 8, 2020

Lukasa left a comment

Lukasa Sep 8, 2020

Lukasa Sep 8, 2020

Lukasa left a comment

Add FileDownloadDelegate for simple file downloads #275

Add FileDownloadDelegate for simple file downloads #275

Conversation

MaxDesiatov commented Jun 24, 2020

Lukasa left a comment

Choose a reason for hiding this comment

MaxDesiatov commented Jun 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov commented Jun 30, 2020

artemredkin commented Jul 14, 2020

ktoso commented Aug 20, 2020

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

MaxDesiatov Sep 7, 2020 • edited Loading

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

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

Lukasa left a comment

Choose a reason for hiding this comment

MaxDesiatov Sep 7, 2020 •

edited

Loading