Updating PR 299 (NSURLSession) to work with the latest Foundation #426

pushkarnk · 2016-06-28T12:14:34Z

Creating this PR for review, on @parkera 's suggestion. The changes that this PR does to pull request 299 include:

Rebasing to the latest Foundation
Addition of support for download task and upload task (with a completion handler)
Enabling building on Ubuntu 14.04 (which has an older libcurl by default)
Renaming internal types (classes, struct, enums and protocols) to start with _

Work in progress:

Unit tests contributed to TestFoundation as a part of pull request 299 - these utilise types internal to Foundation - don't build on Linux. They need rework.
Replacing uses of dispatch_data by NSData/Data backed by dispatch_data

Kindly review and comment. Thanks.

parkera · 2016-06-28T15:54:26Z

Foundation/NSURLSession/EasyHandle.swift

+         let errorCode: Int?
+     }
+ }
+ */


Let's not check in commented out code, since its purpose is unclear.

pushkarnk · 2016-06-28T19:33:10Z

Thanks for the comments. I started with a round of deleting commented code with no message.

pushkarnk · 2016-06-29T20:18:49Z

I addressed a few of the comments today:

Use of String.init?(bytes:encoding:) and String.init?(cString:encoding:) to create Strings from char pointers received from the CURL layer
Adhering to the new naming conventions
Moved some constants in HTTMessage inside a struct
Replaced the use of a [UnicodeScalar] by NSCharacterSet

phausler · 2016-06-29T20:22:38Z

build.py

 ]

 if "XCTEST_BUILD_DIR" in Configuration.current.variables:
 	swift_cflags += [
 		'-I${XCTEST_BUILD_DIR}',
 		'-L${XCTEST_BUILD_DIR}',
-		'-I${SYSROOT}/usr/include/libxml2'
+		'-I${SYSROOT}/usr/include/libxml2',
+                '-I/usr/include/curl'


This should also be ${SYSROOT} attached just like the libxml case so that way cross compile development can specify a root

pushkarnk · 2016-07-01T03:03:15Z

More changes:

Tried to replace CInt with Int but that can't be done everywhere since libcurl accepts and returns Int32s.
Replace NSURL with struct URL.

pushkarnk · 2016-07-04T06:18:48Z

I have replaced Unsafe pointers with struct Data in all places except for the libcurl callbacks

parkera · 2016-07-06T16:00:58Z

Ok - I want to merge this but I have one strict requirement - we need at least the start of a test suite. Is it possible to get some kind of straightforward loop-back test in place so that we know we aren't going to accidentally break this with changes in the future?

pushkarnk · 2016-07-06T16:10:29Z

Sure @parkera I will work on getting some of Daniel's tests running.

pushkarnk · 2016-07-08T07:23:15Z

@parkera @phausler : Whats the equivalent of Xcode's "Enable Testability" on Linux? All of Daniel's test cases could be added here if we could get the @testable import Foundation to compile, since most of his tests use internal classes. I see this error currently:

TestFoundation/TestNSURLSession.swift:17:22: error: module 'Foundation' was not compiled for testing
    @testable import Foundation

parkera · 2016-07-08T18:24:59Z

Isn't the main purpose of @testable to expose some otherwise-private implementation details? Do the tests use those, or do they primarily test the public interface?

pushkarnk · 2016-07-08T18:53:16Z

Some tests use the internal classes to test the public API. Also, there are tests for the internal classes too. We'd need @testable for both

parkera · 2016-07-08T19:28:14Z

IIRC the challenge with @testable on Linux was that the build bot would have to build it this way when running its own tests, but we don't want that to be the way it's built for final consumption...

pushkarnk · 2016-07-08T19:54:29Z

I see that -enable-testing is the flag. If you think using this isn't a good idea, the other option is to rewrite the tests.

danieleggert · 2016-07-17T18:15:56Z

Just seeing this now. I’ve started rebasing my old PR, too.

pushkarnk · 2016-07-20T14:37:17Z

@seabaylea @parkera - As per your suggestions I have done the following:

Migrated to the new Dispatch API
Added 6 basic tests (4 data task + 2 download task) with external URLs - TestURLSession.swift. I think uploadTask tests need to be done via the loopback way.
Given that (2) is a stopgap way of testing URLSession, opened a Jira task requesting for loopback tests contributed from the community - https://bugs.swift.org/browse/SR-2118

I've also rebased to the latest Foundation.

parkera · 2016-07-20T17:28:41Z

Awesome. Kicking off a test.

parkera · 2016-07-20T17:28:44Z

@swift-ci please test

seabaylea · 2016-08-22T16:20:19Z

@parkera "We're going to need a remote URL that's as close to 100% reliable as possible." - agreed, as well as building in some resiliency to ignore requests that timeout (rather than an actual failure).

parkera · 2016-08-22T16:47:37Z

Hm, it failed again

Test Case 'TestURLSession.test_dataTaskWithURLRequest' started at 16:00:24.139
TestFoundation/TestNSURLSession.swift:72: error: TestURLSession.test_dataTaskWithURLRequest : Asynchronous wait failed - Exceeded timeout of 5.0 seconds, with unfulfilled expectations: data task
TestFoundation/TestNSURLSession.swift:73: error: TestURLSession.test_dataTaskWithURLRequest : XCTAssertEqual failed: ("unknown") is not equal to ("Lima") - test_dataTaskWithURLRequest returned an unexpected result

seabaylea · 2016-08-22T16:56:53Z

I'm taking a look at whether we can ignore cases where it times out - unfortunately timeout isn't yet implemented (although that looks pretty straightforward to add).

phausler · 2016-08-22T17:02:48Z

I was getting similar failures when I was merging it in previously. IIRC it had to do with the JSON getting emitted differently from the id to Any conversions

phausler · 2016-08-22T17:03:53Z

Also this PR really needs updates to the xcode project; we should be able to test and run this on Darwin in addition to Linux.

jrose-apple · 2016-08-22T17:21:58Z

Ah, is the Dispatch overlay also rebuilt before you recompile Foundation? I remember we had odd ordering around that at one point.

parkera · 2016-08-22T17:30:40Z

@jrose-apple I'm not sure how to tell. Can you look at the build log and let me know what you think?

seabaylea · 2016-08-22T17:36:07Z

@parkera @jrose-apple whilst foundation is in build-presets.ini before libdispatch (I've always done it the other way in local builds), from the CI build log it looks to me like libdispatch, including the overlay, builds first regardless.

seabaylea · 2016-08-22T17:40:01Z

If we want to add timeout support, and use that to add either retry logic or ignore the test, it looks like we just need to add the following at line 243 in MultiHandle.swift:

        case (CFURLSessionEasyCodeCOULDNT_CONNECT, ETIMEDOUT):
            return NSURLErrorTimedOut

jrose-apple · 2016-08-22T20:38:05Z

Okay, yeah, it looks like it's building in the right order. Actually, it looks like it's finding the dispatch headers, but not the overlay. Maybe that's not in the include paths for swiftpm?

jrose-apple · 2016-08-22T20:39:37Z

(The reason the compiler doesn't catch the missing overlay is because I wanted people to feel free to move things between their Swift and C code when it was safe to do so.)

parkera · 2016-08-22T20:45:36Z

@pushkarnk Can you update the PR as @seabaylea recommends? Once the test passes reliably we can merge this.

seabaylea · 2016-08-23T12:21:08Z

@jrose-apple thanks - that was it. I've raised PRs against SwiftPM and XCTest to add the build-time location of the Swift 3 overlay to their build/test-time include paths:
swiftlang/swift-package-manager#615
swiftlang/swift-corelibs-xctest#169

jrose-apple · 2016-08-23T17:56:04Z

How does this work when we build a toolchain package? Is the Dispatch overlay installed next to the standard library as it is on Apple platforms?

seabaylea · 2016-08-23T18:03:46Z

For the toolchain we install:

./lib/swift/dispatch/dispatch.h
./lib/swift/linux/libdispatch.la
./lib/swift/linux/libdispatch.so
./lib/swift/shims/DispatchShims.h
./lib/swift/linux/x86_64/Dispatch.swiftdoc
./lib/swift/linux/x86_64/Dispatch.swiftmodule

which matches the location of the other libraries

Also includes support for downloadTask and uploadTask with Data and a completion handler. The original PR 299 doesn't build on Ubuntu 14.04, due to an older level of libcurl. This rework removes some of that code (it does not affect any functionality). Updated to use the Dispatch 3.0 API. Includes basic tests with external URLs.

pushkarnk · 2016-08-23T20:49:20Z

@parkera , @seabaylea : I have added some resiliency in the tests by handling timeouts. I also had to add a basic implementation for URLSessionConfiguration.timeoutIntervalForRequest

Can we test again please, to see how things go?

parkera · 2016-08-23T21:17:36Z

@swift-ci please test linux

parkera · 2016-08-23T22:01:10Z

It passed!

pushkarnk · 2016-08-24T05:42:04Z

Thanks @parkera

@phausler - I'll work on updating the Xcode project next.

pushkarnk · 2016-08-24T06:17:52Z

Appears #586 does it. Thanks.

modocache · 2016-08-24T16:30:26Z

One of the tests added in this pull request, TestURLSession.test_dataTaskWithURLCompletionHandler, has begun failing. Apparently when this test was written, https://restcountries.eu/rest/v1/name/USA?fullText=true returned "Washington D.C.". It now returns "Washington, D.C.".

/cc @pushkarnk @parkera

parkera · 2016-08-24T16:31:05Z

It looks like #426 addresses that.

modocache · 2016-08-24T16:31:34Z

Yup, my bad. Sorry for the noise!

parkera · 2016-08-24T16:48:41Z

Sorry, it's #590 actually.

tanner0101 · 2016-08-25T16:51:48Z

Will this PR be included in the swift-3.0-branch?

parkera · 2016-08-25T17:18:56Z

We're planning on merging master to swift-3.0-branch once things are settled down a bit.

parkera reviewed Jun 28, 2016
View reviewed changes

phausler reviewed Jun 29, 2016
View reviewed changes

parkera merged commit c26f465 into swiftlang:master Aug 23, 2016

pushkarnk deleted the nsurlsession-new branch August 24, 2016 06:37

rhyshort mentioned this pull request Aug 24, 2016

Linux Support cloudant/swift-cloudant#121

Closed

Updating PR 299 (NSURLSession) to work with the latest Foundation #426

Updating PR 299 (NSURLSession) to work with the latest Foundation #426

Uh oh!

Conversation

pushkarnk commented Jun 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parkera Jun 28, 2016

Choose a reason for hiding this comment

Uh oh!

pushkarnk commented Jun 28, 2016

Uh oh!

pushkarnk commented Jun 29, 2016

Uh oh!

phausler Jun 29, 2016

Choose a reason for hiding this comment

Uh oh!

pushkarnk commented Jul 1, 2016

Uh oh!

pushkarnk commented Jul 4, 2016

Uh oh!

parkera commented Jul 6, 2016

Uh oh!

pushkarnk commented Jul 6, 2016

Uh oh!

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

Uh oh!

Uh oh!

parkera commented Jul 8, 2016

Uh oh!

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

Uh oh!

Uh oh!

parkera commented Jul 8, 2016

Uh oh!

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

Uh oh!

Uh oh!

danieleggert commented Jul 17, 2016

Uh oh!

pushkarnk commented Jul 20, 2016

Uh oh!

parkera commented Jul 20, 2016

Uh oh!

parkera commented Jul 20, 2016

Uh oh!

seabaylea commented Aug 22, 2016

Uh oh!

parkera commented Aug 22, 2016

Uh oh!

seabaylea commented Aug 22, 2016

Uh oh!

phausler commented Aug 22, 2016

Uh oh!

phausler commented Aug 22, 2016

Uh oh!

jrose-apple commented Aug 22, 2016

Uh oh!

parkera commented Aug 22, 2016

Uh oh!

seabaylea commented Aug 22, 2016

Uh oh!

seabaylea commented Aug 22, 2016

Uh oh!

jrose-apple commented Aug 22, 2016

Uh oh!

jrose-apple commented Aug 22, 2016

Uh oh!

parkera commented Aug 22, 2016

Uh oh!

seabaylea commented Aug 23, 2016

Uh oh!

jrose-apple commented Aug 23, 2016

Uh oh!

seabaylea commented Aug 23, 2016

Uh oh!

pushkarnk commented Aug 23, 2016

Uh oh!

parkera commented Aug 23, 2016

Uh oh!

pushkarnk commented Jun 28, 2016 •

edited

Loading

pushkarnk commented Jul 8, 2016 •

edited

Loading

pushkarnk commented Jul 8, 2016 •

edited

Loading

pushkarnk commented Jul 8, 2016 •

edited

Loading