Skip to content

Commit 76a2d3b

Browse files
tomerdtomerd
authored
better diagnostics for networking errors (#6330)
motivation: improved user experience change: override the default description of nserror and produce a more user friendly diagnostics
1 parent d52001b commit 76a2d3b

File tree

5 files changed

+71
-53
lines changed

5 files changed

+71
-53
lines changed

‎Sources/Basics/Errors.swift

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@
1010
//
1111
//===----------------------------------------------------------------------===//
1212

13+
import protocol Foundation.LocalizedError
14+
import class Foundation.NSError
15+
import var Foundation.NSLocalizedDescriptionKey
1316
import struct TSCBasic.StringError
1417

1518
public typealias StringError = TSCBasic.StringError
@@ -18,6 +21,33 @@ public struct InternalError: Error {
1821
private let description: String
1922
public init(_ description: String) {
2023
assertionFailure(description)
21-
self.description = "Internal error. Please file a bug at https://github.com/apple/swift-package-manager/issues with this info. \(description)"
24+
self
25+
.description =
26+
"Internal error. Please file a bug at https://github.com/apple/swift-package-manager/issues with this info. \(description)"
27+
}
28+
}
29+
30+
extension Error {
31+
public var interpolationDescription: String {
32+
switch self {
33+
case let _error as LocalizedError:
34+
var description = _error.localizedDescription
35+
if let recoverySuggestion = _error.recoverySuggestion {
36+
description += ". \(recoverySuggestion)"
37+
}
38+
return description
39+
case let _error as NSError:
40+
guard var description = _error.userInfo[NSLocalizedDescriptionKey] as? String else {
41+
return "\(self)"
42+
}
43+
44+
if let localizedRecoverySuggestion = _error.localizedRecoverySuggestion {
45+
description += ". \(localizedRecoverySuggestion)"
46+
}
47+
return description
48+
49+
default:
50+
return "\(self)"
51+
}
2252
}
2353
}

‎Sources/Basics/HTTPClient/URLSessionHTTPClient.swift

Lines changed: 27 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -117,11 +117,12 @@ private class DataTaskManager: NSObject, URLSessionDataDelegate {
117117
return task
118118
}
119119

120-
public func urlSession(_ session: URLSession,
121-
dataTask: URLSessionDataTask,
122-
didReceive response: URLResponse,
123-
completionHandler: @escaping (URLSession.ResponseDisposition) -> Void)
124-
{
120+
public func urlSession(
121+
_ session: URLSession,
122+
dataTask: URLSessionDataTask,
123+
didReceive response: URLResponse,
124+
completionHandler: @escaping (URLSession.ResponseDisposition) -> Void
125+
) {
125126
guard let task = self.tasks[dataTask.taskIdentifier] else {
126127
return completionHandler(.cancel)
127128
}
@@ -166,12 +167,13 @@ private class DataTaskManager: NSObject, URLSessionDataDelegate {
166167
}
167168
}
168169

169-
public func urlSession(_ session: URLSession,
170-
task: URLSessionTask,
171-
willPerformHTTPRedirection response: HTTPURLResponse,
172-
newRequest request: URLRequest,
173-
completionHandler: @escaping (URLRequest?) -> Void)
174-
{
170+
public func urlSession(
171+
_ session: URLSession,
172+
task: URLSessionTask,
173+
willPerformHTTPRedirection response: HTTPURLResponse,
174+
newRequest request: URLRequest,
175+
completionHandler: @escaping (URLRequest?) -> Void
176+
) {
175177
// Don't remove task from dictionary because we want to resume it later
176178
guard let task = self.tasks[task.taskIdentifier] else {
177179
return
@@ -198,10 +200,11 @@ private class DataTaskManager: NSObject, URLSessionDataDelegate {
198200
var expectedContentLength: Int64?
199201
var buffer: Data?
200202

201-
init(task: URLSessionDataTask,
202-
progressHandler: LegacyHTTPClient.ProgressHandler?,
203-
completionHandler: @escaping LegacyHTTPClient.CompletionHandler,
204-
authorizationProvider: LegacyHTTPClientConfiguration.AuthorizationProvider?
203+
init(
204+
task: URLSessionDataTask,
205+
progressHandler: LegacyHTTPClient.ProgressHandler?,
206+
completionHandler: @escaping LegacyHTTPClient.CompletionHandler,
207+
authorizationProvider: LegacyHTTPClientConfiguration.AuthorizationProvider?
205208
) {
206209
self.task = task
207210
self.progressHandler = progressHandler
@@ -295,7 +298,7 @@ private class DownloadTaskManager: NSObject, URLSessionDownloadDelegate {
295298

296299
do {
297300
if let error {
298-
throw HTTPClientError.downloadError("\(error)")
301+
throw HTTPClientError.downloadError(error.interpolationDescription)
299302
} else if let error = task.moveFileError {
300303
throw error
301304
} else if let response = downloadTask.response as? HTTPURLResponse {
@@ -359,14 +362,16 @@ extension URLRequest {
359362
}
360363
}
361364

362-
private extension HTTPURLResponse {
363-
func response(body: Data?) -> HTTPClientResponse {
365+
extension HTTPURLResponse {
366+
fileprivate func response(body: Data?) -> HTTPClientResponse {
364367
let headers = HTTPClientHeaders(self.allHeaderFields.map { header in
365368
.init(name: "\(header.key)", value: "\(header.value)")
366369
})
367-
return HTTPClientResponse(statusCode: self.statusCode,
368-
statusText: Self.localizedString(forStatusCode: self.statusCode),
369-
headers: headers,
370-
body: body)
370+
return HTTPClientResponse(
371+
statusCode: self.statusCode,
372+
statusText: Self.localizedString(forStatusCode: self.statusCode),
373+
headers: headers,
374+
body: body
375+
)
371376
}
372377
}

‎Sources/PackageRegistry/RegistryClient.swift

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1659,23 +1659,23 @@ public enum RegistryError: Error, CustomStringConvertible {
16591659
case .pathAlreadyExists(let path):
16601660
return "path already exists '\(path)'"
16611661
case .failedRetrievingReleases(let registry, let packageIdentity, let error):
1662-
return "failed fetching \(packageIdentity) releases list from \(registry): \(error)"
1662+
return "failed fetching \(packageIdentity) releases list from \(registry): \(error.interpolationDescription)"
16631663
case .failedRetrievingReleaseInfo(let registry, let packageIdentity, let version, let error):
1664-
return "failed fetching \(packageIdentity) version \(version) release information from \(registry): \(error)"
1664+
return "failed fetching \(packageIdentity) version \(version) release information from \(registry): \(error.interpolationDescription)"
16651665
case .failedRetrievingReleaseChecksum(let registry, let packageIdentity, let version, let error):
1666-
return "failed fetching \(packageIdentity) version \(version) release checksum from \(registry): \(error)"
1666+
return "failed fetching \(packageIdentity) version \(version) release checksum from \(registry): \(error.interpolationDescription)"
16671667
case .failedRetrievingManifest(let registry, let packageIdentity, let version, let error):
1668-
return "failed retrieving \(packageIdentity) version \(version) manifest from \(registry): \(error)"
1668+
return "failed retrieving \(packageIdentity) version \(version) manifest from \(registry): \(error.interpolationDescription)"
16691669
case .failedDownloadingSourceArchive(let registry, let packageIdentity, let version, let error):
1670-
return "failed downloading \(packageIdentity) version \(version) source archive from \(registry): \(error)"
1670+
return "failed downloading \(packageIdentity) version \(version) source archive from \(registry): \(error.interpolationDescription)"
16711671
case .failedIdentityLookup(let registry, let scmURL, let error):
1672-
return "failed looking up identity for \(scmURL) on \(registry): \(error)"
1672+
return "failed looking up identity for \(scmURL) on \(registry): \(error.interpolationDescription)"
16731673
case .failedLoadingPackageArchive(let path):
16741674
return "failed loading package archive at '\(path)' for publishing"
16751675
case .failedLoadingPackageMetadata(let path):
16761676
return "failed loading package metadata at '\(path)' for publishing"
16771677
case .failedPublishing(let error):
1678-
return "failed publishing: \(error)"
1678+
return "failed publishing: \(error.interpolationDescription)"
16791679
case .missingPublishingLocation:
16801680
return "response missing registry source archive"
16811681
case .serverError(let code, let details):
@@ -1699,7 +1699,7 @@ public enum RegistryError: Error, CustomStringConvertible {
16991699
case .failedLoadingSignature:
17001700
return "failed loading signature for validation"
17011701
case .failedRetrievingSourceArchiveSignature(let registry, let packageIdentity, let version, let error):
1702-
return "failed retrieving '\(packageIdentity)' version \(version) source archive signature from '\(registry)': \(error)"
1702+
return "failed retrieving '\(packageIdentity)' version \(version) source archive signature from '\(registry)': \(error.interpolationDescription)"
17031703
case .manifestNotSigned(let registry, let packageIdentity, let version, let toolsVersion):
17041704
return "manifest for \(packageIdentity) version \(version) tools version \(toolsVersion.map { "\($0)" } ?? "unspecified") from \(registry) is not signed"
17051705
case .missingConfiguration(let details):
@@ -1717,7 +1717,7 @@ public enum RegistryError: Error, CustomStringConvertible {
17171717
case .signerNotTrusted(_, let signingEntity):
17181718
return "the signer \(signingEntity) is not trusted"
17191719
case .failedToValidateSignature(let error):
1720-
return "failed to validate signature: \(error)"
1720+
return "failed to validate signature: \(error.interpolationDescription)"
17211721
case .signingEntityForReleaseChanged(let registry, let package, let version, let latest, let previous):
17221722
return "the signing entity '\(String(describing: latest))' from \(registry) for \(package) version \(version) is different from the previously recorded value '\(previous)'"
17231723
case .signingEntityForPackageChanged(

‎Sources/Workspace/Workspace+BinaryArtifacts.swift

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ extension Workspace {
166166
}
167167
} catch {
168168
errors.append(error)
169-
observabilityScope.emit(error: "failed retrieving '\(indexFile.url)': \(error)")
169+
observabilityScope.emit(error: "failed retrieving '\(indexFile.url)': \(error.interpolationDescription)")
170170
}
171171
}
172172
}
@@ -297,23 +297,20 @@ extension Workspace {
297297
)
298298
self.delegate?.didDownloadBinaryArtifact(from: artifact.url.absoluteString, result: .success(artifactPath), duration: downloadStart.distance(to: .now()))
299299
case .failure(let error):
300-
let reason = (error as? LocalizedError)?.errorDescription ?? "\(error)"
301-
observabilityScope.emit(.remoteArtifactFailedExtraction(artifactURL: artifact.url, targetName: artifact.targetName, reason: reason))
300+
observabilityScope.emit(.remoteArtifactFailedExtraction(artifactURL: artifact.url, targetName: artifact.targetName, reason: error.interpolationDescription))
302301
self.delegate?.didDownloadBinaryArtifact(from: artifact.url.absoluteString, result: .failure(error), duration: downloadStart.distance(to: .now()))
303302
}
304303

305304
observabilityScope.trap { try self.fileSystem.removeFileTree(archivePath) }
306305
})
307306
case .failure(let error):
308-
let reason = (error as? LocalizedError)?.errorDescription ?? "\(error)"
309-
observabilityScope.emit(.artifactFailedValidation(artifactURL: artifact.url, targetName: artifact.targetName, reason: "\(reason)"))
307+
observabilityScope.emit(.artifactFailedValidation(artifactURL: artifact.url, targetName: artifact.targetName, reason: error.interpolationDescription))
310308
self.delegate?.didDownloadBinaryArtifact(from: artifact.url.absoluteString, result: .failure(error), duration: downloadStart.distance(to: .now()))
311309
}
312310
})
313311
case .failure(let error):
314-
let reason = (error as? LocalizedError)?.errorDescription ?? "\(error)"
315312
observabilityScope.trap ({ try self.fileSystem.removeFileTree(archivePath) })
316-
observabilityScope.emit(.artifactFailedDownload(artifactURL: artifact.url, targetName: artifact.targetName, reason: "\(reason)"))
313+
observabilityScope.emit(.artifactFailedDownload(artifactURL: artifact.url, targetName: artifact.targetName, reason: error.interpolationDescription))
317314
self.delegate?.didDownloadBinaryArtifact(from: artifact.url.absoluteString, result: .failure(error), duration: downloadStart.distance(to: .now()))
318315
}
319316
})

‎Tests/BasicsTests/URLSessionHTTPClientTests.swift

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -452,14 +452,7 @@ final class URLSessionHTTPClientTest: XCTestCase {
452452
case .success:
453453
XCTFail("unexpected success")
454454
case .failure(let error):
455-
#if os(macOS)
456-
// FIXME: URLSession loses the full error description when going
457-
// from Swift.Error to NSError which is then received in
458-
// urlSession(_ session: URLSession, task downloadTask: URLSessionTask, didCompleteWithError error: Error?)
459-
XCTAssertNotNil(error as? HTTPClientError)
460-
#else
461455
XCTAssertEqual(error as? HTTPClientError, HTTPClientError.downloadError(clientError.description))
462-
#endif
463456
}
464457
completionExpectation.fulfill()
465458
}
@@ -909,14 +902,7 @@ final class URLSessionHTTPClientTest: XCTestCase {
909902
)
910903
XCTFail("unexpected success")
911904
} catch {
912-
#if os(macOS)
913-
// FIXME: URLSession loses the full error description when going
914-
// from Swift.Error to NSError which is then received in
915-
// urlSession(_ session: URLSession, task downloadTask: URLSessionTask, didCompleteWithError error: Error?)
916-
XCTAssertNotNil(error as? HTTPClientError)
917-
#else
918905
XCTAssertEqual(error as? HTTPClientError, HTTPClientError.downloadError(clientError.description))
919-
#endif
920906
}
921907
}
922908
}

0 commit comments

Comments
 (0)