Make URLSession and URLSessionTask more source-compatible with Darwin #2587

broadwaylamb · 2019-12-13T12:19:28Z

I'd be happy to write tests, but what should we test here?

broadwaylamb · 2019-12-13T12:22:18Z

Foundation/URLSession/URLSession.swift

+
+    /*
+     * data task convenience methods.  These methods create tasks that
+     * bypass the normal delegate calls for response and data delivery,
+     * and provide a simple cancelable asynchronous interface to receiving
+     * data.  Errors will be returned in the NSURLErrorDomain,
+     * see <Foundation/NSURLError.h>.  The delegate, if any, will still be
+     * called for authentication challenges.
+     */
+    open func dataTask(with request: URLRequest, completionHandler: @escaping (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask {
+        return dataTask(with: _Request(request), behaviour: .dataCompletionHandler(completionHandler))
+    }
+
+    open func dataTask(with url: URL, completionHandler: @escaping (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask {
+        return dataTask(with: _Request(url), behaviour: .dataCompletionHandler(completionHandler))
+    }


Moved these from an extension to the class itself. With Darwin Foundation we're able to override these methods in subclasses. Same with downloadTask and uploadTask methods.

SE-0117 strikes again.

I think you can mark each method in the extension with open, and that should reduce the size of this PR and keep changes easier to see.

They were already open, it’s just that we can’t override methods declared in extensions.

Sorry, I didn't double checked my proposal.

I just tested in a small Xcode (Mac) project that reproduces the problem, and I get this error "Overriding non-@objc declarations from extensions is not supported". I tried adding a simple @objc in front of the extension in the Swift framework, and it seems to fix the problem.

Do you think that might be a better solution? I don't mind moving the methods into the main class code, but if someone wrote them as an extension for organization, I would prefer to keep them that one.

I’m afraid @objc won’t work on non-Darwin platforms.

That's true. What a strange limitation of extensions. I wonder why open there doesn't generate a warning, if it cannot be overriden anyway.

AFAIK this has to do with method dispatch. We can't retroactively add methods to class vtable, unless we use some completely different dispatching mechanism, like message passing in Objective-C. That's why @objc works — it relies on Objective-C runtime.

broadwaylamb · 2019-12-13T12:24:15Z

Foundation/URLSession/URLSession.swift

+    @available(*, unavailable, renamed: "getTasksWithCompletionHandler(_:)")
+    open func getTasksWithCompletionHandler(completionHandler: @escaping ([URLSessionDataTask], [URLSessionUploadTask], [URLSessionDownloadTask]) -> Void) {
+        getTasksWithCompletionHandler(completionHandler)
+    }
+
    /* invokes completionHandler with outstanding data, upload and download tasks. */
-    open func getTasksWithCompletionHandler(completionHandler: @escaping ([URLSessionDataTask], [URLSessionUploadTask], [URLSessionDownloadTask]) -> Void)  {
+    open func getTasksWithCompletionHandler(_ completionHandler: @escaping ([URLSessionDataTask], [URLSessionUploadTask], [URLSessionDownloadTask]) -> Void)  {


Darwin Foundation doesn't use an argument label.

Marked the old version as unavailable instead of deprecated, because in the latter case we have ambiguity if this method is used with trailing closure syntax.

broadwaylamb · 2019-12-13T12:25:29Z

Foundation/URLSession/URLSession.swift

@@ -268,7 +268,7 @@ open class URLSession : NSObject {
    }

    open private(set) var delegateQueue: OperationQueue
-    open var delegate: URLSessionDelegate?
+    open private(set) var delegate: URLSessionDelegate?


URLSession.delegate is get-only in Darwin Foundation.

Therefore, we should be able to override it with a get-only property. We can't do this with swift-corelibs-foundation now.

broadwaylamb · 2019-12-13T12:27:43Z

Foundation/URLSession/URLSessionTask.swift

@@ -376,7 +376,7 @@ open class URLSessionTask : NSObject, NSCopying {
    /*
     * The current state of the task within the session.
     */
-    open var state: URLSessionTask.State {
+    open fileprivate(set) var state: URLSessionTask.State {


URLSessionTask.state is get-only in Darwin Foundation.

broadwaylamb · 2020-01-06T11:28:38Z

Could anyone please verify this patch?

@phausler @parkera @millenomi @drodriguez

drodriguez

Thanks for the changes. They look OK and sensible to me. I would still defer to someone in the Apple team to express their opinions, but I would not mind merging these changes in the name of cross-compatibility.

About the missing unit tests, I don't think they are strictly necessary for these changes, but if you want to add them, here are some ideas.

I don't think the fileprivate(set) bits can be tested. It is a compilation error. No way to test that with XCTest. Hopefully the tests didn't use the inexistent API.
About getTaskWithCompletionHandler, I suppose the tests do not use the one with the parameter name (it is probably all trailing closures). It might not be bad to add or modify one of the tests to use an explicit parameter, instead of the trailing closure.
About the data/upload/downloadTask methods, one can check that overriding in subclasses is possible, and that nobody will break that support by adding a subclass and checking that the override is called thru (modify the returned taskDescription and check the modification in the returned value, for example).

But as I said above, I don't think they are strictly necessary.

@swift-ci please test Linux platform

drodriguez · 2020-01-07T20:32:24Z

Foundation/URLSession/URLSession.swift

+
+    /*
+     * data task convenience methods.  These methods create tasks that
+     * bypass the normal delegate calls for response and data delivery,
+     * and provide a simple cancelable asynchronous interface to receiving
+     * data.  Errors will be returned in the NSURLErrorDomain,
+     * see <Foundation/NSURLError.h>.  The delegate, if any, will still be
+     * called for authentication challenges.
+     */
+    open func dataTask(with request: URLRequest, completionHandler: @escaping (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask {
+        return dataTask(with: _Request(request), behaviour: .dataCompletionHandler(completionHandler))
+    }
+
+    open func dataTask(with url: URL, completionHandler: @escaping (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask {
+        return dataTask(with: _Request(url), behaviour: .dataCompletionHandler(completionHandler))
+    }


SE-0117 strikes again.

I think you can mark each method in the extension with open, and that should reduce the size of this PR and keep changes easier to see.

drodriguez · 2020-01-07T23:52:26Z

@swift-ci please test

broadwaylamb · 2020-01-16T12:54:13Z

Can anyone take a look and merge this please? @phausler @parkera @millenomi

phausler

The changes look reasonable to me

drodriguez · 2020-01-16T17:01:54Z

Merged. You might want to ask for a cherry-pick of this to the latest 5.x.y branch going on for Linux.

broadwaylamb commented Dec 13, 2019

View reviewed changes

Make URLSession and URLSessionTask more source-compatible with Darwin

8ca6c4d

broadwaylamb force-pushed the urlsession-fixes branch from efd27ea to 8ca6c4d Compare December 13, 2019 12:29

drodriguez reviewed Jan 7, 2020

View reviewed changes

drodriguez approved these changes Jan 8, 2020

View reviewed changes

phausler approved these changes Jan 16, 2020

View reviewed changes

drodriguez merged commit 12e60f6 into swiftlang:master Jan 16, 2020

broadwaylamb mentioned this pull request Jan 29, 2020

[5.2] Make URLSession and URLSessionTask more source-compatible with Darwin #2634

Closed

broadwaylamb mentioned this pull request Jun 24, 2020

Re-enable URLSession tests on non-Darwin platforms OpenCombine/OpenCombine#169

Closed

Make URLSession and URLSessionTask more source-compatible with Darwin #2587

Make URLSession and URLSessionTask more source-compatible with Darwin #2587

Uh oh!

Conversation

broadwaylamb commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

broadwaylamb Dec 13, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

broadwaylamb Dec 13, 2019 • 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!

broadwaylamb Dec 13, 2019 • 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!

broadwaylamb commented Jan 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drodriguez commented Jan 7, 2020

Uh oh!

broadwaylamb commented Jan 16, 2020

Uh oh!

phausler left a comment

Choose a reason for hiding this comment

Uh oh!

drodriguez commented Jan 16, 2020

Uh oh!

Uh oh!

broadwaylamb commented Dec 13, 2019 •

edited

Loading

broadwaylamb Dec 13, 2019 •

edited

Loading

broadwaylamb Dec 13, 2019 •

edited

Loading

broadwaylamb Dec 13, 2019 •

edited

Loading

broadwaylamb commented Jan 6, 2020 •

edited

Loading