Skip to content

improve string interpolation of errors #6482

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

Merged
merged 4 commits into from
Apr 26, 2023
Merged

improve string interpolation of errors #6482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d05f158
use Error::interpolationDescription when performing string interpolat…
tomerd Apr 25, 2023
5cc111f
Update Sources/Basics/Observability.swift
tomerd Apr 26, 2023
708ba37
Update Sources/Basics/Errors.swift
tomerd Apr 26, 2023
4439a68
fixup
tomerd Apr 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 58 additions & 21 deletions Sources/Basics/AuthorizationProvider.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ public protocol AuthorizationProvider {
}

public protocol AuthorizationWriter {
func addOrUpdate(for url: URL, user: String, password: String, persist: Bool, callback: @escaping (Result<Void, Error>) -> Void)
func addOrUpdate(
for url: URL,
user: String,
password: String,
persist: Bool,
callback: @escaping (Result<Void, Error>) -> Void
)

func remove(for url: URL, callback: @escaping (Result<Void, Error>) -> Void)
}
Expand All @@ -36,9 +42,9 @@ public enum AuthorizationProviderError: Error {
case other(String)
}

public extension AuthorizationProvider {
extension AuthorizationProvider {
@Sendable
func httpAuthorizationHeader(for url: URL) -> String? {
public func httpAuthorizationHeader(for url: URL) -> String? {
guard let (user, password) = self.authentication(for: url) else {
return nil
}
Expand All @@ -50,8 +56,8 @@ public extension AuthorizationProvider {
}
}

private extension URL {
var authenticationID: String? {
extension URL {
fileprivate var authenticationID: String? {
guard let host = host?.lowercased() else {
return nil
}
Expand All @@ -75,7 +81,13 @@ public class NetrcAuthorizationProvider: AuthorizationProvider, AuthorizationWri
_ = try Self.load(fileSystem: fileSystem, path: path)
}

public func addOrUpdate(for url: URL, user: String, password: String, persist: Bool = true, callback: @escaping (Result<Void, Error>) -> Void) {
public func addOrUpdate(
for url: URL,
user: String,
password: String,
persist: Bool = true,
callback: @escaping (Result<Void, Error>) -> Void
) {
guard let machine = url.authenticationID else {
return callback(.failure(AuthorizationProviderError.invalidURLHost))
}
Expand All @@ -87,7 +99,9 @@ public class NetrcAuthorizationProvider: AuthorizationProvider, AuthorizationWri

// Same entry already exists, no need to add or update
let netrc = try? Self.load(fileSystem: self.fileSystem, path: self.path)
guard netrc?.machines.first(where: { $0.name.lowercased() == machine && $0.login == user && $0.password == password }) == nil else {
guard netrc?.machines
.first(where: { $0.name.lowercased() == machine && $0.login == user && $0.password == password }) == nil
else {
return callback(.success(()))
}

Expand All @@ -108,12 +122,18 @@ public class NetrcAuthorizationProvider: AuthorizationProvider, AuthorizationWri

callback(.success(()))
} catch {
callback(.failure(AuthorizationProviderError.other("Failed to update netrc file at \(self.path): \(error)")))
callback(.failure(
AuthorizationProviderError
.other("Failed to update netrc file at \(self.path): \(error.interpolationDescription)")
))
}
}

public func remove(for url: URL, callback: @escaping (Result<Void, Error>) -> Void) {
callback(.failure(AuthorizationProviderError.other("User must edit netrc file at \(self.path) manually to remove entries")))
callback(.failure(
AuthorizationProviderError
.other("User must edit netrc file at \(self.path) manually to remove entries")
))
}

public func authentication(for url: URL) -> (user: String, password: String)? {
Expand All @@ -124,7 +144,9 @@ public class NetrcAuthorizationProvider: AuthorizationProvider, AuthorizationWri
}

private func machine(for url: URL) -> Basics.Netrc.Machine? {
if let machine = url.authenticationID, let existing = self.machines.first(where: { $0.name.lowercased() == machine }) {
if let machine = url.authenticationID,
let existing = self.machines.first(where: { $0.name.lowercased() == machine })
{
return existing
}
if let existing = self.machines.first(where: { $0.isDefault }) {
Expand Down Expand Up @@ -164,7 +186,13 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization
self.observabilityScope = observabilityScope
}

public func addOrUpdate(for url: URL, user: String, password: String, persist: Bool = true, callback: @escaping (Result<Void, Error>) -> Void) {
public func addOrUpdate(
for url: URL,
user: String,
password: String,
persist: Bool = true,
callback: @escaping (Result<Void, Error>) -> Void
) {
guard let server = url.authenticationID else {
return callback(.failure(AuthorizationProviderError.invalidURLHost))
}
Expand Down Expand Up @@ -214,12 +242,14 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization
}

do {
guard let existingItem = try self.search(server: server, protocol: self.protocol(for: url)) as? [String: Any],
let passwordData = existingItem[kSecValueData as String] as? Data,
let password = String(data: passwordData, encoding: String.Encoding.utf8),
let account = existingItem[kSecAttrAccount as String] as? String
guard let existingItem = try self
.search(server: server, protocol: self.protocol(for: url)) as? [String: Any],
let passwordData = existingItem[kSecValueData as String] as? Data,
let password = String(data: passwordData, encoding: String.Encoding.utf8),
let account = existingItem[kSecAttrAccount as String] as? String
else {
throw AuthorizationProviderError.other("Failed to extract credentials for server \(server) from keychain")
throw AuthorizationProviderError
.other("Failed to extract credentials for server \(server) from keychain")
}
return (user: account, password: password)
} catch {
Expand All @@ -229,7 +259,10 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization
case AuthorizationProviderError.other(let detail):
self.observabilityScope.emit(error: detail)
default:
self.observabilityScope.emit(error: "Failed to find credentials for server \(server) in keychain: \(error)")
self.observabilityScope.emit(
error: "Failed to find credentials for server \(server) in keychain",
underlyingError: error
)
}
return nil
}
Expand All @@ -244,7 +277,8 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization

let status = SecItemAdd(query as CFDictionary, nil)
guard status == errSecSuccess else {
throw AuthorizationProviderError.other("Failed to save credentials for server \(server) to keychain: status \(status)")
throw AuthorizationProviderError
.other("Failed to save credentials for server \(server) to keychain: status \(status)")
}
}

Expand All @@ -260,7 +294,8 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization
return false
}
guard status == errSecSuccess else {
throw AuthorizationProviderError.other("Failed to update credentials for server \(server) in keychain: status \(status)")
throw AuthorizationProviderError
.other("Failed to update credentials for server \(server) in keychain: status \(status)")
}
return true
}
Expand All @@ -271,7 +306,8 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization
kSecAttrProtocol as String: `protocol`]
let status = SecItemDelete(query as CFDictionary)
guard status == errSecSuccess else {
throw AuthorizationProviderError.other("Failed to delete credentials for server \(server) from keychain: status \(status)")
throw AuthorizationProviderError
.other("Failed to delete credentials for server \(server) from keychain: status \(status)")
}
}

Expand All @@ -290,7 +326,8 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization
throw AuthorizationProviderError.notFound
}
guard status == errSecSuccess else {
throw AuthorizationProviderError.other("Failed to find credentials for server \(server) in keychain: status \(status)")
throw AuthorizationProviderError
.other("Failed to find credentials for server \(server) in keychain: status \(status)")
}

return item
Expand Down
58 changes: 35 additions & 23 deletions Sources/Basics/Cancellator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ public final class Cancellator: Cancellable {

private let observabilityScope: ObservabilityScope?
private let registry = ThreadSafeKeyValueStore<String, (name: String, handler: CancellationHandler)>()
private let cancelationQueue = DispatchQueue(label: "org.swift.swiftpm.cancellator", qos: .userInteractive, attributes: .concurrent)
private let cancelationQueue = DispatchQueue(
label: "org.swift.swiftpm.cancellator",
qos: .userInteractive,
attributes: .concurrent
)
private let cancelling = ThreadSafeBox<Bool>(false)

private static let signalHandlerLock = NSLock()
Expand All @@ -44,60 +48,60 @@ public final class Cancellator: Cancellable {
public func installSignalHandlers() {
Self.signalHandlerLock.withLock {
precondition(!Self.isSignalHandlerInstalled)
#if os(Windows)

#if os(Windows)
// Closures passed to `SetConsoleCtrlHandler` can't capture context, working around that with a global.
Self.shared = self

// set shutdown handler to terminate sub-processes, etc
_ = SetConsoleCtrlHandler({ _ in
// Terminate all processes on receiving an interrupt signal.
try? Cancellator.shared?.cancel(deadline: .now() + .seconds(30))

// Reset the handler.
_ = SetConsoleCtrlHandler(nil, false)

// Exit as if by signal()
TerminateProcess(GetCurrentProcess(), 3)

return true
}, true)
#else
#else
// trap SIGINT to terminate sub-processes, etc
signal(SIGINT, SIG_IGN)
let interruptSignalSource = DispatchSource.makeSignalSource(signal: SIGINT)
interruptSignalSource.setEventHandler { [weak self] in
// cancel the trap?
interruptSignalSource.cancel()

// Terminate all processes on receiving an interrupt signal.
try? self?.cancel(deadline: .now() + .seconds(30))
#if canImport(Darwin) || os(OpenBSD)

#if canImport(Darwin) || os(OpenBSD)
// Install the default signal handler.
var action = sigaction()
action.__sigaction_u.__sa_handler = SIG_DFL
sigaction(SIGINT, &action, nil)
kill(getpid(), SIGINT)
#elseif os(Android)
#elseif os(Android)
// Install the default signal handler.
var action = sigaction()
action.sa_handler = SIG_DFL
sigaction(SIGINT, &action, nil)
kill(getpid(), SIGINT)
#else
#else
var action = sigaction()
action.__sigaction_handler = unsafeBitCast(
SIG_DFL,
to: sigaction.__Unnamed_union___sigaction_handler.self
)
sigaction(SIGINT, &action, nil)
kill(getpid(), SIGINT)
#endif
#endif
}
interruptSignalSource.resume()
#endif
#endif

Self.isSignalHandlerInstalled = true
}
}
Expand Down Expand Up @@ -138,20 +142,21 @@ public final class Cancellator: Cancellable {
self.registry[key] = nil
}

public func cancel(deadline: DispatchTime) throws -> Void {
public func cancel(deadline: DispatchTime) throws {
self._cancel(deadline: deadline)
}

// marked internal for testing
@discardableResult
internal func _cancel(deadline: DispatchTime? = .none)-> Int {
internal func _cancel(deadline: DispatchTime? = .none) -> Int {
self.cancelling.put(true)

self.observabilityScope?.emit(info: "starting cancellation cycle with \(self.registry.count) cancellation handlers registered")
self.observabilityScope?
.emit(info: "starting cancellation cycle with \(self.registry.count) cancellation handlers registered")

let deadline = deadline ?? .now() + .seconds(30)
// deadline for individual handlers set slightly before overall deadline
let delta: DispatchTimeInterval = .nanoseconds(abs(deadline.distance(to: .now()).nanoseconds() ?? 0) / 5)
let delta: DispatchTimeInterval = .nanoseconds(abs(deadline.distance(to: .now()).nanoseconds() ?? 0) / 5)
let handlersDeadline = deadline - delta

let cancellationHandlers = self.registry.get()
Expand All @@ -164,13 +169,19 @@ public final class Cancellator: Cancellable {
try handler(handlersDeadline)
cancelled.append(name)
} catch {
self.observabilityScope?.emit(warning: "failed cancelling '\(name)': \(error)")
self.observabilityScope?.emit(
warning: "failed cancelling '\(name)'",
underlyingError: error
)
}
}
}

if case .timedOut = group.wait(timeout: deadline) {
self.observabilityScope?.emit(warning: "timeout waiting for cancellation with \(cancellationHandlers.count - cancelled.count) cancellation handlers remaining")
self.observabilityScope?
.emit(
warning: "timeout waiting for cancellation with \(cancellationHandlers.count - cancelled.count) cancellation handlers remaining"
)
} else {
self.observabilityScope?.emit(info: "cancellation cycle completed successfully")
}
Expand All @@ -197,7 +208,8 @@ public struct CancellationError: Error, CustomStringConvertible {
}

static func failedToRegisterProcess(_ process: TSCBasic.Process) -> Self {
Self(description: """
Self(
description: """
failed to register a cancellation handler for this process invocation `\(
process.arguments.joined(separator: " ")
)`
Expand Down
6 changes: 4 additions & 2 deletions Sources/Basics/Errors.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,11 @@ public struct InternalError: Error {
extension Error {
public var interpolationDescription: String {
switch self {
// special case because `LocalizedError` conversion will hide the underlying error
case let _error as DecodingError:
return "\(_error)"
case let _error as LocalizedError:
var description = _error.localizedDescription
var description = _error.errorDescription ?? _error.localizedDescription
if let recoverySuggestion = _error.recoverySuggestion {
description += ". \(recoverySuggestion)"
}
Expand All @@ -45,7 +48,6 @@ extension Error {
description += ". \(localizedRecoverySuggestion)"
}
return description

default:
return "\(self)"
}
Expand Down
Loading