NSLocalizedDescription for http errors taken from libcurl #1198

johnno1962 · 2017-09-02T21:23:14Z

Hi Apple,

At the moment any network error using a URLSession contains only the default "The operation could not be completed” as its NSLocalizedDescription. This PR conveys the value of curl_easy_strerror through the implementation so something more specific is reported such as "Foundation.URLError(_nsError: Peer certificate cannot be authenticated with given CA certificates)"

Thanks.

ianpartridge · 2017-09-05T07:57:54Z

Foundation/URLSession/http/EasyHandle.swift

@@ -170,6 +171,9 @@ extension _EasyHandle {
        let protocols = (CFURLSessionProtocolHTTP | CFURLSessionProtocolHTTPS)
        try! CFURLSession_easy_setopt_long(rawHandle, CFURLSessionOptionPROTOCOLS, protocols).asError()
        try! CFURLSession_easy_setopt_long(rawHandle, CFURLSessionOptionREDIR_PROTOCOLS, protocols).asError()
+        errorBuffer.withUnsafeMutableBufferPointer {


setAllowedProtocolsToHTTPAndHTTPS() doesn't seem to be an accurate name for this function any more.

I’m just using that function for some other initialisation as a convenience rather than create a new one. I think the name if probably good as it is as it relates to it’s primary purpose.

I could move this extra code into another of the setup functions.

What do you think @pushkarnk ?

I see there is a function set(errorBuffer:) which is called from _HTTPURLProtocol.configureEasyHandle(forRequest:) I could set the buffer there but it wouldn’t guarantee the buffer still exists when you call curl_easy_strerror() on the handle.

I’ve pushed a new revision introducing a default error buffer so the messages are always as descriptive as possible. See what you think.

ianpartridge · 2017-09-05T07:59:05Z

Foundation/URLSession/http/HTTPURLProtocol.swift

@@ -575,6 +576,10 @@ extension _HTTPURLProtocol: _EasyHandleDelegate {
    }
 }

+internal struct URLErrorInfo {
+    let code: Int, description: String?


Put these on separate lines.

Fair enough ;) Just being lazy.

ianpartridge · 2017-09-05T08:00:25Z

Foundation/URLSession/http/HTTPURLProtocol.swift

@@ -330,12 +330,13 @@ internal extension _HTTPURLProtocol {
        case stream(InputStream)
    }

-    func failWith(errorCode: Int, request: URLRequest) {
+    func failWith(errorCode: Int, errorDescription: String?, request: URLRequest) {


Would it be better if this function took a URLErrorInfo instead?

This doesn’t realy pan out as you would be having to create a URLErrrorInfo at the last minute in two out of three of the places where this function is called. Better to leave it as an extra argument. URLErrorInfo is mostly to convey the info through the various callbacks in place of just the code.

parkera · 2017-09-05T17:06:20Z

CoreFoundation/URL.subproj/CFURLSessionInterface.c

@@ -31,6 +31,10 @@ static CFURLSessionMultiCode MakeMultiCode(CURLMcode value) {
    return (CFURLSessionMultiCode) { value };
 }

+const char *CFURLSessionErrorDescription(int value) {
+    return curl_easy_strerror(value);
+}


What's the memory management here? Does the result need to be freed?

buffer is either static string or buffer set/owned by EasyHandle.swift. It doesn’t need to be freed.

parkera · 2017-09-05T17:07:46Z

Foundation/URLSession/http/MultiHandle.swift

-    func completedTransfer(forEasyHandle handle: _EasyHandle, errorCode: Int?) {
-        handle.completedTransfer(withErrorCode: errorCode)
+    func completedTransfer(forEasyHandle handle: _EasyHandle, errorInfo: URLErrorInfo?) {
+        handle.completedTransfer(withErrorInfo: errorInfo)
    }


This should all use Foundation's existing abstraction for errors: NSError, instead of a custom struct.

We can’t construct an NSError before we pass it around as the code doesn’t initially know the URL that is needed for the userInfo.

That can be set later by creating another NSError, or using the concept of underlying error.

It's important that we don't create several different kinds of abstractions for the same concept in one source base.

parkera · 2017-09-05T17:08:31Z

Foundation/URLSession/http/HTTPURLProtocol.swift

@@ -557,7 +558,7 @@ extension _HTTPURLProtocol: _EasyHandleDelegate {
            completeTask()
        case .failWithError(let errorCode):
            internalState = .transferFailed
-            failWith(errorCode: errorCode, request: request)
+            failWith(errorCode: errorCode, errorDescription: "Completion failure", request: request)


We're in a somewhat unfortunate place here, because localization doesn't quite work yet, but we still should find a way to make this use the usual localized string lookup API (on bundle) instead of hard coding English strings.

I’ve created a placeholder for a future NSLocalizedString(_, _) implementation and used it.

parkera · 2017-09-05T17:10:34Z

CoreFoundation/URL.subproj/CFURLSessionInterface.h

@@ -48,6 +48,10 @@ typedef struct CFURLSessionEasyCode {
    int value;
 } CFURLSessionEasyCode;

+CF_EXPORT const char * _Nonnull CFURLSessionErrorDescription(int value);


CF API really should use a CFStringRef as a result instead of C strings. That resolves the memory management question of the result for callers.

Changed to CFString. Unsure of the allocator I should be using if you want to check it.

CF (almost) always uses kCFAllocatorSystemDefault internally.

parkera · 2017-09-05T19:38:13Z

CoreFoundation/URL.subproj/CFURLSessionInterface.c

@@ -19,6 +19,7 @@
 //===----------------------------------------------------------------------===//

 #include "CFURLSessionInterface.h"
+#include "CFString.h"


#include <CoreFoundation/CFString.h>

parkera · 2017-09-05T19:38:37Z

CoreFoundation/URL.subproj/CFURLSessionInterface.c

@@ -31,6 +32,10 @@ static CFURLSessionMultiCode MakeMultiCode(CURLMcode value) {
    return (CFURLSessionMultiCode) { value };
 }

+CFStringRef CFURLSessionErrorDescription(int value) {


CFURLSessionCreateErrorDescription

New version with this change and your other suggestions.

parkera · 2017-09-05T19:39:58Z

CoreFoundation/URL.subproj/CFURLSessionInterface.h

@@ -48,6 +48,10 @@ typedef struct CFURLSessionEasyCode {
    int value;
 } CFURLSessionEasyCode;

+CF_EXPORT CFStringRef _Nonnull CFURLSessionErrorDescription(int value);
+
+CF_EXPORT int const CFURLSessionEasyErrorSize;


This feels like an abstraction leak, but I'm not sure what to do about it exactly.

yeah, the buffer thing is a bit untidy.

parkera · 2017-09-05T19:41:55Z

Foundation/URLSession/http/MultiHandle.swift

-    func completedTransfer(forEasyHandle handle: _EasyHandle, errorCode: Int?) {
-        handle.completedTransfer(withErrorCode: errorCode)
+    func completedTransfer(forEasyHandle handle: _EasyHandle, errorInfo: URLErrorInfo?) {
+        handle.completedTransfer(withErrorInfo: errorInfo)
    }


That can be set later by creating another NSError, or using the concept of underlying error.

It's important that we don't create several different kinds of abstractions for the same concept in one source base.

johnno1962 · 2017-09-06T14:56:14Z

I was using the errorBuffer incorrectly. In the event of an error it can be written to with a more specific text which you have to check for or fall back to curl_easy_strerror() as shown in the example further down https://curl.haxx.se/libcurl/c/CURLOPT_ERRORBUFFER.html. I’m now seeing more complete error messages like: Optional(Foundation.URLError(_nsError: Couldn't resolve host 'wsssww.bbc.co.uk'))

johnno1962 · 2017-09-07T19:11:10Z

This has stabilized and I’ve squashed it unless you have any further comments.

xwu · 2017-09-07T19:35:38Z

Foundation/URLSession/http/HTTPURLProtocol.swift

-        let error = URLError(_nsError: NSError(domain: NSURLErrorDomain, code: errorCode, userInfo: userInfo))
-        completeTask(withError: error)
-        self.client?.urlProtocol(self, didFailWithError: error)
+        let urlErrror = URLError(_nsError: NSError(domain: NSURLErrorDomain, code: error.code, userInfo: userInfo))


xwu · 2017-09-07T19:41:19Z

Foundation/NSString.swift

@@ -20,6 +20,11 @@ extension unichar : ExpressibleByUnicodeScalarLiteral {
    }
 }

+// Placeholder for a future implementation
+public func NSLocalizedString(_ key: String, _ comment: String?) -> String {


This is, I believe, not the correct signature for NSLocalizedString in Swift. For one, “comment” should be an argument label (i.e., comment: and not _ comment:), but there are also other parameters with default values that are not reflected here. I understand it is a placeholder, but the signature should match Darwin Foundation.

Also, if we're going to add it we may as well put in the placeholder as it's done in Darwin:

public func NSLocalizedString(_ key: String, tableName: String? = nil, bundle: Bundle = Bundle.main, value: String = "", comment: String) -> String { return bundle.localizedString(forKey: key, value:value, table:tableName) }

Done. Seems to work

xwu · 2017-09-07T19:43:16Z

CoreFoundation/URL.subproj/CFURLSessionInterface.c

@@ -135,6 +141,7 @@ CFURLSessionEasyCode CFURLSessionInit(void) {
    return MakeEasyCode(curl_global_init(CURL_GLOBAL_SSL));
 }

+int const CFURLSessionEasyErrorSize = { CURL_ERROR_SIZE+1 };


Nit: spaces around +

parkera · 2017-09-07T19:51:17Z

Foundation/URLSession/http/HTTPURLProtocol.swift

@@ -557,7 +561,9 @@ extension _HTTPURLProtocol: _EasyHandleDelegate {
            completeTask()
        case .failWithError(let errorCode):
            internalState = .transferFailed
-            failWith(errorCode: errorCode, request: request)
+            let error = NSError(domain: NSURLErrorDomain, code: errorCode,
+                                userInfo: [NSLocalizedDescriptionKey: "Completion failure"])


We should be calling that new NSLocalizedString function here (once its signature is fixed).

Signature fixed. NSLocalizedString() is called indirectly in the creation of the final URLError struct in failWith(error:….

pushkarnk · 2017-09-08T09:49:53Z

fyi @saiHemak

johnno1962 · 2017-09-14T16:19:15Z

Hi @parkera, anything else you need from me to merge this? I can then rebase and we could focus on landing #1113 and I’ll be out of your hair for another year! The Android toolchain is essentially complete.

alblue · 2017-09-15T11:10:02Z

@swift-ci please test

pushkarnk · 2017-09-18T12:13:40Z

@johnno1962 Sorry for the delay in reviewing. I went through the review comments and the changes, and I think we should go ahead and merge this.

Does this look good to you @parkera ?

pushkarnk · 2017-09-18T12:14:38Z

@swift-ci please test

pushkarnk · 2017-09-18T12:30:12Z

@johnno1962 That the Android toolchain is complete is fantastic news indeed! Thank you :)

alblue · 2017-09-18T15:25:46Z

@parkera has the above met your requirements now? If so, can you complete the review?

ianpartridge · 2017-09-19T10:25:25Z

Foundation/NSString.swift

+                       bundle: Bundle = Bundle.main,
+                       value: String = "",
+                       comment: String) -> String {
+    return bundle.localizedString(forKey: key, value:value, table:tableName)


Nit: spaces after colons.

Thanks Ian. Amended.

ianpartridge · 2017-09-19T10:53:11Z

@swift-ci please test

alblue · 2017-10-05T14:12:13Z

@parkera can you revisit this please; in particular the signature changes in d809b59 which may or may not be desirable?

johnno1962 · 2017-10-05T14:30:08Z

Signature changes were to bring it into line with the documented Darwin signature. It was me that made it optional.

alblue · 2017-10-13T11:59:05Z

Can we split this into two PRs?

The first can add the NSLocalizedDescription with the right syntax. I think that's the main one that @parkera wanted to check. Once we've go that agreed and merged, we can rebase the remainder of this change onto that one. Sound good?

johnno1962 · 2017-10-13T12:14:00Z

OK, I’ve created #1263 but my main concern as it has always been is to have this PR merged before #1216 as it will break in way that I don’t have the time to fix/test. Hopefully we can agree the signature of NSLocalizedDescription quickly!

johnno1962 · 2017-10-13T13:16:18Z

Right, rebased and merged. You know I’ll never never get that hour of my life back 😀!

alblue · 2017-10-13T15:01:10Z

OK, I've gone through all of @parkera @xwu and @ianpartridge comments and I'm happy that they've been addressed. I'm going to kick off a test run, and provided that passes and there's no objections from @parkera et al will do a test'n'merge later on tonight.

alblue · 2017-10-13T15:01:53Z

@swift-ci please test

Changes have been made as requested

alblue · 2017-10-13T19:27:59Z

@swift-ci please test and merge

johnno1962 · 2017-10-13T20:30:04Z

johnno1962 force-pushed the NSLocalizedDescription branch from dd12300 to f9b2c18 Compare September 3, 2017 01:00

ianpartridge requested a review from pushkarnk September 4, 2017 09:45

ianpartridge reviewed Sep 5, 2017

View reviewed changes

parkera requested changes Sep 5, 2017

View reviewed changes

parkera reviewed Sep 5, 2017

View reviewed changes

johnno1962 force-pushed the NSLocalizedDescription branch from 1214e1d to 6318490 Compare September 7, 2017 08:54

xwu reviewed Sep 7, 2017

View reviewed changes

parkera previously requested changes Sep 7, 2017

View reviewed changes

johnno1962 force-pushed the NSLocalizedDescription branch 2 times, most recently from 56bc8e9 to 79fa1d3 Compare September 7, 2017 20:05

johnno1962 mentioned this pull request Sep 14, 2017

Thread safety for URLSession #1210

Merged

pushkarnk approved these changes Sep 18, 2017

View reviewed changes

ianpartridge reviewed Sep 19, 2017

View reviewed changes

johnno1962 force-pushed the NSLocalizedDescription branch from 5cf17a7 to d809b59 Compare September 19, 2017 10:28

This was referenced Oct 7, 2017

Refactoring URLSession for multiple native protocols #1216

Closed

For testing Foundation on Android #1189

Merged

johnno1962 force-pushed the NSLocalizedDescription branch 2 times, most recently from 7b7c423 to 21d2b72 Compare October 13, 2017 13:14

NSLocalizedDescription for http errors

eafde74

johnno1962 force-pushed the NSLocalizedDescription branch from 21d2b72 to eafde74 Compare October 13, 2017 14:43

johnno1962 mentioned this pull request Oct 13, 2017

Rewrite of PR #1103 + addition Android specific patches #1113

Closed

swift-ci merged commit a0f8df7 into swiftlang:master Oct 13, 2017

NSLocalizedDescription for http errors taken from libcurl #1198

NSLocalizedDescription for http errors taken from libcurl #1198

Uh oh!

Conversation

johnno1962 commented Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

johnno1962 Sep 5, 2017 • 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!

johnno1962 Sep 5, 2017 • 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!

johnno1962 Sep 5, 2017 • 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!

johnno1962 Sep 5, 2017 • 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!

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!

johnno1962 commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnno1962 commented Sep 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parkera Sep 7, 2017 • 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!

johnno1962 commented Sep 2, 2017 •

edited

Loading

johnno1962 Sep 5, 2017 •

edited

Loading

johnno1962 Sep 5, 2017 •

edited

Loading

johnno1962 Sep 5, 2017 •

edited

Loading

johnno1962 Sep 5, 2017 •

edited

Loading

johnno1962 commented Sep 6, 2017 •

edited

Loading

parkera Sep 7, 2017 •

edited

Loading

johnno1962 commented Sep 14, 2017 •

edited

Loading