Basic NSURLSession implementation #299

danieleggert · 2016-03-23T17:35:14Z

I'm creating this pull request to get feedback on my implementation so far.

There are still many TODOs in the code, but it’s far along enough to evaluate the architecture and design I have chosen for this task. I’ve tried to outline quite a bit in the code itself in form of comments. Please let me know:

which parts need more explanation
which parts should be redesigned
which parts need more testing

The NSURLSession.swift file has a NSURLSession API implementation overview.

As for testing, I'm hoping to spend quite a bit of time on building an in-process HTTP server to better test some of the desired behaviour. Particularly when testing things such as different payloads, headers, and POST / PUT requests that's the only way to get good testing.

I'd like to split the NSURLSession+libcurl.swift file into multiple smaller files, but that'd break with the existing style. Any thoughts?

The CFURLSessionInterface.{c,h} files are very verbose in order to not expose libcurl directly. This was suggested by @phausler and @rothomp3 — but I'm not sure if this is what they were hoping for.

All unimplemented code should still be marked with NSUnimplemented() where applicable.

This does not at all try to implement so-called background session which would have
to rely on a system daemon to perform HTTP requests white the app is not running (either
suspended or terminated). Such code would have to rely on IPC and the launchd equivalent
on the platform it is running on.

phausler · 2016-03-23T17:42:04Z

TestFoundation/main.swift

+//    testCase(TestNSValue.allTests),
+//    testCase(TestNSUserDefaults.allTests),
+//    testCase(TestNSXMLParser.allTests),
+//    testCase(TestNSXMLDocument.allTests),


obviously these will all have to be re-enabled before merging is considered.

danieleggert · 2016-03-23T17:50:09Z

I should note that this relies on https://github.com/apple/swift-corelibs-libdispatch — and it has an extremely rudimentary version of NSOperationQueue to support the delegate queue on NSURLSession.

phausler · 2016-03-23T17:50:34Z

CoreFoundation/URL.subproj/CFURLSessionInterface.h

+    CFURLSession_EasyCodeNO_CONNECTION_AVAILABLE, // CURLE_NO_CONNECTION_AVAILABLE
+    CFURLSession_EasyCodeSSL_PINNEDPUBKEYNOTMATCH, // CURLE_SSL_PINNEDPUBKEYNOTMATCH
+    CFURLSession_EasyCodeSSL_INVALIDCERTSTATUS, // CURLE_SSL_INVALIDCERTSTATUS
+} CFURLSession_EasyCode;


it is worth noting that when porting to Linux these will not be exported in the same manner as enums on Darwin. We have numerous other examples that show the #if os(OSX) || os(iOS) re-export as constant pattern that must be done. it might be worthwhile to modify that script that generates these to generate those as well.

Makes sense. I’ll deal with that once libdispatch lands for Linux — until then it won’t compile either way.

I changed this to avoid using enum in C.

phausler · 2016-03-23T17:52:32Z

per the NSOQ stub (and other dispatch usage) we are still waiting on libdispatch to completely land; hopefully before then I will have some more things for NSOQ that will give a decent implementation for something more complete than what you have so far.

danieleggert · 2016-04-04T18:39:33Z

Question: What changes should be done on this before it can be merged back into master?
One thing I'm aware of: this needs libdispatch, and that's not there, yet, on Linux. What’s a reasonable workaround? Or should we wait?

Also, I'd like feedback on the fact that NSURLSession.swift and NSURLSession+libcurl.swift are now 2090 and 1741 lines respectively, and that it may make sense to break these into multiple smaller files. Any thoughts?

parkera · 2016-04-04T23:05:06Z

We're still working on dispatch; it's certainly the plan for it to be available. We should proceed as if it'll be there in the end.

I'm ok with splitting implementation into multiple files as well.

Thanks for your work on this @danieleggert.

danieleggert · 2016-04-06T08:29:18Z

I’ve split it into these files

NSURLSession.swift
NSURLSessionConfiguration.swift
NSURLSessionDelegate.swift
NSURLSessionTask.swift
Configuration.swift
EasyHandle.swift
HTTPBodySource.swift
HTTPMessage.swift
MultiHandle.swift
TaskRegistry.swift
TransferState.swift
libcurlHelpers.swift

parkera · 2016-04-06T17:57:24Z

Foundation.xcodeproj/project.pbxproj

@@ -2150,6 +2226,7 @@
 					r,
 					r,
 				);
+				OTHER_SWIFT_FLAGS = "-DDEPLOYMENT_ENABLE_LIBDISPATCH";


We should probably leave this out until everything is ready to go right?

parkera · 2016-04-06T17:59:40Z

I'd like to double check a few things:

Is this disabled by default on Linux, since dispatch is not yet integrated?
I'm unsure about enabling by default on Darwin. I suppose that's fine, since it's for our own purposes.
The tests should only run if it's enabled (obviously).
Can we remove some of the scheme settings?

Once we're set, we'll squash merge this in so we can work on it iteratively after that (including enabling on Linux).

danieleggert · 2016-04-06T18:02:26Z

@parkera This all assumes that libdispatch is available on Linux — we’ll have to hold off until then. I’ll do a few more tweaks to this tomorrow and the day after and then squash everything into a single commit.

One option would be to add the old version of NSURLSession.swift as NSURLSession+Unimplemented.swift and use that one for Linux. Thoughts?

danieleggert · 2016-04-06T21:54:50Z

It’s all squashed into a single commit now, but I might keep working on this.

danieleggert · 2016-04-06T21:56:01Z

Note that this includes #306 and #287 — those should be merged first, or this one changed if they’re not to be merged.

czechboy0 · 2016-04-22T07:47:45Z

@danieleggert Conflicts :(

dtorres · 2016-05-03T15:33:50Z

CoreFoundation/URL.subproj/CFURLSessionInterface.c

+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// This file contains wrappes / helpers to import libcurl into Swift.


typo here (wrappers)

danieleggert · 2016-05-10T15:58:11Z

I’ll make sure conflicts get resolved once libdispatch is working.

mominul · 2016-08-02T10:55:15Z

@danieleggert Is this pull request ready to merge?

MaxDesiatov · 2016-08-08T15:31:46Z

any updates on this PR?

parkera · 2016-10-06T17:01:22Z

@danieleggert Thanks for your work on this. I believe it's been merged as part of the later work.

sgharms · 2016-10-06T20:38:35Z

@parkera Do we know which release of swift will have this content merged in?

parkera · 2016-10-06T21:16:45Z

It's in Swift 3 actually. Give it a shot and let us know on the mailing lists if you have any problems.

seabaylea · 2016-10-07T08:11:17Z

@sgharms The main content went into Swift 3, and there's been some addition bug fixes in the 3.0.1 previews. There's still some missing function - particularly things like task cancellation. Having people "kick the tires" and provide tests for anything that doesn't work would be really useful.

sgharms · 2016-10-26T19:57:51Z

@seabaylea

My apologies for slow reply here. I went on vacation 😎 .

Let me admit that there's a great chance I'm exposing my profound ignorance here, so I hope I'll not embarrass myself too much.

My Environment

➜  octopress git:(master) ✗ swift --version
Swift version 3.0 (swift-3.0-RELEASE)
Target: x86_64-unknown-linux-gnu
➜  octopress git:(master) ✗ cat /etc/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=16.04
DISTRIB_CODENAME=xenial
DISTRIB_DESCRIPTION="Ubuntu 16.04.1 LTS"

Code

        import Foundation
        let config = URLSessionConfiguration.default // Session Configuration
        let session = URLSession(configuration: config) // Load configuration into Session
        let url = URL(string: "http://apple.com")!

        let task = session.dataTask(with: url, completionHandler: {
            (data, response, error) in
            print("Callback ran")

        })
        task.resume()

        print("End of line")

Output

End of line

Error

I never see "Callback ran"

Conclusion

Based on the stack trace it would seem to me that using Swift to accomplish basic HTTP operations is not supported on my platform.

sgharms · 2016-10-26T20:42:24Z

As supplement, I asked someone with 3.0 Mac to run the same code and got the following:

I suspect the (suspended) note indicates that Playgrounds isn't working with the async call?

In any case, I find the behavior in both situations surprising and didn't see warnings around this mentioned in the guide. Am I missing something?

dtorres · 2016-10-26T20:53:32Z

@sgharms I am assuming you are running this in the CLI in linux. well, the request doesn't get fired because the program has already exited. You need to maintain the program running until (at least) the request has had a chance to run to see the results. Look into NSRunLoop or dispatch_main(). At least one of them should be available in Linux's Swift environment

sgharms · 2016-10-26T20:55:24Z

Ah! OK. I had wrongly assumed that there was a default run loop that would make sure all the spawned tasks closed. Thanks.

[SKSwiftPMWorkspace] Set swiftpm config file when creating Workspace

phausler reviewed Mar 23, 2016
View reviewed changes

danieleggert mentioned this pull request Mar 23, 2016

Add 5 missing attributes to NSURLRequest #290

Closed

phausler reviewed Mar 23, 2016
View reviewed changes

danieleggert force-pushed the urlsession branch 3 times, most recently from f9dd2c3 to 85abca1 Compare April 1, 2016 15:58

danieleggert force-pushed the urlsession branch 2 times, most recently from 9d087d0 to 63a7b48 Compare April 4, 2016 17:46

danieleggert changed the title ~~[WIP] Basic NSURLSession implementation~~ Basic NSURLSession implementation Apr 4, 2016

parkera reviewed Apr 6, 2016
View reviewed changes

danieleggert force-pushed the urlsession branch 3 times, most recently from 6d3cb17 to 836b865 Compare April 6, 2016 21:53

danieleggert force-pushed the urlsession branch 3 times, most recently from d639e9c to 6f151ae Compare April 14, 2016 11:27

Basic NSURLSession API.

05bcbcd

danieleggert force-pushed the urlsession branch from 6f151ae to 05bcbcd Compare April 15, 2016 09:40

dtorres reviewed May 3, 2016
View reviewed changes

parkera closed this Oct 6, 2016

atrick pushed a commit to atrick/swift-corelibs-foundation that referenced this pull request Jan 12, 2021

Merge pull request swiftlang#299 from apple/swiftpm-config

4fcf071

[SKSwiftPMWorkspace] Set swiftpm config file when creating Workspace

Basic NSURLSession implementation #299

Basic NSURLSession implementation #299

Uh oh!

Conversation

danieleggert commented Mar 23, 2016

Uh oh!

phausler Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

danieleggert commented Mar 23, 2016

Uh oh!

phausler Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

danieleggert Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

danieleggert Apr 4, 2016

Choose a reason for hiding this comment

Uh oh!

phausler commented Mar 23, 2016

Uh oh!

danieleggert commented Apr 4, 2016

Uh oh!

parkera commented Apr 4, 2016

Uh oh!

danieleggert commented Apr 6, 2016

Uh oh!

parkera Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

parkera commented Apr 6, 2016

Uh oh!

danieleggert commented Apr 6, 2016

Uh oh!

danieleggert commented Apr 6, 2016

Uh oh!

danieleggert commented Apr 6, 2016

Uh oh!

czechboy0 commented Apr 22, 2016

Uh oh!

dtorres May 3, 2016

Choose a reason for hiding this comment

Uh oh!

danieleggert commented May 10, 2016

Uh oh!

mominul commented Aug 2, 2016

Uh oh!

MaxDesiatov commented Aug 8, 2016

Uh oh!

parkera commented Oct 6, 2016

Uh oh!

sgharms commented Oct 6, 2016

Uh oh!

parkera commented Oct 6, 2016

Uh oh!

seabaylea commented Oct 7, 2016

Uh oh!

sgharms commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

My Environment

Code

Output

Error

Conclusion

Uh oh!

sgharms commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtorres commented Oct 26, 2016

Uh oh!

sgharms commented Oct 26, 2016

Uh oh!

Uh oh!

sgharms commented Oct 26, 2016 •

edited

Loading

sgharms commented Oct 26, 2016 •

edited

Loading