Lots more cleaning up (#11)

* Update min SwiftHTTPTypes version
* Fix readme link
* Add missing dependency declaration
* Lots of general code cleanup
* Log better timing in NetworkClient
* Improve locking usage in AuthState for TokenAuthenticationHandler
* Tests respect LOG_LEVEL env

Author: gwynne

Package.swift

@@ -24,21 +24,29 @@ let swiftSettings: [SwiftSetting] = [
 
 let package = Package(
     name: "StructuredAPIClient",
+    platforms: [
+        .macOS(.v10_15),
+        .iOS(.v13),
+        .watchOS(.v6),
+        .tvOS(.v13),
+    ],
     products: [
         .library(name: "StructuredAPIClient", targets: ["StructuredAPIClient"]),
         .library(name: "StructuredAPIClientTestSupport", targets: ["StructuredAPIClientTestSupport"]),
     ],
     dependencies: [
         // Swift logging API
         .package(url: "https://github.com/apple/swift-log.git", from: "1.5.0"),
-        .package(url: "https://github.com/apple/swift-http-types.git", from: "1.0.0"),
+        .package(url: "https://github.com/apple/swift-http-types.git", from: "1.0.2"),
+        .package(url: "https://github.com/stairtree/async-helpers.git", from: "0.2.0"),
     ],
     targets: [
         .target(
             name: "StructuredAPIClient",
             dependencies: [
                 .product(name: "Logging", package: "swift-log"),
                 .product(name: "HTTPTypes", package: "swift-http-types"),
+                .product(name: "AsyncHelpers", package: "async-helpers"),
             ],
             swiftSettings: swiftSettings
         ),

README.md

@@ -11,7 +11,7 @@
 </a>
 
 <a href="https://codecov.io/gh/stairtree/StructuredAPIClient">
-<img src="https://img.shields.io/codecov/c/gh/stairtree/StructuredAPIClient?style=plastic&logo=codecov&label=codecov&token=MD69L97AOO">
+<img src="https://img.shields.io/codecov/c/gh/stairtree/StructuredAPIClient?style=plastic&logo=codecov&label=codecov">
 </a>
 
 <a href="https://swift.org">

Sources/StructuredAPIClient/Handlers/AddHTTPHeadersHandler.swift

@@ -16,52 +16,58 @@ import Foundation
 import FoundationNetworking
 #endif
 
-/// Add headers to an existing `Transport`.
+/// Add headers to all requests made by an existing ``Transport``.
 public final class AddHTTPHeadersHandler: Transport {
 
     /// An enumeration of the possible modes for working with headers.
-    public enum Mode: CaseIterable, Sendable {
-        /// Accumulating behavior - if a given header is already specified by a request, the transport's value is
-        /// appended, as per `URLRequest.addValue(_:forHTTPHeaderField:)`.
+    public enum Mode: CaseIterable, Sendable, Hashable {
+        /// Accumulating behavior - if a given header is already specified by a request, the transport's
+        /// value is appended, as per `URLRequest.addValue(_:forHTTPHeaderField:)`.
         ///
         /// - Warning: This is rarely what you want.
         case append
 
-        /// Overwriting behavior - if a given header is already specified by a request, the transport's value for the
-        /// header replaces it, as per `URLRequest.setValue(_:forHTTPHeaderField:)`. In this mode, a request's header
-        /// value is always overwritten.
+        /// Overwriting behavior - if a given header is already specified by a request, the transport's
+        /// value for the header replaces it, as per `URLRequest.setValue(_:forHTTPHeaderField:)`. In this
+        /// mode, a request's header value is always overwritten.
         case replace
 
-        /// Polyfill behavior - if a given header is already specified by a request, it is left untouched, and the
-        /// transport's value is ignored.
+        /// Polyfill behavior - if a given header is already specified by a request, it is left untouched,
+        /// and the transport's value is ignored.
         ///
         /// This behavior is the default.
         case add
     }
 
-    /// The base `Transport` to extend with extra headers.
+    /// The base ``Transport`` to extend with extra headers.
     ///
     /// - Note: Never `nil` in practice for this transport.
     public let next: (any Transport)?
 
     /// Additional headers that will be applied to the request upon sending.
     private let headers: [String: String]
 
-    /// The mode used to add additional headers. Defaults to `.append` for legacy compatibility.
+    /// The mode used to add additional headers. Defaults to ``Mode/append`` for legacy compatibility.
     private let mode: Mode
 
-    /// Create a `Transport` that adds headers to the base `Transport`
+    /// Create a ``Transport`` that adds headers to the given base ``Transport``
     /// - Parameters:
-    ///   - base: The base `Transport` that will have the headers applied
-    ///   - headers: Headers to apply to the base `Transport`
-    ///   - mode: The mode to use for resolving conflicts between a request's headers and the transport's headers.
+    ///   - base: The base ``Transport`` that will have the headers applied
+    ///   - headers: Headers to apply to the base ``Transport``
+    ///   - mode: The mode to use for resolving conflicts between a request's headers and the
+    ///     transport's headers.
     public init(base: any Transport, headers: [String: String], mode: Mode = .add) {
         self.next = base
         self.headers = headers
         self.mode = mode
     }
 
+    // See `Transport.send(request:completion:)`
     public func send(request: URLRequest, completion: @escaping @Sendable (Result<TransportResponse, any Error>) -> Void) {
+        guard !self.headers.isEmpty else {
+            return self.next?.send(request: request, completion: completion) ?? ()
+        }
+
         var newRequest = request
 
         for (key, value) in self.headers {

Sources/StructuredAPIClient/Handlers/TokenAuthenticationHandler.swift

@@ -16,8 +16,9 @@ import Foundation
 @preconcurrency import FoundationNetworking
 #endif
 import Logging
+import AsyncHelpers
 
-// Handle token auth and add appropriate auth headers to an existing transport.
+// Handle token auth and add appropriate auth headers to requests sent by an existing ``Transport``.
 public final class TokenAuthenticationHandler: Transport {
     public let next: (any Transport)?
     private let logger: Logger
@@ -58,58 +59,40 @@ public protocol Token: Sendable {
 }
 
 final class AuthState: @unchecked Sendable {
-    private final class LockedTokens: @unchecked Sendable {
-        private let lock = NSLock()
-        private var accessToken: (any Token)?
-        private var refreshToken: (any Token)?
-        
-        init(accessToken: (any Token)?, refreshToken: (any Token)?) {
-            self.accessToken = accessToken
-            self.refreshToken = refreshToken
-        }
-        
-        func withLock<R>(_ closure: @escaping @Sendable (inout (any Token)?, inout (any Token)?) throws -> R) rethrows -> R {
-            try self.lock.withLock {
-                try closure(&self.accessToken, &self.refreshToken)
-            }
-        }
-    }
-    
-    private let tokens: LockedTokens
+    private let tokens: Locking.LockedValueBox<(accessToken: (any Token)?, refreshToken: (any Token)?)>
     let provider: any TokenProvider
     let logger: Logger
 
     internal init(accessToken: (any Token)? = nil, refreshToken: (any Token)? = nil, provider: any TokenProvider, logger: Logger? = nil) {
-        self.tokens = .init(accessToken: accessToken, refreshToken: refreshToken)
+        self.tokens = .init((accessToken: accessToken, refreshToken: refreshToken))
         self.provider = provider
         self.logger = logger ?? Logger(label: "AuthState")
     }
 
     func token(_ completion: @escaping @Sendable (Result<String, any Error>) -> Void) {
-        if let raw = self.tokens.withLock({ token, _ in token.flatMap { ($0.expiresAt ?? Date.distantFuture) > Date() ? $0.raw : nil } }) {
+        let (latestAccessToken, latestRefreshToken) = self.tokens.withLockedValue { ($0.accessToken, $0.refreshToken) }
+
+        if let raw = latestAccessToken.flatMap({ ($0.expiresAt ?? Date.distantFuture) > Date() ? $0.raw : nil }) {
             return completion(.success(raw))
-        } else if let refresh = self.tokens.withLock({ _, token in token.flatMap { ($0.expiresAt ?? Date.distantFuture) > Date() ? $0 : nil } }) {
-            logger.trace("Refreshing token")
+        } else if let refresh = latestRefreshToken.flatMap({ ($0.expiresAt ?? Date.distantFuture) > Date() ? $0 : nil }) {
+            self.logger.trace("Refreshing token")
             self.provider.refreshToken(withRefreshToken: refresh, completion: { result in
                 switch result {
                 case let .failure(error):
                     return completion(.failure(error))
                 case let .success(access):
-                    self.tokens.withLock { token, _ in token = access }
+                    self.tokens.withLockedValue { $0.accessToken = access }
                     return completion(.success(access.raw))
                 }
             })
         } else {
-            logger.trace("Fetching initial tokens")
+            self.logger.trace("Fetching initial tokens")
             self.provider.fetchToken(completion: { result in
                 switch result {
                 case let .failure(error):
                     return completion(.failure(error))
                 case let .success((access, refresh)):
-                    self.tokens.withLock { accessToken, refreshToken in
-                        accessToken = access
-                        refreshToken = refresh
-                    }
+                    self.tokens.withLockedValue { $0 = (accessToken: access, refreshToken: refresh) }
                     return completion(.success(access.raw))
                 }
             })

Sources/StructuredAPIClient/NetworkClient+AsyncAwait.swift

@@ -29,19 +29,22 @@ public final class NetworkClient {
         self.logger = logger ?? Logger(label: "NetworkClient")
     }
 
-    /// Fetch any `NetworkRequest` type and return the response asynchronously.
+    /// Fetch any ``NetworkRequest`` type and return the response asynchronously.
     public func load<Request: NetworkRequest>(_ req: Request, completion: @escaping @Sendable (Result<Request.ResponseDataType, any Error>) -> Void) {
-        let start = DispatchTime.now()
+        let start = Date()
         // Construct the URLRequest
         do {
             let logger = self.logger
-            let urlRequest = try req.makeRequest(baseURL: baseURL)
-            logger.trace("\(urlRequest.debugString)")
+            let urlRequest = try req.makeRequest(baseURL: self.baseURL)
 
             // Send it to the transport
-            transport().send(request: urlRequest) { result in
-                // TODO: Deliver a more accurate split of the different phases of the request
-                defer { logger.trace("Request '\(urlRequest.debugString)' took \(String(format: "%.4f", (.now() - start).milliseconds))ms") }
+            self.transport().send(request: urlRequest) { result in
+                let middle = Date()
+                logger.trace("Request '\(urlRequest.debugString)' received response in \(start.millisecondsBeforeNowFormatted)ms")
+                defer {
+                    logger.trace("Request '\(urlRequest.debugString)' was processed in \(middle.millisecondsBeforeNowFormatted)ms")
+                    logger.trace("Request '\(urlRequest.debugString)' took a total of \(start.millisecondsBeforeNowFormatted)ms")
+                }
 
                 completion(result.flatMap { resp in .init { try req.parseResponse(resp) } })
             }
@@ -51,22 +54,33 @@ public final class NetworkClient {
     }
 }
 
-internal extension DispatchTime {
-    static func -(lhs: Self, rhs: Self) -> Self {
-        .init(uptimeNanoseconds: lhs.uptimeNanoseconds - rhs.uptimeNanoseconds)
+extension NetworkClient {
+    /// Fetch any ``NetworkRequest`` type asynchronously and return the response.
+    public func load<Request: NetworkRequest>(_ req: Request) async throws -> Request.ResponseDataType {
+        try await withCheckedThrowingContinuation { continuation in
+            self.load(req) {
+                continuation.resume(with: $0)
+            }
+        }
     }
-    
-    var milliseconds: Double {
-        Double(self.uptimeNanoseconds) / 1_000_000
+}
+
+private extension Date {
+    var millisecondsBeforeNowFormatted: String {
+        #if canImport(Darwin)
+        if #available(macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0, *) {
+            return (self.timeIntervalSinceNow * -1000.0).formatted(.number.precision(.fractionLength(4...4)).grouping(.never))
+        }
+        #endif
+        
+        // This is both easier and much faster than using NumberFormatter
+        let msInterval = (self.timeIntervalSinceNow * -10_000_000.0).rounded(.toNearestOrEven) / 10_000.0
+        return "\(Int(msInterval))\("\(msInterval)0000".drop(while: { $0 != "." }).prefix(5))"
     }
 }
 
-#if !canImport(Darwin)
-extension NSLocking {
-    package func withLock<R>(_ body: @Sendable () throws -> R) rethrows -> R {
-        self.lock()
-        defer { self.unlock() }
-        return try body()
+internal extension URLRequest {
+    var debugString: String {
+        "\(self.httpMethod.map { "[\($0)] " } ?? "")\(url.map { "\($0) " } ?? "")"
     }
 }
-#endif

Sources/StructuredAPIClient/NetworkClient.swift

@@ -17,20 +17,20 @@ import FoundationNetworking
 #endif
 import HTTPTypes
 
-/// Any request that can be sent as a `URLRequest` with a `NetworkClient`, and returns a response.
+/// Any request that can be sent as a `URLRequest` with a ``NetworkClient``, and returns a response.
 public protocol NetworkRequest: Sendable {
     /// The decoded data type that represents the response.
     associatedtype ResponseDataType: Sendable
 
     /// Returns a request based on the given base URL.
-    /// - Parameter baseURL: The `NetworkClient`'s base URL.
+    /// - Parameter baseURL: The ``NetworkClient``'s base URL.
     func makeRequest(baseURL: URL) throws -> URLRequest
 
-    /// Processes a response returned from the transport and either returns the associated response type or throws an
-    /// application-specific error.
+    /// Processes a response returned from the transport and either returns the associated response type
+    /// or throws an application-specific error.
     ///
     /// - Parameters:
-    ///   - response: A `TransportResponse` containing the response to a request.
+    ///   - response: A ``TransportResponse`` containing the response to a request.
     func parseResponse(_ response: TransportResponse) throws -> ResponseDataType
 }