Skip to content

Swift 3 API Parity: Use URLRequest rather than NSURLRequest in APIs #600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 26, 2016

Conversation

seabaylea
Copy link
Contributor

A number of the URLSession APIs should now take URLRequest rather than NSURLRequest or NSMutableURLRequest as parameters. Examples of this include:

open func dataTask(with request: URLRequest) -> URLSessionDataTask
open func uploadTask(with request: URLRequest, from bodyData: Data) -> URLSessionUploadTask
open func downloadTask(with request: URLRequest) -> URLSessionDownloadTask

This PR switches over to using URLRequest rather than the existing use of NSURLRequest.

I've additionally added tests for URLRequest in addition to NSURLRequest (which still exists) by duplicating the existing NSURLRequest tests.

Whilst doing this I hit a crash when trying to mutate a value in a copied URLRequest. This is caused because, when we call _appyMutation for a non-uniquely referenced value, we create a new _MutableHandle. The _MutableHandle constructor calls copy() on the referent (in our case a NSMutableURLSession). Calling NSMutableURLSession.copy() actually returns a NSURLSession (as it inherits copy() from NSURLSession), which fails because its not a MutableType. I've fixed this by overriding copy in NSMutableURLSession to return mutableCopy().

@parkera
Copy link
Contributor

parkera commented Aug 26, 2016

@swift-ci please test linux

@parkera parkera merged commit cffa65a into swiftlang:master Aug 26, 2016
@phausler
Copy link
Contributor

we should consider adding these structural tests into the overlay unit tests as well

@seabaylea seabaylea deleted the urlrequest branch September 1, 2016 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants