Initial implementation of FTP #1122

saiHemak · 2017-07-20T11:52:59Z

This PR includes ..

FTP Data Task, Download Task implementations ,Upload task with completion handler
Refactoring of URLProtocol code to support both FTP and HTTP protocol implementations.
Basic FTPServer implementation which supports download,data and upload task
Test cases for FTP implementation

alblue

Since this is a WIP adding a blocking review to make sure it doesn't get merged at the current stage. Let me know when it's good to go and I can remove this review.

pushkarnk · 2017-07-21T06:40:16Z

Foundation/NSURLSession/ftp/FTPURLProtocol.swift

+        }
+    }
+
+    /* override func transferCompleted(withErrorCode errorCode: Int?) {


We need to remove the commented code.

pushkarnk · 2017-07-21T06:40:46Z

Foundation/NSURLSession/http/HTTPBodySource.swift

@@ -128,12 +128,12 @@ internal final class _HTTPBodyFileSource {
        guard let channel = DispatchIO(type: .stream, path: fileSystemRepresentation,
                                       oflag: O_RDONLY, mode: 0, queue: workQueue,
                                       cleanupHandler: {_ in }) else {
-            fatalError("Cant create DispatchIO channel")
+                                        fatalError("Cant create DispatchIO channel")


Indentation error.

pushkarnk · 2017-07-21T06:41:59Z

Foundation/NSURLSession/http/HTTPURLProtocol.swift

        if let location = response.allHeaderFields[value] as? String {
            return location
        }
        return nil
    }
 }
+


These two extra lines could be removed?

pushkarnk · 2017-07-21T06:43:50Z

Foundation.xcodeproj/project.pbxproj

@@ -1339,6 +1344,24 @@
 			path = Foundation;
 			sourceTree = "<group>";
 		};
+		617F57F01F1F5C4B0004F3F0 /* libcurlhelpers */ = {


May be we should call the directory simply libcurl (because it contains libcurl abstractions) instead of libcurlhelpers

pushkarnk · 2017-07-21T07:04:51Z

Foundation/NSURLSession/NativeProtocol.swift

+internal let enableDebugOutput: Bool = {
+    return (ProcessInfo.processInfo.environment["URLSessionDebug"] != nil)
+}()
+


It may help if we could add a line or two describing the role of this new internal class :-)

pushkarnk · 2017-07-21T07:05:47Z

Foundation/NSURLSession/NativeProtocol.swift

+        return .completeTask
+    }
+
+    func seekInputStream(to position: UInt64) throws {


Empty method?

xwu · 2017-07-23T06:27:27Z

Foundation/NSURLSession/NativeProtocol.swift

@@ -0,0 +1,700 @@
+// Foundation/NSURLSession/TransferState.swift - NSURLSession & libcurl


Why does this header refer to an apparently non-existent other file?

xwu · 2017-07-23T06:30:39Z

Foundation/NSURLSession/ftp/FTPURLProtocol.swift

+//  Foundation
+//
+//  Created by sai hema on 6/27/17.
+//  Copyright © 2017 Apple. All rights reserved.


This should be replaced with the standard header found in other Foundation files.

xwu · 2017-07-23T06:33:28Z

Foundation/NSURLSession/NativeProtocol.swift

+        task?.response = response
+        //We don't want a timeout to be triggered after this. The timeout timer needs to be cancelled.
+        easyHandle.timeoutTimer = nil
+        //because we deregister the task with the session on internalState being set to taskCompleted


Please rewrap, capitalize, and space these comments to match the rest of the file.

pushkarnk · 2017-07-27T13:46:14Z

@saiHemak Does the refactored code consider #1121 and #1132 ? Thanks.

ianpartridge · 2017-09-11T09:32:11Z

@swift-ci please test

saiHemak · 2017-09-12T05:14:46Z

@pushkarnk Yes ..I have taken care of it ..

ianpartridge

I've made a few basic review comments but had to stop as the PR is so large it is extremely difficult to review.

I would much prefer it if this was broken down into a series of incremental commits.

ianpartridge · 2017-09-12T08:11:20Z

Foundation/URLSession/URLSessionConfiguration.swift

@@ -47,7 +47,7 @@ open class URLSessionConfiguration : NSObject, NSCopying {
        self.urlCredentialStorage = nil
        self.urlCache = nil
        self.shouldUseExtendedBackgroundIdleMode = false
-        self.protocolClasses = [_HTTPURLProtocol.self]
+        self.protocolClasses = [_HTTPURLProtocol.self,_FTPURLProtocol.self]


Space after comma.

ianpartridge · 2017-09-12T08:11:28Z

Foundation/URLSession/ftp/FTPURLProtocol.swift

@@ -0,0 +1,155 @@
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2016 Apple Inc. and the Swift project authors


ianpartridge · 2017-09-12T09:04:17Z

Foundation/URLSession/NativeProtocol.swift

+    /// return value's `isHeaderComplete` will then by `true`.
+    ///
+    /// - Throws: When a parsing error occurs
+    func byAppendingFTP(headerLine data: Data, exectedContentLength: Int64) throws -> _NativeProtocol._TransferState {


Typo: expectedContentLength

ianpartridge · 2017-09-12T09:13:53Z

Foundation/URLSession/http/HTTPBodySource.swift

@@ -40,15 +40,15 @@ internal func splitData(dispatchData data: DispatchData, atPosition position: In
 }

 /// A (non-blocking) source for HTTP body data.
-internal protocol _HTTPBodySource: class {
+internal protocol _BodySource: class {


As this is no longer specific to HTTP, should it be moved out of the http directory?

ianpartridge · 2017-09-12T09:16:00Z

Foundation/URLSession/ftp/FTPURLProtocol.swift

+    }
+
+    override func configureEasyHandle(for request: URLRequest) {
+        //easyHandle.set(verboseModeOn: true)


Remove these comments.

saiHemak · 2017-09-12T10:53:33Z

@ianpartridge , I have addressed your comments.

saiHemak · 2017-09-12T11:01:55Z

@ianpartridge : At this point incremental commits would be a laborious task, considering the refactoring effort involved .Do you have any other suggestions ?

saiHemak · 2017-09-12T11:04:10Z

@alblue please review ..

ianpartridge · 2017-09-12T11:28:41Z

No I don't have any suggestions - personally I find this unreviewable 😢

pushkarnk · 2017-09-14T07:05:51Z

We should have the URLSession refactoring part of this PR carved out into a different PR, because the objective of that refactor is to hoist some code (into a common super class) so that it can be reused in other native protocol implementations (we have the DATA protocol still unimplemented).

saiHemak · 2017-09-14T10:07:56Z

@pushkarnk : I have raise another PR for the URLSession refactoring [https://github.com//pull/1216].

alblue

Minor observations

alblue · 2017-09-15T10:38:15Z

Foundation/URLSession/NativeProtocol.swift

+import Dispatch
+
+internal let enableLibcurlDebugOutput: Bool = {
+    return  (ProcessInfo.processInfo.environment["URLSessionDebugLibcurl"] != nil)


The parenthesis around these values appear to have no value. Can you remove them, and get rid of the double-space after the return keyword?

alblue · 2017-09-15T10:41:38Z

Foundation/URLSession/NativeProtocol.swift

+    }
+
+    func didReceive(data: Data) -> _EasyHandle._Action {
+        guard case .transferInProgress(var ts) = internalState else { fatalError("Received body data, but no transfer in progress.") }


Can you insert a newline after the { and before the fatalError, along with } on its own line? That way the line size doesn't balloon for no reason.

alblue · 2017-09-15T10:41:53Z

Foundation/URLSession/NativeProtocol.swift

+    }
+
+    func validateHeaderComplete(transferSate: _TransferState) -> URLResponse? {
+        guard transferSate.isHeaderComplete else { fatalError("Received body data, but the header is not complete, yet.") }


Same as above

alblue · 2017-09-15T10:42:02Z

Foundation/URLSession/NativeProtocol.swift

+    }
+
+    fileprivate func notifyDelegate(aboutReceivedData data: Data) {
+        guard let t = self.task else { fatalError("Cannot notify") }


Same as above

alblue · 2017-09-15T10:42:41Z

Foundation/URLSession/NativeProtocol.swift

+    }
+
+    func fill(writeBuffer buffer: UnsafeMutableBufferPointer<Int8>) -> _EasyHandle._WriteBufferResult {
+        guard case .transferInProgress(let ts) = internalState else { fatalError("Requested to fill write buffer, but transfer isn't in progress.") }


Same as above

alblue · 2017-09-15T11:02:57Z

TestFoundation/TestFTP.swift

+        let dataTask1 = sesh.dataTask(with: req, completionHandler: { data, res, error in
+            XCTAssertNil(error)
+            defer { expect.fulfill() }
+


Superflous blank line

alblue · 2017-09-15T11:03:02Z

TestFoundation/TestFTP.swift

+        let dataTask1 = sesh.downloadTask(with: req, completionHandler: { url, res, error in
+            XCTAssertNil(error)
+            defer { expect.fulfill() }
+


Superflous blank line

alblue · 2017-09-15T11:03:22Z

TestFoundation/TestFTP.swift

+        let uploadTask = sesh.uploadTask(with: req, from: saveData, completionHandler: { data, res, error in
+            XCTAssertNil(error)
+            defer { expect.fulfill() }
+


Superflous blank line

alblue · 2017-09-15T11:03:33Z

TestFoundation/TestFTP.swift

+         waitForExpectations(timeout: 20)
+    }
+}
+


Superflous blank lines

alblue · 2017-09-15T11:03:53Z

TestFoundation/TestURLSession.swift

 }

 extension SharedDelegate: URLSessionDownloadDelegate {
    func urlSession(_ session: URLSession, downloadTask: URLSessionDownloadTask, didFinishDownloadingTo location: URL) {
    }
 }
+


Superflous blank lines

saiHemak · 2017-09-15T14:54:18Z

@alblue : Thanks for the review. I will be addressing the your comments on URL Session refactoring code through #1216

alblue · 2017-09-18T10:29:42Z

Sounds good - let's leave this as is for now, and after changes in #1216 come back and revisit this one?

saiHemak · 2017-09-18T11:04:21Z

@ALBUE: Sure

alblue · 2017-10-16T16:15:13Z

I'm of the opinion that leaving this PR open will result in confusion between this and the current version, which is in #1216. Seeing as we've still got a way to go before that one lands, and since the content in this pull request is out of date, I recommend we close this one, get #1216 addressed, and then you can rebase the changes on your ftpimplementation branch, which will hopefully be in a smaller state by then. It will also save going back through this review history (though you can reference it from your new PR for traceability if you want) since many of the comments (especially the formatting ones) will be irrelevant by then. @saiHemak does that sound OK?

alblue · 2017-11-06T11:35:45Z

Closing, and deferring future changes to subsequent PRs.

saiHemak force-pushed the ftpimplementation branch from 9772656 to 48668a4 Compare July 20, 2017 11:58

alblue self-requested a review July 20, 2017 18:14

alblue suggested changes Jul 20, 2017

View reviewed changes

pushkarnk reviewed Jul 21, 2017

View reviewed changes

xwu reviewed Jul 23, 2017

View reviewed changes

saiHemak force-pushed the ftpimplementation branch 2 times, most recently from 705dc5b to 39e96d8 Compare July 26, 2017 08:50

saiHemak force-pushed the ftpimplementation branch from 39e96d8 to 9fa771e Compare July 28, 2017 06:44

johnno1962 mentioned this pull request Aug 2, 2017

Rewrite of PR #1103 + addition Android specific patches #1113

Closed

saiHemak force-pushed the ftpimplementation branch from 9fa771e to 868ea8e Compare August 18, 2017 10:14

saiHemak force-pushed the ftpimplementation branch 4 times, most recently from ccc22de to e8fdce9 Compare September 4, 2017 09:04

saiHemak force-pushed the ftpimplementation branch 4 times, most recently from de89c00 to da13e79 Compare September 11, 2017 08:13

ianpartridge suggested changes Sep 12, 2017

View reviewed changes

saiHemak force-pushed the ftpimplementation branch from da13e79 to 1e0b318 Compare September 12, 2017 10:44

Initial implementation of FTP protocol

43bcd81

saiHemak force-pushed the ftpimplementation branch from 1e0b318 to 43bcd81 Compare September 12, 2017 10:51

saiHemak mentioned this pull request Sep 14, 2017

Refactoring URLSession for multiple native protocols #1216

Closed

saiHemak changed the title ~~[Work In Progress] Initial implementation of FTP~~ Initial implementation of FTP Sep 15, 2017

alblue suggested changes Sep 15, 2017

View reviewed changes

alblue closed this Nov 6, 2017

saiHemak mentioned this pull request Nov 27, 2017

Moving HTTPBodySource out of http folder and renaming to BodySource #1336

Merged

		@@ -0,0 +1,700 @@
		// Foundation/NSURLSession/TransferState.swift - NSURLSession & libcurl

Initial implementation of FTP #1122

Initial implementation of FTP #1122

Uh oh!

Conversation

saiHemak commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pushkarnk Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pushkarnk commented Jul 27, 2017

Uh oh!

ianpartridge commented Sep 11, 2017

Uh oh!

saiHemak commented Sep 12, 2017

Uh oh!

ianpartridge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saiHemak commented Sep 12, 2017

Uh oh!

saiHemak commented Sep 12, 2017

Uh oh!

saiHemak commented Sep 12, 2017

Uh oh!

ianpartridge commented Sep 12, 2017

Uh oh!

pushkarnk commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saiHemak commented Sep 14, 2017

Uh oh!

alblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

saiHemak commented Jul 20, 2017 •

edited

Loading

pushkarnk Jul 21, 2017 •

edited

Loading

pushkarnk commented Sep 14, 2017 •

edited

Loading