diff --git a/Demo/Demo/Gravatar-UIKit-Demo/DemoUIImageViewExtensionViewController.swift b/Demo/Demo/Gravatar-UIKit-Demo/DemoUIImageViewExtensionViewController.swift index 7a63bc21..f0fe75f6 100644 --- a/Demo/Demo/Gravatar-UIKit-Demo/DemoUIImageViewExtensionViewController.swift +++ b/Demo/Demo/Gravatar-UIKit-Demo/DemoUIImageViewExtensionViewController.swift @@ -135,11 +135,12 @@ class DemoUIImageViewExtensionViewController: UIViewController { present(controller, animated: true) } + var task: Task? @objc private func fetchAvatarButtonHandler() { let options = setupOptions() let placeholderImage: UIImage? = showPlaceholderSwitchWithLabel.isOn ? UIImage(named: "placeholder") : nil - Task { + task = Task { do { let result = try await avatarImageView.gravatar.setImage( avatarID: .email(emailInputField.text ?? ""), @@ -150,7 +151,6 @@ class DemoUIImageViewExtensionViewController: UIViewController { print("success!") print("result url: \(result.sourceURL)") print("retrived Image point size: \(result.image.size)") - } catch { print(error) } @@ -158,9 +158,7 @@ class DemoUIImageViewExtensionViewController: UIViewController { } @objc private func cancelOngoingButtonHandler() { - Task { - await avatarImageView.gravatar.cancelImageDownload() - } + task?.cancel() } private func setupOptions() -> [ImageSettingOption] { diff --git a/Sources/Gravatar/Network/Services/ImageDownloadService.swift b/Sources/Gravatar/Network/Services/ImageDownloadService.swift index e8a0c880..37f350a6 100644 --- a/Sources/Gravatar/Network/Services/ImageDownloadService.swift +++ b/Sources/Gravatar/Network/Services/ImageDownloadService.swift @@ -4,7 +4,7 @@ import UIKit /// /// This is the default type which implements ``ImageDownloader``.. /// Unless specified otherwise, `ImageDownloadService` will use a `URLSession` based `HTTPClient`, and a in-memory image cache. -public struct ImageDownloadService: ImageDownloader, Sendable { +public actor ImageDownloadService: ImageDownloader, Sendable { private let client: HTTPClient let imageCache: ImageCaching @@ -29,12 +29,20 @@ public struct ImageDownloadService: ImageDownloader, Sendable { let request = URLRequest.imageRequest(url: url, forceRefresh: forceRefresh) if !forceRefresh, let image = try await cachedImage(for: url) { + try Task.checkCancellation() return ImageDownloadResult(image: image, sourceURL: url) } + let task = Task { try await fetchAndProcessImage(request: request, processor: processingMethod.processor) } - let image = try await awaitAndCacheImage(from: task, cacheKey: url.absoluteString) + + // Create `.inProgress` entry before we await to prevent re-entrancy issues + let cacheKey = url.absoluteString + imageCache.setEntry(.inProgress(task), for: cacheKey) + + let image = try await awaitAndCacheImage(from: task, cacheKey: cacheKey) + try Task.checkCancellation() return ImageDownloadResult(image: image, sourceURL: url) } @@ -43,6 +51,7 @@ public struct ImageDownloadService: ImageDownloader, Sendable { switch entry { case .inProgress(let task): let image = try await task.value + try Task.checkCancellation() return image case .ready(let image): return image @@ -50,10 +59,10 @@ public struct ImageDownloadService: ImageDownloader, Sendable { } private func awaitAndCacheImage(from task: Task, cacheKey key: String) async throws -> UIImage { - imageCache.setEntry(.inProgress(task), for: key) let image: UIImage do { image = try await task.value + try Task.checkCancellation() } catch { imageCache.setEntry(nil, for: key) throw error @@ -65,6 +74,7 @@ public struct ImageDownloadService: ImageDownloader, Sendable { private func fetchAndProcessImage(request: URLRequest, processor: ImageProcessor) async throws -> UIImage { do { let (data, _) = try await client.fetchData(with: request) + try Task.checkCancellation() guard let image = processor.process(data) else { throw ImageFetchingError.imageProcessorFailed } @@ -77,20 +87,6 @@ public struct ImageDownloadService: ImageDownloader, Sendable { throw ImageFetchingError.responseError(reason: .unexpected(error)) } } - - public func cancelTask(for url: URL) { - if let entry = imageCache.getEntry(with: url.absoluteString) { - switch entry { - case .inProgress(let task): - if !task.isCancelled { - task.cancel() - imageCache.setEntry(nil, for: url.absoluteString) - } - default: - break - } - } - } } extension URLRequest { diff --git a/Sources/Gravatar/Network/Services/ImageDownloader.swift b/Sources/Gravatar/Network/Services/ImageDownloader.swift index 17f743c7..eb609537 100644 --- a/Sources/Gravatar/Network/Services/ImageDownloader.swift +++ b/Sources/Gravatar/Network/Services/ImageDownloader.swift @@ -14,8 +14,4 @@ public protocol ImageDownloader: Sendable { forceRefresh: Bool, processingMethod: ImageProcessingMethod ) async throws -> ImageDownloadResult - - /// Cancels the download task for the given `URL`. - /// - Parameter url: `URL` of the download task. - func cancelTask(for url: URL) } diff --git a/Sources/GravatarUI/GravatarCompatibleUI/UIImageView+Gravatar.swift b/Sources/GravatarUI/GravatarCompatibleUI/UIImageView+Gravatar.swift index 01a9990f..3fa07c6d 100644 --- a/Sources/GravatarUI/GravatarCompatibleUI/UIImageView+Gravatar.swift +++ b/Sources/GravatarUI/GravatarCompatibleUI/UIImageView+Gravatar.swift @@ -124,12 +124,6 @@ extension GravatarWrapper where Component: UIImageView { } } - public func cancelImageDownload() { - if let sourceURL { - imageDownloader?.cancelTask(for: sourceURL) - } - } - /// Downloads the Gravatar profile image and sets it to this UIImageView. Throws ``ImageFetchingComponentError``. /// /// - Parameters: diff --git a/Sources/TestHelpers/TestImageCache.swift b/Sources/TestHelpers/TestImageCache.swift index b3bd4967..1348d570 100644 --- a/Sources/TestHelpers/TestImageCache.swift +++ b/Sources/TestHelpers/TestImageCache.swift @@ -1,31 +1,63 @@ import Gravatar import UIKit -package class TestImageCache: ImageCaching, @unchecked Sendable { +package final class TestImageCache: ImageCaching, @unchecked Sendable { let imageCache = ImageCache() - package private(set) var getImageCallsCount = 0 - package private(set) var setImageCallsCount = 0 - package private(set) var setTaskCallsCount = 0 + package typealias CacheMessage = (operation: CacheMessageType, key: String) + private var cacheMessages = [CacheMessage]() + + package enum CacheMessageType { + case setToNil + case inProgress + case ready + case get + } + + package var getImageCallsCount: Int { messageCount(type: .get) } + package var setImageCallsCount: Int { messageCount(type: .ready) } + package var setTaskCallsCount: Int { messageCount(type: .inProgress) } + + // Serial queue to synchronize access to shared mutable state + private let accessQueue = DispatchQueue(label: "com.testImageCache.accessQueue") package init() {} package func setEntry(_ entry: Gravatar.CacheEntry?, for key: String) { - guard let entry else { - imageCache.setEntry(nil, for: key) - return - } - switch entry { - case .inProgress: - setTaskCallsCount += 1 - case .ready: - setImageCallsCount += 1 + accessQueue.sync { + var message: CacheMessage + defer { cacheMessages.append(message) } + guard let entry else { + imageCache.setEntry(nil, for: key) + message = (operation: .setToNil, key: key) + return + } + switch entry { + case .inProgress: + message = (operation: .inProgress, key: key) + case .ready: + message = (operation: .ready, key: key) + } + imageCache.setEntry(entry, for: key) } - imageCache.setEntry(entry, for: key) } package func getEntry(with key: String) -> Gravatar.CacheEntry? { - getImageCallsCount += 1 - return imageCache.getEntry(with: key) + accessQueue.sync { + cacheMessages.append(CacheMessage(operation: .get, key: key)) + return imageCache.getEntry(with: key) + } + } + + package func messageCount(type: CacheMessageType) -> Int { + accessQueue.sync { + cacheMessages.filter { $0.operation == type }.count + } + } + + package func messageCount(type: CacheMessageType, forKey key: String) -> Int { + accessQueue.sync { + cacheMessages.filter { $0.operation == type && $0.key == key }.count + } } } diff --git a/Sources/TestHelpers/URLSessionMock.swift b/Sources/TestHelpers/URLSessionMock.swift index 7516f3a9..c219e37b 100644 --- a/Sources/TestHelpers/URLSessionMock.swift +++ b/Sources/TestHelpers/URLSessionMock.swift @@ -11,7 +11,7 @@ package actor URLSessionMock: URLSessionProtocol { let returnData: Data let response: HTTPURLResponse - let error: NSError? + private(set) var error: NSError? private(set) var isCancellable: Bool = false private(set) var maxDurationSeconds: Double = 2 package private(set) var callsCount = 0 @@ -63,6 +63,10 @@ package actor URLSessionMock: URLSessionProtocol { self.request = request } + package func update(error: NSError?) async { + self.error = error + } + package func update(isCancellable: Bool) async { self.isCancellable = isCancellable } diff --git a/Tests/GravatarTests/ImageDownloadServiceTests.swift b/Tests/GravatarTests/ImageDownloadServiceTests.swift index 2772618a..e360fbe0 100644 --- a/Tests/GravatarTests/ImageDownloadServiceTests.swift +++ b/Tests/GravatarTests/ImageDownloadServiceTests.swift @@ -27,52 +27,43 @@ final class ImageDownloadServiceTests: XCTestCase { do { let _ = try await service.fetchImage(with: imageURL) XCTFail() - } catch ImageFetchingError.responseError(reason: .URLSessionError(error: let error)) { + } catch { XCTAssertNotNil(error as? CancellationError) let entry = cache.getEntry(with: imageURL.absoluteString) XCTAssertNil(entry) - } catch { - XCTFail() } } - let task2 = Task { - try await Task.sleep(nanoseconds: UInt64(0.05 * 1_000_000_000)) - service.cancelTask(for: imageURL) - } + try await Task.sleep(nanoseconds: UInt64(0.05 * 1_000_000_000)) + task1.cancel() await task1.value - try await task2.value } func testCallAfterAFailedCallWorksFine() async throws { + let cache = TestImageCache() + let imageURL = try XCTUnwrap(URL(string: "https://gravatar.com/avatar/HASH")) let response = HTTPURLResponse.successResponse(with: imageURL) - let sessionMock = URLSessionMock(returnData: ImageHelper.testImageData, response: response) + let sessionMock = URLSessionMock(returnData: ImageHelper.testImageData, response: response, error: NSError(domain: "test", code: 1)) await sessionMock.update(isCancellable: true) - let service = imageDownloadService(with: sessionMock) - + let service = imageDownloadService(with: sessionMock, cache: cache) let task1 = Task { do { let _ = try await service.fetchImage(with: imageURL) XCTFail() - } catch ImageFetchingError.responseError(reason: .URLSessionError(error: let error)) { - XCTAssertNotNil(error as? CancellationError) } catch { - XCTFail() + XCTAssertNotNil(error) + let entry = cache.getEntry(with: imageURL.absoluteString) + XCTAssertNil(entry) } } - let task2 = Task { - try await Task.sleep(nanoseconds: UInt64(0.1 * 1_000_000_000)) - service.cancelTask(for: imageURL) - } - await task1.value - try await task2.value - // The task is cancelled, now we retry and it should succeed. + // The task has failed, now we retry and it should succeed. await sessionMock.update(isCancellable: false) + await sessionMock.update(error: nil) let result = try await service.fetchImage(with: imageURL) XCTAssertNotNil(result.image) } @@ -116,6 +107,138 @@ final class ImageDownloadServiceTests: XCTestCase { XCTAssertEqual(request?.url?.absoluteString, "https://gravatar.com/avatar/HASH") XCTAssertNotNil(imageResponse.image) } + + func testSimultaneousFetchShouldOnlyTriggerOneNetworkRequest() async throws { + let imageURL = URL(string: "https://example.com/image.png")! + + let mockImageData = UIImage(systemName: "iphone.gen2")!.pngData()! + + let sessionMock = URLSessionMock( + returnData: mockImageData, + response: HTTPURLResponse.successResponse(with: imageURL) + ) + + // Simulate download tasks that have a longer duration + await sessionMock.update(isCancellable: true) + + let cache = TestImageCache() + let service = imageDownloadService(with: sessionMock, cache: cache) + + // When + // Start simultaneous fetches + let fetchTask1 = Task { try await service.fetchImage(with: imageURL, forceRefresh: false) } + let fetchTask2 = Task { try await service.fetchImage(with: imageURL, forceRefresh: false) } + let fetchTask3 = Task { try await service.fetchImage(with: imageURL, forceRefresh: false) } + let fetchTask4 = Task { try await service.fetchImage(with: imageURL, forceRefresh: false) } + let fetchTask5 = Task { try await service.fetchImage(with: imageURL, forceRefresh: false) } + + // Then + let result1 = try await fetchTask1.value + let result2 = try await fetchTask2.value + let result3 = try await fetchTask3.value + let result4 = try await fetchTask4.value + let result5 = try await fetchTask5.value + + // Assert that all fetches return the same image + XCTAssertEqual(result1.image.pngData(), mockImageData) + XCTAssertEqual(result2.image.pngData(), mockImageData) + XCTAssertEqual(result3.image.pngData(), mockImageData) + XCTAssertEqual(result4.image.pngData(), mockImageData) + XCTAssertEqual(result5.image.pngData(), mockImageData) + + // Assert that all fetches attempted to read from the cache + XCTAssertEqual(cache.messageCount(type: .get), 5) + + // Assert that only one fetch set an `.inProgress` CacheEntry + XCTAssertEqual(cache.messageCount(type: .inProgress, forKey: imageURL.absoluteString), 1) + + // Assert that only one fetch set an `.ready` CacheEntry + XCTAssertEqual(cache.messageCount(type: .ready, forKey: imageURL.absoluteString), 1) + } + + func testSimultaneousFetchShouldOnlyTriggerOneNetworkRequestPerUrl() async throws { + let imageURL1 = URL(string: "https://example.com/image1.png")! + let imageURL2 = URL(string: "https://example.com/image2.png")! + + let mockImageData = UIImage(systemName: "iphone.gen2")!.pngData()! + + let sessionMock = URLSessionMock( + returnData: mockImageData, + response: HTTPURLResponse() + ) + + let cache = TestImageCache() + let service = imageDownloadService(with: sessionMock, cache: cache) + + let expectation = XCTestExpectation(description: "Image fetches should complete") + + // When + // Start simultaneous fetches + let fetchTask1 = Task { try await service.fetchImage(with: imageURL1, forceRefresh: false) } + let fetchTask2 = Task { try await service.fetchImage(with: imageURL2, forceRefresh: false) } + let fetchTask3 = Task { try await service.fetchImage(with: imageURL1, forceRefresh: false) } + let fetchTask4 = Task { try await service.fetchImage(with: imageURL2, forceRefresh: false) } + let fetchTask5 = Task { try await service.fetchImage(with: imageURL1, forceRefresh: false) } + let fetchTask6 = Task { try await service.fetchImage(with: imageURL2, forceRefresh: false) } + + // Then + let result1 = try await fetchTask1.value + let result2 = try await fetchTask2.value + let result3 = try await fetchTask3.value + let result4 = try await fetchTask4.value + let result5 = try await fetchTask5.value + let result6 = try await fetchTask6.value + + expectation.fulfill() + await fulfillment(of: [expectation], timeout: 0.5) + + // Assert that all fetches return the same image + XCTAssertEqual(result1.image.pngData(), mockImageData) + XCTAssertEqual(result2.image.pngData(), mockImageData) + XCTAssertEqual(result3.image.pngData(), mockImageData) + XCTAssertEqual(result4.image.pngData(), mockImageData) + XCTAssertEqual(result5.image.pngData(), mockImageData) + XCTAssertEqual(result6.image.pngData(), mockImageData) + + // Assert that all fetches attempted to read from the cache + XCTAssertEqual( + cache.messageCount(type: .get), + 6, + "All fetches should have attempted to read from the cache" + ) + XCTAssertEqual( + cache.messageCount(type: .get, forKey: imageURL1.absoluteString), 3, + "All fetches for '\(imageURL1)' should have attempted to read from the cache" + ) + XCTAssertEqual( + cache.messageCount(type: .get, forKey: imageURL2.absoluteString), 3, + "All fetches for '\(imageURL2)' should have attempted to read from the cache" + ) + + // Assert that only one fetch set an `.inProgress` CacheEntry + XCTAssertEqual( + cache.messageCount(type: .inProgress, forKey: imageURL1.absoluteString), + 1, + "Only one fetch for '\(imageURL1)' should have set an `.inProgress` CacheEntry" + ) + XCTAssertEqual( + cache.messageCount(type: .inProgress, forKey: imageURL2.absoluteString), + 1, + "Only one fetch for '\(imageURL2)' should have set an `.inProgress` CacheEntry" + ) + + // Assert that only one fetch set an `.ready` CacheEntry + XCTAssertEqual( + cache.messageCount(type: .ready, forKey: imageURL1.absoluteString), + 1, + "Only one fetch for '\(imageURL1)' should have set a `.ready` CacheEntry" + ) + XCTAssertEqual( + cache.messageCount(type: .ready, forKey: imageURL2.absoluteString), + 1, + "Only one fetch for '\(imageURL2)' should have set a `.ready` CacheEntry" + ) + } } private func imageDownloadService(with session: URLSessionProtocol, cache: ImageCaching? = nil) -> ImageDownloadService { diff --git a/Tests/GravatarUITests/GravatarWrapper+UIImageViewTests.swift b/Tests/GravatarUITests/GravatarWrapper+UIImageViewTests.swift index da4c65df..413e9ac2 100644 --- a/Tests/GravatarUITests/GravatarWrapper+UIImageViewTests.swift +++ b/Tests/GravatarUITests/GravatarWrapper+UIImageViewTests.swift @@ -83,40 +83,6 @@ final class GravatarWrapper_UIImageViewTests: XCTestCase { XCTAssertNotNil(imageView.gravatar.placeholder) } - @MainActor - func testCancelOngoingDownload() async throws { - let imageView = UIImageView(frame: frame) - let cache = TestImageCache() - - let imageURL = try XCTUnwrap(URL(string: "https://gravatar.com/avatar/HASH")) - let response = HTTPURLResponse.successResponse(with: imageURL) - let sessionMock = URLSessionMock(returnData: ImageHelper.testImageData, response: response) - await sessionMock.update(isCancellable: true) - let imageDownloader = ImageDownloadService.mock(with: sessionMock, cache: cache) - - let task1 = Task { - do { - try await imageView.gravatar.setImage( - avatarID: .email("hello@gmail.com"), - options: [.imageDownloader(imageDownloader)] - ) - XCTFail() - } catch ImageFetchingComponentError.responseError(reason: .URLSessionError(error: let error)) { - XCTAssertNotNil(error as? CancellationError) - } catch { - XCTFail() - } - } - - let task2 = Task { - try await Task.sleep(nanoseconds: UInt64(0.1 * 1_000_000_000)) - await imageView.gravatar.cancelImageDownload() - } - - await task1.value - try await task2.value - } - @MainActor func testRemoveCurrentImageWhileLoadingNoPlaceholder() async throws { let imageView = UIImageView(frame: frame)