Skip to content

Implementation of URLSessionTask '.error', NSCopying #713

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Dec 15, 2016
Merged

Implementation of URLSessionTask '.error', NSCopying #713

merged 9 commits into from
Dec 15, 2016

Conversation

naithar
Copy link
Contributor

@naithar naithar commented Nov 11, 2016 •
edited
Loading

Added an implementation for .error property and NSCopying for URLSessionTask
Added tests.

.cancel() is implemented in: #689

@naithar naithar changed the title Implementation URLSessionTask '.error', NSCopying Implementation of URLSessionTask '.error', NSCopying Nov 11, 2016
@naithar
Copy link
Contributor Author

naithar commented Nov 22, 2016

@pushkarnk can you review, please?

pushkarnk
pushkarnk reviewed Nov 22, 2016
View reviewed changes
Foundation/NSURLSession/NSURLSessionTask.swift
@@ -221,7 +221,7 @@ open class URLSessionTask : NSObject, NSCopying {
* The error, if any, delivered via -URLSession:task:didCompleteWithError:
* This property will be nil in the event that no error occured.
*/
/*@NSCopying*/ open var error: NSError? { NSUnimplemented() }
/*@NSCopying*/ open fileprivate(set) var error: NSError?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that we can keep this fileprivate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows to set error without creating a backing private property.
The other option is to create fileprivate var _error: Error? and make error a get only computed property.

@naithar
Removed 'fileprivate(set)'. Added backing private property. Switched …
50428d8 
…NSError with Error to conform to macOS Foundation
@naithar
Copy link
Contributor Author

naithar commented Nov 22, 2016

@pushkarnk updated. Also switched NSError with Error as in MacOS Foundation.

@pushkarnk
Copy link
Member

Thank you @naithar ! Looks good to me.

@pushkarnk
Copy link
Member

@swift-ci please test and merge

@pushkarnk
Copy link
Member

@swift-ci please test

1 similar comment
@pushkarnk
Copy link
Member

@swift-ci please test

@pushkarnk
Copy link
Member

There was a segfault:

Test Case 'TestURLSession.test_taskError' started at 01:26:18.558
/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift/utils/build-script-impl: line 252:  1201 Segmentation fault      "$@"
/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift/utils/build-script: fatal error: command terminated with a non-zero exit status 139, aborting
Build step 'Execute shell' marked build as failure

@pushkarnk
Copy link
Member

We see the same segfault in #719 again in the newly added test case. 🤔

@pushkarnk pushkarnk mentioned this pull request Nov 23, 2016
Merged
@naithar
Copy link
Contributor Author

naithar commented Nov 23, 2016

Tested on local environment, no segmentation fault atm. #712 Also failed, though for the other reason.
Could the difference in testing environments cause this error?

@pushkarnk
Copy link
Member

Yeah, may be @parkera can help us here!

@parkera
Copy link
Contributor

parkera commented Nov 24, 2016

A change elsewhere in the stack maybe? I'll try another test and we'll go from there...

@parkera
Copy link
Contributor

parkera commented Nov 24, 2016

@swift-ci please test

@pushkarnk
Copy link
Member

same failure again

@pushkarnk
Copy link
Member

@swift-ci please test

@pushkarnk
Copy link
Member

clang-4.0: error: unable to execute command: Bus error

Trying again.

@pushkarnk
Copy link
Member

@swift-ci please test

@naithar
Copy link
Contributor Author

naithar commented Dec 9, 2016

Any updates?

@pushkarnk
Copy link
Member

@swift-ci please test

@pushkarnk
Copy link
Member

@naithar May I ask if your local environment is based on Ubuntu 16.04?

@naithar
Copy link
Contributor Author

naithar commented Dec 12, 2016

@pushkarnk no, i used 15.10 last time for tests. I'll try running on 16.04 tomorrow as it's already used for some internal swift projects.

@naithar
Copy link
Contributor Author

naithar commented Dec 14, 2016

This PR does indeed crash on Ubuntu 16.04. I'll try to work this out.

@naithar
Copy link
Contributor Author

naithar commented Dec 14, 2016 •
edited
Loading

Currently i would blame the force unwrapping task.error as! NSError, but workaround with switch didn't work. I'll continue looking

Test crashes on accessing NSURLSessionTask's .error value.

@naithar
Copy link
Contributor Author

naithar commented Dec 14, 2016 •
edited
Loading

@pushkarnk with this changes tests should be running correctly.
I'll replace NSError with Swift.Error type in the next PR.

I'll also look into #719 to see if it's somehow connected with my error.

@parkera
Copy link
Contributor

parkera commented Dec 14, 2016

@swift-ci please test

1 similar comment
@pushkarnk
Copy link
Member

@swift-ci please test

@pushkarnk
Copy link
Member

pushkarnk commented Dec 15, 2016 •
edited
Loading

Thanks @naithar !

@pushkarnk pushkarnk merged commit afd53fc into swiftlang:master Dec 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pushkarnk pushkarnk pushkarnk left review comments

Assignees

@pushkarnk pushkarnk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@naithar @pushkarnk @parkera