improve registry identity lookup cache (#6112)

motivation: avoid unecessary server lookups

changes:
* treat 404 as valid response code
* cache both positive and negative server results to avoid redundant server calls

rdar://104954616

Author: tomerd

Sources/PackageRegistry/RegistryClient.swift

@@ -73,7 +73,7 @@ public final class RegistryClient: Cancellable {
     }
 
     public var configured: Bool {
-        return !self.configuration.isEmpty
+        !self.configuration.isEmpty
     }
 
     /// Cancel any outstanding requests
@@ -118,7 +118,11 @@ public final class RegistryClient: Cancellable {
         self.httpClient.execute(request, observabilityScope: observabilityScope, progress: nil) { result in
             completion(
                 result.tryMap { response in
-                    try self.checkResponseStatusAndHeaders(response, expectedStatusCode: 200, expectedContentType: .json)
+                    try self.checkResponseStatusAndHeaders(
+                        response,
+                        expectedStatusCode: 200,
+                        expectedContentType: .json
+                    )
 
                     guard let data = response.body else {
                         throw RegistryError.invalidResponse
@@ -134,7 +138,7 @@ public final class RegistryClient: Cancellable {
                     return PackageMetadata(
                         registry: registry,
                         versions: versions,
-                        alternateLocations: alternateLocations?.map { $0.url }
+                        alternateLocations: alternateLocations?.map(\.url)
                     )
                 }.mapError {
                     RegistryError.failedRetrievingReleases($0)
@@ -182,7 +186,11 @@ public final class RegistryClient: Cancellable {
         self.httpClient.execute(request, observabilityScope: observabilityScope, progress: nil) { result in
             completion(
                 result.tryMap { response in
-                    try self.checkResponseStatusAndHeaders(response, expectedStatusCode: 200, expectedContentType: .swift)
+                    try self.checkResponseStatusAndHeaders(
+                        response,
+                        expectedStatusCode: 200,
+                        expectedContentType: .swift
+                    )
 
                     guard let data = response.body else {
                         throw RegistryError.invalidResponse
@@ -197,7 +205,8 @@ public final class RegistryClient: Cancellable {
 
                     let alternativeManifests = try response.headers.parseManifestLinks()
                     for alternativeManifest in alternativeManifests {
-                        result[alternativeManifest.filename] = (toolsVersion: alternativeManifest.toolsVersion, content: .none)
+                        result[alternativeManifest.filename] = (toolsVersion: alternativeManifest.toolsVersion,
+                                                                content: .none)
                     }
                     return result
                 }.mapError {
@@ -253,7 +262,11 @@ public final class RegistryClient: Cancellable {
         self.httpClient.execute(request, observabilityScope: observabilityScope, progress: nil) { result in
             completion(
                 result.tryMap { response -> String in
-                    try self.checkResponseStatusAndHeaders(response, expectedStatusCode: 200, expectedContentType: .swift)
+                    try self.checkResponseStatusAndHeaders(
+                        response,
+                        expectedStatusCode: 200,
+                        expectedContentType: .swift
+                    )
 
                     guard let data = response.body else {
                         throw RegistryError.invalidResponse
@@ -310,14 +323,19 @@ public final class RegistryClient: Cancellable {
             switch result {
             case .success(let response):
                 do {
-                    try self.checkResponseStatusAndHeaders(response, expectedStatusCode: 200, expectedContentType: .json)
+                    try self.checkResponseStatusAndHeaders(
+                        response,
+                        expectedStatusCode: 200,
+                        expectedContentType: .json
+                    )
 
                     guard let data = response.body else {
                         throw RegistryError.invalidResponse
                     }
 
                     let versionMetadata = try self.jsonDecoder.decode(Serialization.VersionMetadata.self, from: data)
-                    guard let sourceArchive = versionMetadata.resources.first(where: { $0.name == "source-archive" }) else {
+                    guard let sourceArchive = versionMetadata.resources.first(where: { $0.name == "source-archive" })
+                    else {
                         throw RegistryError.missingSourceArchive
                     }
 
@@ -330,16 +348,21 @@ public final class RegistryClient: Cancellable {
                                                version: version,
                                                fingerprint: .init(origin: .registry(registry.url), value: checksum),
                                                observabilityScope: observabilityScope,
-                                               callbackQueue: callbackQueue) { storageResult in
+                                               callbackQueue: callbackQueue)
+                        { storageResult in
                             switch storageResult {
                             case .success:
                                 completion(.success(checksum))
                             case .failure(PackageFingerprintStorageError.conflict(_, let existing)):
                                 switch self.fingerprintCheckingMode {
                                 case .strict:
-                                    completion(.failure(RegistryError.checksumChanged(latest: checksum, previous: existing.value)))
+                                    completion(.failure(RegistryError
+                                            .checksumChanged(latest: checksum, previous: existing.value)))
                                 case .warn:
-                                    observabilityScope.emit(warning: "The checksum \(checksum) from \(registry.url.absoluteString) does not match previously recorded value \(existing.value) from \(String(describing: existing.origin.url?.absoluteString))")
+                                    observabilityScope
+                                        .emit(
+                                            warning: "The checksum \(checksum) from \(registry.url.absoluteString) does not match previously recorded value \(existing.value) from \(String(describing: existing.origin.url?.absoluteString))"
+                                        )
                                     completion(.success(checksum))
                                 }
                             case .failure(let error):
@@ -435,9 +458,13 @@ public final class RegistryClient: Cancellable {
                             if expectedChecksum != actualChecksum {
                                 switch self.fingerprintCheckingMode {
                                 case .strict:
-                                    return completion(.failure(RegistryError.invalidChecksum(expected: expectedChecksum, actual: actualChecksum)))
+                                    return completion(.failure(RegistryError
+                                            .invalidChecksum(expected: expectedChecksum, actual: actualChecksum)))
                                 case .warn:
-                                    observabilityScope.emit(warning: "The checksum \(actualChecksum) does not match previously recorded value \(expectedChecksum)")
+                                    observabilityScope
+                                        .emit(
+                                            warning: "The checksum \(actualChecksum) does not match previously recorded value \(expectedChecksum)"
+                                        )
                                 }
                             }
                             // validate that the destination does not already exist (again, as this is async)
@@ -478,13 +505,17 @@ public final class RegistryClient: Cancellable {
                                        version: version,
                                        kind: .registry,
                                        observabilityScope: observabilityScope,
-                                       callbackQueue: callbackQueue) { result in
+                                       callbackQueue: callbackQueue)
+                { result in
                     switch result {
                     case .success(let fingerprint):
                         body(.success(fingerprint.value))
                     case .failure(let error):
                         if error as? PackageFingerprintStorageError != .notFound {
-                            observabilityScope.emit(error: "Failed to get registry fingerprint for \(package) \(version) from storage: \(error)")
+                            observabilityScope
+                                .emit(
+                                    error: "Failed to get registry fingerprint for \(package) \(version) from storage: \(error)"
+                                )
                         }
                         // Try fetching checksum from registry again no matter which kind of error it is
                         self.fetchSourceArchiveChecksum(package: package,
@@ -541,6 +572,11 @@ public final class RegistryClient: Cancellable {
 
         self.httpClient.execute(request, observabilityScope: observabilityScope, progress: nil) { result in
             completion(result.tryMap { response in
+                // 404 is valid, no identities mapped
+                if response.statusCode == 404 {
+                    return []
+                }
+
                 try self.checkResponseStatusAndHeaders(response, expectedStatusCode: 200, expectedContentType: .json)
 
                 guard let data = response.body else {
@@ -588,7 +624,9 @@ public final class RegistryClient: Cancellable {
         }
     }
 
-    private func makeAsync<T>(_ closure: @escaping (Result<T, Error>) -> Void, on queue: DispatchQueue) -> (Result<T, Error>) -> Void {
+    private func makeAsync<T>(_ closure: @escaping (Result<T, Error>) -> Void,
+                              on queue: DispatchQueue) -> (Result<T, Error>) -> Void
+    {
         { result in queue.async { closure(result) } }
     }
 
@@ -679,30 +717,34 @@ public enum RegistryError: Error, CustomStringConvertible {
     }
 }
 
-private extension RegistryClient {
-    enum APIVersion: String {
+extension RegistryClient {
+    fileprivate enum APIVersion: String {
         case v1 = "1"
     }
 }
 
-private extension RegistryClient {
-    enum MediaType: String {
+extension RegistryClient {
+    fileprivate enum MediaType: String {
         case json
         case swift
         case zip
     }
 
-    enum ContentType: String {
+    fileprivate enum ContentType: String {
         case json = "application/json"
         case swift = "text/x-swift"
         case zip = "application/zip"
     }
 
-    func acceptHeader(mediaType: MediaType) -> String {
+    private func acceptHeader(mediaType: MediaType) -> String {
         "application/vnd.swift.registry.v\(self.apiVersion.rawValue)+\(mediaType)"
     }
 
-    func checkResponseStatusAndHeaders(_ response: HTTPClient.Response, expectedStatusCode: Int, expectedContentType: ContentType) throws {
+    private func checkResponseStatusAndHeaders(
+        _ response: HTTPClient.Response,
+        expectedStatusCode: Int,
+        expectedContentType: ContentType
+    ) throws {
         guard response.statusCode == expectedStatusCode else {
             throw RegistryError.invalidResponseStatus(expected: expectedStatusCode, actual: response.statusCode)
         }
@@ -727,8 +769,8 @@ extension RegistryClient {
     }
 }
 
-private extension RegistryClient {
-    struct AlternativeLocationLink {
+extension RegistryClient {
+    fileprivate struct AlternativeLocationLink {
         let url: URL
         let kind: Kind
 
@@ -739,21 +781,21 @@ private extension RegistryClient {
     }
 }
 
-private extension RegistryClient {
-    struct ManifestLink {
+extension RegistryClient {
+    fileprivate struct ManifestLink {
         let url: URL
         let filename: String
         let toolsVersion: ToolsVersion
     }
 }
 
-private extension HTTPClientHeaders {
+extension HTTPClientHeaders {
     /*
      <https://github.com/mona/LinkedList>; rel="canonical",
      <ssh://[email protected]:mona/LinkedList.git>; rel="alternate",
       */
-    func parseAlternativeLocationLinks() throws -> [RegistryClient.AlternativeLocationLink]? {
-        return try self.get("Link").map { header -> [RegistryClient.AlternativeLocationLink] in
+    fileprivate func parseAlternativeLocationLinks() throws -> [RegistryClient.AlternativeLocationLink]? {
+        try self.get("Link").map { header -> [RegistryClient.AlternativeLocationLink] in
             let linkLines = header.split(separator: ",").map(String.init).map { $0.spm_chuzzle() ?? $0 }
             return try linkLines.compactMap { linkLine in
                 try parseAlternativeLocationLine(linkLine)
@@ -770,11 +812,15 @@ private extension HTTPClientHeaders {
             return nil
         }
 
-        guard let link = fields.first(where: { $0.hasPrefix("<") }).map({ String($0.dropFirst().dropLast()) }), let url = URL(string: link) else {
+        guard let link = fields.first(where: { $0.hasPrefix("<") }).map({ String($0.dropFirst().dropLast()) }),
+              let url = URL(string: link)
+        else {
             return nil
         }
 
-        guard let rel = fields.first(where: { $0.hasPrefix("rel=") }).flatMap({ parseLinkFieldValue($0) }), let kind = RegistryClient.AlternativeLocationLink.Kind(rawValue: rel) else {
+        guard let rel = fields.first(where: { $0.hasPrefix("rel=") }).flatMap({ parseLinkFieldValue($0) }),
+              let kind = RegistryClient.AlternativeLocationLink.Kind(rawValue: rel)
+        else {
             return nil
         }
 
@@ -785,12 +831,12 @@ private extension HTTPClientHeaders {
     }
 }
 
-private extension HTTPClientHeaders {
+extension HTTPClientHeaders {
     /*
      <http://packages.example.com/mona/LinkedList/1.1.1/Package.swift?swift-version=4>; rel="alternate"; filename="[email protected]"; swift-tools-version="4.0"
      */
-    func parseManifestLinks() throws -> [RegistryClient.ManifestLink] {
-        return try self.get("Link").map { header -> [RegistryClient.ManifestLink] in
+    fileprivate func parseManifestLinks() throws -> [RegistryClient.ManifestLink] {
+        try self.get("Link").map { header -> [RegistryClient.ManifestLink] in
             let linkLines = header.split(separator: ",").map(String.init).map { $0.spm_chuzzle() ?? $0 }
             return try linkLines.compactMap { linkLine in
                 try parseManifestLinkLine(linkLine)
@@ -807,19 +853,26 @@ private extension HTTPClientHeaders {
             return nil
         }
 
-        guard let link = fields.first(where: { $0.hasPrefix("<") }).map({ String($0.dropFirst().dropLast()) }), let url = URL(string: link) else {
+        guard let link = fields.first(where: { $0.hasPrefix("<") }).map({ String($0.dropFirst().dropLast()) }),
+              let url = URL(string: link)
+        else {
             return nil
         }
 
-        guard let rel = fields.first(where: { $0.hasPrefix("rel=") }).flatMap({ parseLinkFieldValue($0) }), rel == "alternate" else {
+        guard let rel = fields.first(where: { $0.hasPrefix("rel=") }).flatMap({ parseLinkFieldValue($0) }),
+              rel == "alternate"
+        else {
             return nil
         }
 
-        guard let filename = fields.first(where: { $0.hasPrefix("filename=") }).flatMap({ parseLinkFieldValue($0) }) else {
+        guard let filename = fields.first(where: { $0.hasPrefix("filename=") }).flatMap({ parseLinkFieldValue($0) })
+        else {
             return nil
         }
 
-        guard let toolsVersion = fields.first(where: { $0.hasPrefix("swift-tools-version=") }).flatMap({ parseLinkFieldValue($0) }) else {
+        guard let toolsVersion = fields.first(where: { $0.hasPrefix("swift-tools-version=") })
+            .flatMap({ parseLinkFieldValue($0) })
+        else {
             return nil
         }
 
@@ -835,8 +888,8 @@ private extension HTTPClientHeaders {
     }
 }
 
-private extension HTTPClientHeaders {
-    func parseLinkFieldValue(_ field: String) -> String? {
+extension HTTPClientHeaders {
+    private func parseLinkFieldValue(_ field: String) -> String? {
         let parts = field.split(separator: "=")
             .map(String.init)
             .map { $0.spm_chuzzle() ?? $0 }
@@ -852,8 +905,8 @@ private extension HTTPClientHeaders {
 // MARK: - Serialization
 
 // marked public for cross module visibility
-public extension RegistryClient {
-    enum Serialization {
+extension RegistryClient {
+    public enum Serialization {
         public struct PackageMetadata: Codable {
             public let releases: [String: Release]
 
@@ -937,16 +990,16 @@ public extension RegistryClient {
 
 // MARK: - Utilities
 
-private extension AbsolutePath {
-    func withExtension(_ extension: String) -> AbsolutePath {
+extension AbsolutePath {
+    fileprivate func withExtension(_ extension: String) -> AbsolutePath {
         guard !self.isRoot else { return self }
         let `extension` = `extension`.spm_dropPrefix(".")
         return self.parentDirectory.appending(component: "\(basename).\(`extension`)")
     }
 }
 
-private extension URLComponents {
-    mutating func appendPathComponents(_ components: String...) {
+extension URLComponents {
+    fileprivate mutating func appendPathComponents(_ components: String...) {
         path += (path.last == "/" ? "" : "/") + components.joined(separator: "/")
     }
 }

Sources/Workspace/Workspace.swift

@@ -3715,7 +3715,7 @@ extension Workspace {
         private let transformationMode: TransformationMode
 
         private let cacheTTL = DispatchTimeInterval.seconds(300) // 5m
-        private let identitiesCache = ThreadSafeKeyValueStore<URL, (identity: PackageIdentity, expirationTime: DispatchTime)>()
+        private let identityLookupCache = ThreadSafeKeyValueStore<URL, (result: Result<PackageIdentity?, Error>, expirationTime: DispatchTime)>()
 
         init(underlying: ManifestLoaderProtocol, registryClient: RegistryClient, transformationMode: TransformationMode) {
             self.underlying = underlying
@@ -3946,18 +3946,25 @@ extension Workspace {
             callbackQueue: DispatchQueue,
             completion: @escaping (Result<PackageIdentity?, Error>) -> Void
         ) {
-            if let cached = self.identitiesCache[url], cached.expirationTime > .now() {
-                return completion(.success(cached.identity))
+            if let cached = self.identityLookupCache[url], cached.expirationTime > .now() {
+                switch cached.result {
+                case .success(let identity):
+                    return completion(.success(identity))
+                case .failure:
+                    // server error, do not try again
+                    return completion(.success(.none))
+                }
             }
 
             self.registryClient.lookupIdentities(url: url, observabilityScope: observabilityScope, callbackQueue: callbackQueue) { result in
                 switch result {
                 case .failure(let error):
+                    self.identityLookupCache[url] = (result: .failure(error), expirationTime: .now() + self.cacheTTL)
                     completion(.failure(error))
                 case .success(let identities):
                     // FIXME: returns first result... need to consider how to address multiple ones
                     let identity = identities.first
-                    self.identitiesCache[url] = identity.map { (identity: $0, expirationTime: .now() + self.cacheTTL) }
+                    self.identityLookupCache[url] = (result: .success(identity), expirationTime: .now() + self.cacheTTL)
                     completion(.success(identity))
                 }
             }