Implementation of URLProtocol and refactoring URLSession #968

nethraravindran · 2017-04-26T12:37:35Z

Currently URLSessionTask is tightly coupled with HTTP/HTTPS specific functions. This makes it difficult to introduce support for other native and custom protocols. Refactoring URLSession helps in adding support for other native protocols like ftp and data as well. Also, URLProtocol implementation provides support for custom protocols.

This document provides the proposal for restructuring URLSession
https://gist.github.com/pushkarnk/1bae3167d2e62b09ba0dd4b0dd285179

ianpartridge · 2017-04-26T14:23:12Z

Foundation/NSURLProtocol.swift

+    open class func registerClass(_ protocolClass: AnyClass) -> Bool {
+        if protocolClass is URLProtocol.Type {
+            guard !_registeredProtocolClasses.contains(where: { $0 === protocolClass }) else { return true }
+            _lock.lock()


I think this needs to go before the guard otherwise threads can still race?

ianpartridge · 2017-04-26T14:23:39Z

Foundation/NSURLProtocol.swift

+    }
+
+    // TODO: Synchronize all the class funcs that use the registered protocol classes array
+    internal class func getProtocolClass(for urlRequest: URLRequest) -> AnyClass? {


Locking needed here too.

ianpartridge · 2017-04-26T14:26:48Z

Foundation/NSURLProtocol.swift

-
+
+    private static var _registeredProtocolClasses = [AnyClass]()
+    private static var _lock = NSLock()


Perhaps call this _classesLock?

pushkarnk · 2017-04-27T08:26:03Z

TestFoundation/main.swift

@@ -80,7 +80,7 @@ XCTMain([
    testCase(TestNSURLResponse.allTests),
    testCase(TestNSHTTPURLResponse.allTests),
 //Disabling because of https://bugs.swift.org/browse/SR-4655 and https://bugs.swift.org/browse/SR-4647


This comment needs to be removed if we are enabling the tests

I don't think we should re-enable the tests under this PR.

pushkarnk · 2017-04-27T09:43:48Z

Foundation/NSURLProtocol.swift

+        }
+    }
+
+    func urlProtocol(_ protocol: URLProtocol, didFailWithError error: Error) {


nit: Will it be possible to factor out some code this function shares with urlProtocol(_ protocol: URLProtocol, didFailWithError error: Error)?

pushkarnk · 2017-04-27T09:46:32Z

Foundation/NSURLProtocol.swift

-    open class func unregisterClass(_ protocolClass: AnyClass) { NSUnimplemented() }
+    open class func unregisterClass(_ protocolClass: AnyClass) {
+        if let idx = _registeredProtocolClasses.index(where: { $0 === protocolClass }) {
+            _classesLock.lock()


The lock acquisition needs to be raised above the if, given that _registeredProtocolClasses may be concurrently accessed

pushkarnk · 2017-04-27T09:48:32Z

Foundation/NSURLSession/NSURLSession.swift

@@ -219,6 +219,11 @@ open class URLSession : NSObject {
        let c = URLSession._Configuration(URLSessionConfiguration: configuration)
        self._configuration = c
        self.multiHandle = _MultiHandle(configuration: c, workQueue: workQueue)
+        // registering all the protocol classes with URLProtocol
+        let _ = URLProtocol.registerClass(_HTTPURLProtocol.self)


I am wondering if we want to register the native protocol classes every time a new URLSession is created. This can be a one-time activity.

pushkarnk · 2017-04-27T09:48:55Z

Foundation/NSURLSession/NSURLSession.swift

@@ -234,6 +239,11 @@ open class URLSession : NSObject {
        let c = URLSession._Configuration(URLSessionConfiguration: configuration)
        self._configuration = c
        self.multiHandle = _MultiHandle(configuration: c, workQueue: workQueue)
+        // registering all the protocol classes with URLProtocol
+        let _ = URLProtocol.registerClass(_HTTPURLProtocol.self)


I am wondering if we want to register the native protocol classes every time a new URLSession is created. This can be a one-time activity.

pushkarnk · 2017-04-27T09:51:02Z

Foundation/NSURLSession/NSURLSessionTask.swift

-        self.easyHandle = _EasyHandle(delegate: self)
+        if let urlProtocolClass = URLProtocol.getProtocolClass(for: request) {
+            if urlProtocolClass is URLProtocol.Type {
+                self._protocol = (urlProtocolClass as! URLProtocol.Type).init(task: self, cachedResponse: nil, client: nil)


nit: Can we separate the downcast on a different line of code ? Though this is fine, I'm for better readability!

pushkarnk · 2017-04-27T09:52:13Z

Foundation/NSURLSession/NSURLSessionTask.swift

@@ -149,14 +117,20 @@ open class URLSessionTask : NSObject, NSCopying {
        set { taskAttributesIsolation.async(flags: .barrier) { self._response = newValue } }
    }
    fileprivate var _response: URLResponse? = nil
+    internal var setResponse : URLResponse? {


Can't we call this simply response ?

pushkarnk · 2017-04-27T09:54:28Z

Foundation/NSURLSession/http/HTTPURLProtocol.swift

+            suspend()
+        } else {
+            self.internalState = .transferFailed
+            completeTask(withError: (self.task?.error)!)


We need to avoid this unguarded unwrapping

pushkarnk · 2017-04-27T09:55:44Z

Foundation/NSURLSession/http/HTTPURLProtocol.swift

+        guard let url = request.url else { fatalError("No URL in request.") }
+
+        self.internalState = .transferReady(createTransferState(url: url, workQueue: (task?.workQueue)!))
+        if (task?.suspendCount)! < 1 {


Will it be possible to avoid this unchecked unwrapping?

pushkarnk · 2017-04-27T09:58:36Z

@nethraravindran Since some new files have been created, I guess you'd need to update the Foundation.xcodeproj file and include those changes as well.

pushkarnk · 2017-04-27T10:04:44Z

Foundation/NSURLProtocol.swift

+
+    func urlProtocolDidFinishLoading(_ protocol: URLProtocol) {
+        guard let session = `protocol`.task?.session as? URLSession else { fatalError() }
+        switch session.behaviour(for: `protocol`.task!) {


May be we should set a guard for protocol.task and generate a fatalError if its is nil? We can then access task in the remainder of the method without worries of nil :)

pushkarnk · 2017-05-03T09:46:01Z

@swift-ci please test

pushkarnk · 2017-05-03T09:48:10Z

@swift-ci test

ianpartridge · 2017-05-03T13:30:39Z

@parkera We think this is ready for merge. Can you give your view? Thanks.

parkera · 2017-05-04T17:59:49Z

If you're happy with it, go ahead.

weissi · 2017-05-05T15:28:48Z

Foundation/NSURLProtocol.swift

+        if protocolClass is URLProtocol.Type {
+            _classesLock.lock()
+            guard !_registeredProtocolClasses.contains(where: { $0 === protocolClass }) else {
+                _classesLock.unlock()


I'd use

defer { _classesLock.unlock() }

here and in the following cases, otherwise it looks quite easy that a further refactoring forgets to unlock...

I remembered what @parkera had to say about this :)
#902 (comment)

very surprising but fair enough, thanks for pointing that out 👍

ianpartridge · 2017-05-05T15:38:07Z

Thanks @parkera we're still looking at the URLSession test failures that showed up in the CI so will wait until we understand those a bit better.

pushkarnk · 2017-05-31T06:37:54Z

Foundation/NSURLSession/http/HTTPURLProtocol.swift

+        //set the request timeout
+        //TODO: the timeout value needs to be reset on every data transfer
+
+        var timeoutInterval = Int(httpSession.configuration.timeoutIntervalForRequest) * 1000


This timeout logic needs to be moved to URLSessionTask since it is not something specific to HTTP. We can do this in a separate PR though.

pushkarnk · 2017-05-31T07:06:53Z

Foundation/NSURLSession/NSURLSessionTask.swift

-    fileprivate let body: _Body
-    fileprivate let tempFileURL: URL
+    internal var suspendCount = 1
+    internal var totalDownloaded = 0


This is specific to the http download tasks. We'd need to consider moving it to the HTTP implementation in future.

nethraravindran · 2017-05-31T07:07:45Z

Rebased to the latest master and fixed few bugs. Ready for test and merge.

pushkarnk · 2017-05-31T07:19:14Z

@swift-ci please test

pushkarnk · 2017-05-31T08:29:36Z

cc @nethraravindran build failures due to some compilation errors and warning

pushkarnk · 2017-05-31T09:39:58Z

@swift-ci please test

pushkarnk · 2017-05-31T09:46:33Z

@swift-ci please test

pushkarnk · 2017-05-31T10:24:03Z

Another TODO: There's a lot of places where we're doing optional chaining. I understand that we will not have nil values at these points under normal circumstances. But for those abnormal nils that may come up once in a while, we have to define clear code paths (may be a guard withfatalError()).

Since the tests are passing and the code changes are big, I am fine with merging this now. But it does warrant some future improvement work. Thanks!

ianpartridge reviewed Apr 26, 2017

View reviewed changes

pushkarnk reviewed Apr 27, 2017

View reviewed changes

weissi reviewed May 5, 2017

View reviewed changes

jey mentioned this pull request May 8, 2017

Add Linux test runner dduan/Just#80

Closed

pushkarnk reviewed May 31, 2017

View reviewed changes

URLProtocol implementation and refactoring URLSession

ec515ac

pushkarnk merged commit 5b1b508 into swiftlang:master May 31, 2017

pushkarnk mentioned this pull request Jun 19, 2017

[regression]Fix for test_finishTaskAndInvalidate failure #1049

Merged

pushkarnk mentioned this pull request Sep 5, 2017

URLSessionTask retain cycle and memory leak fix - all Linuxies #1195

Merged

saiHemak mentioned this pull request Sep 14, 2017

Refactoring URLSession for multiple native protocols #1216

Closed



		private static var _registeredProtocolClasses = [AnyClass]()
		private static var _lock = NSLock()

Implementation of URLProtocol and refactoring URLSession #968

Implementation of URLProtocol and refactoring URLSession #968

Uh oh!

Conversation

nethraravindran commented Apr 26, 2017

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

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pushkarnk commented May 3, 2017

Uh oh!

pushkarnk commented May 3, 2017

Uh oh!

ianpartridge commented May 3, 2017

Uh oh!

parkera commented May 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pushkarnk May 5, 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!

ianpartridge commented May 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pushkarnk May 31, 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!

nethraravindran Jun 6, 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!

nethraravindran commented May 31, 2017

Uh oh!

pushkarnk commented May 31, 2017

Uh oh!

pushkarnk commented May 31, 2017

Uh oh!

pushkarnk commented May 31, 2017

Uh oh!

pushkarnk commented May 31, 2017

Uh oh!

pushkarnk commented May 31, 2017

pushkarnk commented Apr 27, 2017 •

edited

Loading

pushkarnk May 5, 2017 •

edited

Loading

pushkarnk May 31, 2017 •

edited

Loading

nethraravindran Jun 6, 2017 •

edited

Loading