Merge pull request #232 from Automattic/etoledom/image-caching-concur…

…rency

Refactor image cache

Sources/Gravatar/Cache/ImageCaching.swift

@@ -1,24 +1,73 @@
-import Foundation
 import UIKit
 
+/// Represents a cache for images.
+///
+/// An ImageCaching represents a cache for CacheEntry elements. Each CacheEntry is either an instance of an image, or the task of retrieving an image from
+/// remote.
 public protocol ImageCaching {
-    func setImage(_ image: UIImage, forKey key: String)
-    func getImage(forKey key: String) -> UIImage?
+    /// Saves an image in the cache.
+    /// - Parameters:
+    ///   - image: The cache entry to set.
+    ///   - key: The entry's key, used to be found via `.getEntry(key:)`.
+    func setEntry(_ entry: CacheEntry, for key: String)
+
+    /// Gets a `CacheEntry` from cache for the given key, or nil if none is found.
+    ///
+    /// - Parameter key: The key for the entry to get.
+    /// - Returns: The cache entry which could contain an image, the task that is retrieving the image. Nil is returned if nothing is found.
+    ///
+    /// `.inProgress(task)`  is used by the image downloader to check if there's already an ongoing download task for the same image. If yes, the image
+    /// downloader  awaits that ask instead of starting a new one.
+    func getEntry(with key: String) -> CacheEntry?
 }
 
-public class ImageCache: ImageCaching {
-    private let cache = NSCache<NSString, UIImage>()
+/// The default `ImageCaching` used by this SDK.
+public struct ImageCache: ImageCaching {
+    private let cache = NSCache<NSString, CacheEntryObject>()
 
     /// The default cache used by the image dowloader.
-    public static var shared: ImageCaching = ImageCache()
+    public static let shared: ImageCaching = ImageCache()
 
     public init() {}
 
-    public func setImage(_ image: UIImage, forKey key: String) {
-        cache.setObject(image, forKey: key as NSString)
+    public func setEntry(_ entry: CacheEntry, for key: String) {
+        cache[key] = .init(entry)
+    }
+
+    public func getEntry(with key: String) -> CacheEntry? {
+        cache[key]
     }
+}
+
+/// ImageCache can save an in-progress task of retreiving an image from remote.
+/// This enum represent both possible states for an image in the cache system.
+public enum CacheEntry: Sendable {
+    /// A task of retreiving an image is in progress.
+    case inProgress(Task<UIImage, Error>)
+    /// An image instance is readily available.
+    case ready(UIImage)
+}
+
+private final class CacheEntryObject {
+    let entry: CacheEntry
+    init(entry: CacheEntry) { self.entry = entry }
+}
 
-    public func getImage(forKey key: String) -> UIImage? {
-        cache.object(forKey: key as NSString)
+extension NSCache where KeyType == NSString, ObjectType == CacheEntryObject {
+    fileprivate subscript(_ key: String) -> CacheEntry? {
+        get {
+            let key = key as NSString
+            let value = object(forKey: key)
+            return value?.entry
+        }
+        set {
+            let key = key as NSString
+            if let entry = newValue {
+                let value = CacheEntryObject(entry: entry)
+                setObject(value, forKey: key)
+            } else {
+                removeObject(forKey: key)
+            }
+        }
     }
 }

Sources/Gravatar/Network/ImageDownloadResult.swift

@@ -2,7 +2,7 @@ import Foundation
 import UIKit
 
 /// Represents the result of a  Gravatar image download task.
-public struct ImageDownloadResult {
+public struct ImageDownloadResult: Sendable {
     public init(image: UIImage, sourceURL: URL) {
         self.image = image
         self.sourceURL = sourceURL

Sources/Gravatar/Network/Services/HTTPClient.swift

@@ -4,7 +4,7 @@ import Foundation
 ///
 /// You can provide your own type conforming to this protocol to gain control over all networking operations performed internally by this SDK.
 /// For more info, see ``AvatarService/init(client:cache:)`` and ``ProfileService/init(client:)``.
-public protocol HTTPClient {
+public protocol HTTPClient: Sendable {
     /// Performs a data request using the  information provided,  and delivers the result asynchronously.
     /// - Parameter request: A URL request object that provides request-specific information such as the URL and cache policy.
     /// - Returns: An asynchronously-delivered tuple that contains the URL contents as a Data instance, and a HTTPURLResponse.

Sources/Gravatar/Network/Services/ImageDownloadService.swift

@@ -22,36 +22,56 @@ public struct ImageDownloadService: ImageDownloader {
 
     private func fetchImage(from url: URL, forceRefresh: Bool, processor: ImageProcessor) async throws -> ImageDownloadResult {
         let request = URLRequest.imageRequest(url: url, forceRefresh: forceRefresh)
-        do {
+
+        let client = self.client
+        let task = Task<UIImage, Error> {
             let (data, _) = try await client.fetchData(with: request)
             guard let image = processor.process(data) else {
                 throw ImageFetchingError.imageProcessorFailed
             }
-            imageCache.setImage(image, forKey: url.absoluteString)
-            return ImageDownloadResult(image: image, sourceURL: url)
-        } catch let error as HTTPClientError {
-            throw ImageFetchingError.responseError(reason: error.map())
-        } catch {
-            throw ImageFetchingError.responseError(reason: .unexpected(error))
+            return image
         }
+
+        imageCache.setEntry(.inProgress(task), for: url.absoluteString)
+
+        let image = try await getImage(from: task)
+        imageCache.setEntry(.ready(image), for: url.absoluteString)
+        return ImageDownloadResult(image: image, sourceURL: url)
     }
 
     public func fetchImage(
         with url: URL,
         forceRefresh: Bool = false,
         processingMethod: ImageProcessingMethod = .common()
     ) async throws -> ImageDownloadResult {
-        if !forceRefresh, let result = cachedImageResult(for: url) {
-            return result
+        if !forceRefresh, let image = try await cachedImage(for: url) {
+            return ImageDownloadResult(image: image, sourceURL: url)
         }
         return try await fetchImage(from: url, forceRefresh: forceRefresh, processor: processingMethod.processor)
     }
 
-    func cachedImageResult(for url: URL) -> ImageDownloadResult? {
-        guard let cachedImage = imageCache.getImage(forKey: url.absoluteString) else {
+    private func cachedImage(for url: URL) async throws -> UIImage? {
+        guard let entry = imageCache.getEntry(with: url.absoluteString) else {
             return nil
         }
-        return ImageDownloadResult(image: cachedImage, sourceURL: url)
+
+        switch entry {
+        case .inProgress(let task):
+            return try await getImage(from: task)
+        case .ready(let image):
+            return image
+        }
+    }
+
+    private func getImage(from task: Task<UIImage, Error>) async throws -> UIImage {
+        do {
+            let image = try await task.value
+            return image
+        } catch let error as HTTPClientError {
+            throw ImageFetchingError.responseError(reason: error.map())
+        } catch {
+            throw ImageFetchingError.responseError(reason: .unexpected(error))
+        }
     }
 }
 

Sources/Gravatar/Network/URLSessionProtocol.swift

@@ -1,7 +1,7 @@
 import UIKit
 
 /// Protocol for dependency injection purposes. `URLSession` conforms to  this protocol.
-public protocol URLSessionProtocol {
+public protocol URLSessionProtocol: Sendable {
     func dataTask(
         with request: URLRequest,
         completionHandler: @escaping @Sendable (Data?, URLResponse?, Error?) -> Void

Sources/Gravatar/UIImage/Processor/ImageProcessor.swift

@@ -2,6 +2,6 @@ import Foundation
 import UIKit
 
 /// Processor to apply to the downloaded image data.
-public protocol ImageProcessor {
+public protocol ImageProcessor: Sendable {
     func process(_ data: Data) -> UIImage?
 }

Tests/GravatarTests/GravatarImageCacheTests.swift

@@ -2,11 +2,22 @@
 import XCTest
 
 final class GravatarImageCacheTests: XCTestCase {
-    private let key: String = "key"
+    private let key = "ImageKey"
 
-    func testSetAndGet() throws {
+    func testSetAndGet() {
         let cache = ImageCache()
-        cache.setImage(ImageHelper.testImage, forKey: key)
-        XCTAssertNotNil(cache.getImage(forKey: key))
+        cache.setEntry(.ready(ImageHelper.testImage), for: key)
+        let image = cache.getEntry(with: key)
+        XCTAssertNotNil(image)
+    }
+
+    func testRequestingMultipleTimes() {
+        let cache = ImageCache()
+        let task = Task<UIImage, Error> {
+            ImageHelper.testImage
+        }
+        cache.setEntry(.inProgress(task), for: key)
+        let image = cache.getEntry(with: key)
+        XCTAssertNotNil(image)
     }
 }

Tests/GravatarTests/ImageDownloadServiceTests.swift

@@ -26,6 +26,7 @@ final class ImageDownloadServiceTests: XCTestCase {
         let imageResponse = try await service.fetchImage(with: URL(string: imageURL)!)
 
         XCTAssertEqual(cache.setImageCallsCount, 1)
+        XCTAssertEqual(cache.setTaskCallCount, 1)
         XCTAssertEqual(cache.getImageCallCount, 3)
         XCTAssertEqual(sessionMock.callsCount, 1)
         XCTAssertEqual(sessionMock.request?.url?.absoluteString, "https://gravatar.com/avatar/HASH")

Tests/GravatarTests/TestImageCache.swift

@@ -1,21 +1,32 @@
-import Foundation
 import Gravatar
 import UIKit
 
 class TestImageCache: ImageCaching {
-    var dict: [String: UIImage] = [:]
+    var dict: [String: CacheEntryWrapper] = [:]
+
     var getImageCallCount = 0
     var setImageCallsCount = 0
+    var setTaskCallCount = 0
 
-    init() {}
-
-    func setImage(_ image: UIImage, forKey key: String) {
-        setImageCallsCount += 1
-        dict[key] = image
+    func setEntry(_ entry: Gravatar.CacheEntry, for key: String) {
+        switch entry {
+        case .inProgress:
+            setTaskCallCount += 1
+        case .ready:
+            setImageCallsCount += 1
+        }
+        dict[key] = CacheEntryWrapper(entry)
     }
 
-    func getImage(forKey key: String) -> UIImage? {
+    func getEntry(with key: String) -> Gravatar.CacheEntry? {
         getImageCallCount += 1
-        return dict[key]
+        return dict[key]?.cacheEntry
+    }
+}
+
+class CacheEntryWrapper {
+    let cacheEntry: CacheEntry
+    init(_ cacheEntry: CacheEntry) {
+        self.cacheEntry = cacheEntry
     }
 }

Tests/GravatarTests/TestImageProcessor.swift

@@ -1,8 +1,7 @@
-import Foundation
 import Gravatar
 import UIKit
 
-class TestImageProcessor: ImageProcessor {
+final class TestImageProcessor: ImageProcessor {
     init() {}
 
     var processedData = false

Tests/GravatarTests/TestURLSession.swift

@@ -9,7 +9,7 @@ enum TestDataTaskFailReason: Equatable {
     case urlMismatch
 }
 
-class TestURLSession: URLSessionProtocol {
+final class TestURLSession: URLSessionProtocol {
     func upload(for request: URLRequest, from bodyData: Data) async throws -> (Data, URLResponse) {
         XCTFail("Not implemented")
         fatalError()

Tests/GravatarTests/URLSessionMock.swift

@@ -1,7 +1,7 @@
 import Foundation
 import Gravatar
 
-class URLSessionMock: URLSessionProtocol {
+final class URLSessionMock: URLSessionProtocol {
     static let jsonData = """
     {
         "name": "John",

Tests/GravatarUITests/TestImageCache.swift

@@ -1,21 +1,32 @@
-import Foundation
 import Gravatar
 import UIKit
 
 class TestImageCache: ImageCaching {
-    var dict: [String: UIImage] = [:]
+    var dict: [String: CacheEntryWrapper] = [:]
+
     var getImageCallCount = 0
     var setImageCallsCount = 0
+    var setTaskCallCount = 0
 
-    init() {}
-
-    func setImage(_ image: UIImage, forKey key: String) {
-        setImageCallsCount += 1
-        dict[key] = image
+    func setEntry(_ entry: Gravatar.CacheEntry, for key: String) {
+        switch entry {
+        case .inProgress:
+            setTaskCallCount += 1
+        case .ready:
+            setImageCallsCount += 1
+        }
+        dict[key] = CacheEntryWrapper(entry)
     }
 
-    func getImage(forKey key: String) -> UIImage? {
+    func getEntry(with key: String) -> Gravatar.CacheEntry? {
         getImageCallCount += 1
-        return dict[key]
+        return dict[key]?.cacheEntry
+    }
+}
+
+class CacheEntryWrapper {
+    let cacheEntry: CacheEntry
+    init(_ cacheEntry: CacheEntry) {
+        self.cacheEntry = cacheEntry
     }
 }

Tests/GravatarUITests/TestImageProcessor.swift

@@ -2,7 +2,7 @@ import Foundation
 import Gravatar
 import UIKit
 
-class TestImageProcessor: ImageProcessor {
+final class TestImageProcessor: ImageProcessor {
     init() {}
 
     var processedData = false

Tests/GravatarUITests/URLSessionMock.swift

@@ -1,7 +1,7 @@
 import Foundation
 import Gravatar
 
-class URLSessionMock: URLSessionProtocol {
+final class URLSessionMock: URLSessionProtocol {
     static let jsonData = """
     {
         "name": "John",