From 1a15766c2f20faab86fe6e547d0efdb1b3b23347 Mon Sep 17 00:00:00 2001 From: Andrew Montgomery Date: Thu, 26 Sep 2024 12:18:41 -0500 Subject: [PATCH 01/18] Update TestImageCache to support thread-safe access to properties --- Sources/TestHelpers/TestImageCache.swift | 53 ++++++++++++++++++------ 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/Sources/TestHelpers/TestImageCache.swift b/Sources/TestHelpers/TestImageCache.swift index b3bd4967..54c1ee9b 100644 --- a/Sources/TestHelpers/TestImageCache.swift +++ b/Sources/TestHelpers/TestImageCache.swift @@ -4,28 +4,57 @@ import UIKit package class TestImageCache: ImageCaching, @unchecked Sendable { let imageCache = ImageCache() + package typealias CacheMessage = (operation: CacheMessageType, key: String) + private var cacheMessages = [CacheMessage]() + + package enum CacheMessageType { + case setToNil + case inProgress + case ready + case get + } + package private(set) var getImageCallsCount = 0 package private(set) var setImageCallsCount = 0 package private(set) var setTaskCallsCount = 0 + // 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: + setTaskCallsCount += 1 + message = (operation: .inProgress, key: key) + case .ready: + setImageCallsCount += 1 + 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 { + getImageCallsCount += 1 + 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 + } } } From 4d07b68f709d77cf3158381edcc1e521b533c708 Mon Sep 17 00:00:00 2001 From: Andrew Montgomery Date: Thu, 26 Sep 2024 12:19:19 -0500 Subject: [PATCH 02/18] Test simultaneous fetches only fetch one image from network --- .../ImageDownloadServiceTests.swift | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/Tests/GravatarTests/ImageDownloadServiceTests.swift b/Tests/GravatarTests/ImageDownloadServiceTests.swift index 2772618a..4801bf75 100644 --- a/Tests/GravatarTests/ImageDownloadServiceTests.swift +++ b/Tests/GravatarTests/ImageDownloadServiceTests.swift @@ -116,6 +116,44 @@ 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 response = HTTPURLResponse.successResponse(with: imageURL) + + let mockImageData = UIImage(systemName: "iphone.gen2")!.pngData()! + let sessionMock = URLSessionMock(returnData: mockImageData, response: response) + let cache = TestImageCache() + let service = imageDownloadService(with: sessionMock, cache: cache) + + let expectation = XCTestExpectation(description: "First image fetch should complete.") + + // When + // Start two simultaneous fetches + let fetchTask1 = Task { try await service.fetchImage(with: imageURL, forceRefresh: false) } + let fetchTask2 = Task { try await service.fetchImage(with: imageURL, forceRefresh: false) } + + // Then + let result1 = try await fetchTask1.value + let result2 = try await fetchTask2.value + + expectation.fulfill() + await fulfillment(of: [expectation], timeout: 0.5) + + // Assert that both fetches return the same image + XCTAssertEqual(result1.image.pngData(), mockImageData) + XCTAssertEqual(result2.image.pngData(), mockImageData) + + // Assert that the image was returned twice + XCTAssertEqual(cache.messageCount(type: .get), 2) + + // Assert that only one fetch set an `.inProgress` CacheEntry + XCTAssertEqual(cache.messageCount(type: .inProgress), 1) + + // Assert that only one fetch set an `.ready` CacheEntry + XCTAssertEqual(cache.messageCount(type: .ready), 1) + } } private func imageDownloadService(with session: URLSessionProtocol, cache: ImageCaching? = nil) -> ImageDownloadService { From 32af7842bc4d94070a4bb74d30a191acf1306e52 Mon Sep 17 00:00:00 2001 From: Pinar Olguc Date: Thu, 26 Sep 2024 17:55:24 +0300 Subject: [PATCH 03/18] Omit image download cancel method --- ...moUIImageViewExtensionViewController.swift | 9 ++++---- .../Services/ImageDownloadService.swift | 21 ++++++------------- .../Network/Services/ImageDownloader.swift | 4 ---- .../UIImageView+Gravatar.swift | 6 ------ 4 files changed, 10 insertions(+), 30 deletions(-) diff --git a/Demo/Demo/Gravatar-UIKit-Demo/DemoUIImageViewExtensionViewController.swift b/Demo/Demo/Gravatar-UIKit-Demo/DemoUIImageViewExtensionViewController.swift index 7a63bc21..9ff81a84 100644 --- a/Demo/Demo/Gravatar-UIKit-Demo/DemoUIImageViewExtensionViewController.swift +++ b/Demo/Demo/Gravatar-UIKit-Demo/DemoUIImageViewExtensionViewController.swift @@ -18,6 +18,7 @@ class DemoUIImageViewExtensionViewController: UIViewController { textField.placeholder = "Enter Gravatar email" textField.autocapitalizationType = .none textField.keyboardType = .emailAddress + textField.text = "p8914704@gmail.com" return textField }() @@ -135,11 +136,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 +152,6 @@ class DemoUIImageViewExtensionViewController: UIViewController { print("success!") print("result url: \(result.sourceURL)") print("retrived Image point size: \(result.image.size)") - } catch { print(error) } @@ -158,9 +159,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..4e11abd0 100644 --- a/Sources/Gravatar/Network/Services/ImageDownloadService.swift +++ b/Sources/Gravatar/Network/Services/ImageDownloadService.swift @@ -34,7 +34,9 @@ public struct ImageDownloadService: ImageDownloader, Sendable { let task = Task { try await fetchAndProcessImage(request: request, processor: processingMethod.processor) } + try Task.checkCancellation() let image = try await awaitAndCacheImage(from: task, cacheKey: url.absoluteString) + try Task.checkCancellation() return ImageDownloadResult(image: image, sourceURL: url) } @@ -77,25 +79,14 @@ 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 { fileprivate static func imageRequest(url: URL, forceRefresh: Bool) -> URLRequest { - var request = forceRefresh ? URLRequest(url: url, cachePolicy: .reloadIgnoringLocalCacheData) : URLRequest(url: url) + var request = forceRefresh ? URLRequest(url: url, cachePolicy: .reloadIgnoringLocalAndRemoteCacheData) : URLRequest(url: url) + if forceRefresh { + request.setValue("no-cache, no-store, max-age=0", forHTTPHeaderField: "Cache-Control") + } request.httpShouldHandleCookies = false request.addValue("image/*", forHTTPHeaderField: "Accept") return request 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: From 40efb43cee765f7dde4646e8b2e5c36d55aa1ff2 Mon Sep 17 00:00:00 2001 From: Pinar Olguc Date: Thu, 26 Sep 2024 17:56:12 +0300 Subject: [PATCH 04/18] Make ImageDownloadService an actor --- Sources/Gravatar/Network/Services/ImageDownloadService.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Gravatar/Network/Services/ImageDownloadService.swift b/Sources/Gravatar/Network/Services/ImageDownloadService.swift index 4e11abd0..978bdbb8 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 From 4a31575e1969b7207d2740678c8e87bd95656455 Mon Sep 17 00:00:00 2001 From: Pinar Olguc Date: Thu, 26 Sep 2024 17:58:33 +0300 Subject: [PATCH 05/18] Revert some lines --- .../DemoUIImageViewExtensionViewController.swift | 1 - Sources/Gravatar/Network/Services/ImageDownloadService.swift | 5 +---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/Demo/Demo/Gravatar-UIKit-Demo/DemoUIImageViewExtensionViewController.swift b/Demo/Demo/Gravatar-UIKit-Demo/DemoUIImageViewExtensionViewController.swift index 9ff81a84..f0fe75f6 100644 --- a/Demo/Demo/Gravatar-UIKit-Demo/DemoUIImageViewExtensionViewController.swift +++ b/Demo/Demo/Gravatar-UIKit-Demo/DemoUIImageViewExtensionViewController.swift @@ -18,7 +18,6 @@ class DemoUIImageViewExtensionViewController: UIViewController { textField.placeholder = "Enter Gravatar email" textField.autocapitalizationType = .none textField.keyboardType = .emailAddress - textField.text = "p8914704@gmail.com" return textField }() diff --git a/Sources/Gravatar/Network/Services/ImageDownloadService.swift b/Sources/Gravatar/Network/Services/ImageDownloadService.swift index 978bdbb8..d946d01e 100644 --- a/Sources/Gravatar/Network/Services/ImageDownloadService.swift +++ b/Sources/Gravatar/Network/Services/ImageDownloadService.swift @@ -83,10 +83,7 @@ public actor ImageDownloadService: ImageDownloader, Sendable { extension URLRequest { fileprivate static func imageRequest(url: URL, forceRefresh: Bool) -> URLRequest { - var request = forceRefresh ? URLRequest(url: url, cachePolicy: .reloadIgnoringLocalAndRemoteCacheData) : URLRequest(url: url) - if forceRefresh { - request.setValue("no-cache, no-store, max-age=0", forHTTPHeaderField: "Cache-Control") - } + var request = forceRefresh ? URLRequest(url: url, cachePolicy: .reloadIgnoringLocalCacheData) : URLRequest(url: url) request.httpShouldHandleCookies = false request.addValue("image/*", forHTTPHeaderField: "Accept") return request From b0ae9648b803aeb3b1f6d22eb1f46954cb9a8b2a Mon Sep 17 00:00:00 2001 From: Andrew Montgomery Date: Thu, 26 Sep 2024 12:59:50 -0500 Subject: [PATCH 06/18] Remove unit tests related to canceling tasks --- .../ImageDownloadServiceTests.swift | 62 ------------------- .../GravatarWrapper+UIImageViewTests.swift | 34 ---------- 2 files changed, 96 deletions(-) diff --git a/Tests/GravatarTests/ImageDownloadServiceTests.swift b/Tests/GravatarTests/ImageDownloadServiceTests.swift index 4801bf75..11af7b3a 100644 --- a/Tests/GravatarTests/ImageDownloadServiceTests.swift +++ b/Tests/GravatarTests/ImageDownloadServiceTests.swift @@ -15,68 +15,6 @@ final class ImageDownloadServiceTests: XCTestCase { XCTAssertNotNil(imageResponse.image) } - func testFetchImageCancel() async throws { - 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 cache = TestImageCache() - 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) - 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) - } - - await task1.value - try await task2.value - } - - func testCallAfterAFailedCallWorksFine() async throws { - 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 service = imageDownloadService(with: sessionMock) - - 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() - } - } - - 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. - await sessionMock.update(isCancellable: false) - let result = try await service.fetchImage(with: imageURL) - XCTAssertNotNil(result.image) - } - func testImageProcessingError() async throws { let imageURL = try XCTUnwrap(URL(string: "https://gravatar.com/avatar/HASH")) let response = HTTPURLResponse.successResponse(with: imageURL) 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) From 46dd7e9d700df2d52aab5195579b88275deeb716 Mon Sep 17 00:00:00 2001 From: Andrew Montgomery Date: Thu, 26 Sep 2024 15:41:19 -0500 Subject: [PATCH 07/18] Add more simultaneous fetches, update comments --- .../ImageDownloadServiceTests.swift | 28 +++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/Tests/GravatarTests/ImageDownloadServiceTests.swift b/Tests/GravatarTests/ImageDownloadServiceTests.swift index 11af7b3a..c6455b4d 100644 --- a/Tests/GravatarTests/ImageDownloadServiceTests.swift +++ b/Tests/GravatarTests/ImageDownloadServiceTests.swift @@ -58,33 +58,45 @@ final class ImageDownloadServiceTests: XCTestCase { func testSimultaneousFetchShouldOnlyTriggerOneNetworkRequest() async throws { let imageURL = URL(string: "https://example.com/image.png")! - let response = HTTPURLResponse.successResponse(with: imageURL) - let mockImageData = UIImage(systemName: "iphone.gen2")!.pngData()! - let sessionMock = URLSessionMock(returnData: mockImageData, response: response) + + let sessionMock = URLSessionMock( + returnData: mockImageData, + response: HTTPURLResponse.successResponse(with: imageURL) + ) + let cache = TestImageCache() let service = imageDownloadService(with: sessionMock, cache: cache) - let expectation = XCTestExpectation(description: "First image fetch should complete.") + let expectation = XCTestExpectation(description: "Image fetches should complete") // When - // Start two simultaneous fetches + // 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 expectation.fulfill() await fulfillment(of: [expectation], timeout: 0.5) - // Assert that both fetches return the same image + // 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 the image was returned twice - XCTAssertEqual(cache.messageCount(type: .get), 2) + // 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), 1) From 2343cb040555df8f42021a80c66a811d7018a444 Mon Sep 17 00:00:00 2001 From: Andrew Montgomery Date: Thu, 26 Sep 2024 15:42:51 -0500 Subject: [PATCH 08/18] Move .inProgress so that it is called before a suspension point --- .../Gravatar/Network/Services/ImageDownloadService.swift | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Sources/Gravatar/Network/Services/ImageDownloadService.swift b/Sources/Gravatar/Network/Services/ImageDownloadService.swift index d946d01e..28e5fd11 100644 --- a/Sources/Gravatar/Network/Services/ImageDownloadService.swift +++ b/Sources/Gravatar/Network/Services/ImageDownloadService.swift @@ -35,7 +35,10 @@ public actor ImageDownloadService: ImageDownloader, Sendable { try await fetchAndProcessImage(request: request, processor: processingMethod.processor) } try Task.checkCancellation() - 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) } @@ -52,7 +55,6 @@ public actor 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 From c44dc0d570a0f740c08d82695d27783bf47c24b6 Mon Sep 17 00:00:00 2001 From: Andrew Montgomery Date: Thu, 26 Sep 2024 15:43:16 -0500 Subject: [PATCH 09/18] Add some formatting --- Sources/Gravatar/Network/Services/ImageDownloadService.swift | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Sources/Gravatar/Network/Services/ImageDownloadService.swift b/Sources/Gravatar/Network/Services/ImageDownloadService.swift index 28e5fd11..1b684a2a 100644 --- a/Sources/Gravatar/Network/Services/ImageDownloadService.swift +++ b/Sources/Gravatar/Network/Services/ImageDownloadService.swift @@ -31,13 +31,17 @@ public actor ImageDownloadService: ImageDownloader, Sendable { if !forceRefresh, let image = try await cachedImage(for: url) { return ImageDownloadResult(image: image, sourceURL: url) } + let task = Task { try await fetchAndProcessImage(request: request, processor: processingMethod.processor) } + try Task.checkCancellation() + // 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) From 43c8af06588e222a6c001edba7c3243a1a3a2f3b Mon Sep 17 00:00:00 2001 From: Andrew Montgomery Date: Thu, 26 Sep 2024 21:17:22 -0500 Subject: [PATCH 10/18] Convert TestImageCache properties to computed properties --- Sources/TestHelpers/TestImageCache.swift | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/Sources/TestHelpers/TestImageCache.swift b/Sources/TestHelpers/TestImageCache.swift index 54c1ee9b..46ef7d5f 100644 --- a/Sources/TestHelpers/TestImageCache.swift +++ b/Sources/TestHelpers/TestImageCache.swift @@ -14,9 +14,9 @@ package class TestImageCache: ImageCaching, @unchecked Sendable { case get } - package private(set) var getImageCallsCount = 0 - package private(set) var setImageCallsCount = 0 - package private(set) var setTaskCallsCount = 0 + 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") @@ -34,10 +34,8 @@ package class TestImageCache: ImageCaching, @unchecked Sendable { } switch entry { case .inProgress: - setTaskCallsCount += 1 message = (operation: .inProgress, key: key) case .ready: - setImageCallsCount += 1 message = (operation: .ready, key: key) } imageCache.setEntry(entry, for: key) @@ -46,7 +44,6 @@ package class TestImageCache: ImageCaching, @unchecked Sendable { package func getEntry(with key: String) -> Gravatar.CacheEntry? { accessQueue.sync { - getImageCallsCount += 1 cacheMessages.append(CacheMessage(operation: .get, key: key)) return imageCache.getEntry(with: key) } From b1c70ebf6cb50fa4941075317390a68f966b3313 Mon Sep 17 00:00:00 2001 From: Andrew Montgomery Date: Thu, 26 Sep 2024 21:22:03 -0500 Subject: [PATCH 11/18] Make Sendable class final --- Sources/TestHelpers/TestImageCache.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/TestHelpers/TestImageCache.swift b/Sources/TestHelpers/TestImageCache.swift index 46ef7d5f..0b0f2317 100644 --- a/Sources/TestHelpers/TestImageCache.swift +++ b/Sources/TestHelpers/TestImageCache.swift @@ -1,7 +1,7 @@ import Gravatar import UIKit -package class TestImageCache: ImageCaching, @unchecked Sendable { +package final class TestImageCache: ImageCaching, @unchecked Sendable { let imageCache = ImageCache() package typealias CacheMessage = (operation: CacheMessageType, key: String) From 1a1539ced77cb010cc4efeb4de62812e31811607 Mon Sep 17 00:00:00 2001 From: Andrew Montgomery Date: Thu, 26 Sep 2024 21:25:11 -0500 Subject: [PATCH 12/18] Add support for filtering cache messages by cache key --- Sources/TestHelpers/TestImageCache.swift | 6 ++++++ Tests/GravatarTests/ImageDownloadServiceTests.swift | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Sources/TestHelpers/TestImageCache.swift b/Sources/TestHelpers/TestImageCache.swift index 0b0f2317..1348d570 100644 --- a/Sources/TestHelpers/TestImageCache.swift +++ b/Sources/TestHelpers/TestImageCache.swift @@ -54,4 +54,10 @@ package final class TestImageCache: ImageCaching, @unchecked Sendable { 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/Tests/GravatarTests/ImageDownloadServiceTests.swift b/Tests/GravatarTests/ImageDownloadServiceTests.swift index c6455b4d..fb845ae5 100644 --- a/Tests/GravatarTests/ImageDownloadServiceTests.swift +++ b/Tests/GravatarTests/ImageDownloadServiceTests.swift @@ -99,10 +99,10 @@ final class ImageDownloadServiceTests: XCTestCase { XCTAssertEqual(cache.messageCount(type: .get), 5) // Assert that only one fetch set an `.inProgress` CacheEntry - XCTAssertEqual(cache.messageCount(type: .inProgress), 1) + XCTAssertEqual(cache.messageCount(type: .inProgress, forKey: imageURL.absoluteString), 1) // Assert that only one fetch set an `.ready` CacheEntry - XCTAssertEqual(cache.messageCount(type: .ready), 1) + XCTAssertEqual(cache.messageCount(type: .ready, forKey: imageURL.absoluteString), 1) } } From 05bd7e7eb232ee5cbff54aefe7b8768dab27335d Mon Sep 17 00:00:00 2001 From: Andrew Montgomery Date: Thu, 26 Sep 2024 21:37:23 -0500 Subject: [PATCH 13/18] Add unit test to test multiple URL's multiple times, concurrently --- .../ImageDownloadServiceTests.swift | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/Tests/GravatarTests/ImageDownloadServiceTests.swift b/Tests/GravatarTests/ImageDownloadServiceTests.swift index fb845ae5..91d93858 100644 --- a/Tests/GravatarTests/ImageDownloadServiceTests.swift +++ b/Tests/GravatarTests/ImageDownloadServiceTests.swift @@ -104,6 +104,90 @@ final class ImageDownloadServiceTests: XCTestCase { // 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 { From e534d87d090e61e48d1fcc20cf9c193533c89cc0 Mon Sep 17 00:00:00 2001 From: Andrew Montgomery Date: Fri, 27 Sep 2024 09:16:13 -0500 Subject: [PATCH 14/18] Add delay to download tasks --- Tests/GravatarTests/ImageDownloadServiceTests.swift | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Tests/GravatarTests/ImageDownloadServiceTests.swift b/Tests/GravatarTests/ImageDownloadServiceTests.swift index 91d93858..bf703830 100644 --- a/Tests/GravatarTests/ImageDownloadServiceTests.swift +++ b/Tests/GravatarTests/ImageDownloadServiceTests.swift @@ -65,6 +65,9 @@ final class ImageDownloadServiceTests: XCTestCase { 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) From 78334c0296f1a34e4e80340c2164fa538aab1e36 Mon Sep 17 00:00:00 2001 From: Pinar Olguc Date: Fri, 27 Sep 2024 19:12:54 +0300 Subject: [PATCH 15/18] Additional tests about cancellation --- .../Services/ImageDownloadService.swift | 6 ++- Sources/TestHelpers/URLSessionMock.swift | 6 ++- .../ImageDownloadServiceTests.swift | 53 +++++++++++++++++++ 3 files changed, 62 insertions(+), 3 deletions(-) diff --git a/Sources/Gravatar/Network/Services/ImageDownloadService.swift b/Sources/Gravatar/Network/Services/ImageDownloadService.swift index 1b684a2a..37f350a6 100644 --- a/Sources/Gravatar/Network/Services/ImageDownloadService.swift +++ b/Sources/Gravatar/Network/Services/ImageDownloadService.swift @@ -29,6 +29,7 @@ public actor 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) } @@ -36,8 +37,6 @@ public actor ImageDownloadService: ImageDownloader, Sendable { try await fetchAndProcessImage(request: request, processor: processingMethod.processor) } - try Task.checkCancellation() - // Create `.inProgress` entry before we await to prevent re-entrancy issues let cacheKey = url.absoluteString imageCache.setEntry(.inProgress(task), for: cacheKey) @@ -52,6 +51,7 @@ public actor 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 @@ -62,6 +62,7 @@ public actor ImageDownloadService: ImageDownloader, Sendable { let image: UIImage do { image = try await task.value + try Task.checkCancellation() } catch { imageCache.setEntry(nil, for: key) throw error @@ -73,6 +74,7 @@ public actor 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 } 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 bf703830..86e42a30 100644 --- a/Tests/GravatarTests/ImageDownloadServiceTests.swift +++ b/Tests/GravatarTests/ImageDownloadServiceTests.swift @@ -55,6 +55,59 @@ final class ImageDownloadServiceTests: XCTestCase { XCTAssertNotNil(imageResponse.image) } + func testFetchImageCancel() async throws { + 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 cache = TestImageCache() + let service = imageDownloadService(with: sessionMock, cache: cache) + + let task1 = Task { + do { + let _ = try await service.fetchImage(with: imageURL) + XCTFail() + } catch { + XCTAssertNotNil(error as? CancellationError) + let entry = cache.getEntry(with: imageURL.absoluteString) + XCTAssertNil(entry) + } + } + + try await Task.sleep(nanoseconds: UInt64(0.05 * 1_000_000_000)) + task1.cancel() + + await task1.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, error: NSError(domain: "test", code: 1)) + await sessionMock.update(isCancellable: true) + let service = imageDownloadService(with: sessionMock, cache: cache) + let task1 = Task { + do { + let _ = try await service.fetchImage(with: imageURL) + XCTFail() + } catch { + XCTAssertNotNil(error) + let entry = cache.getEntry(with: imageURL.absoluteString) + XCTAssertNil(entry) + } + } + + await task1.value + + // The task is cancelled, 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) + } + func testSimultaneousFetchShouldOnlyTriggerOneNetworkRequest() async throws { let imageURL = URL(string: "https://example.com/image.png")! From 3762e3ae3c798d569b3ce3e7dabb8e02b3668af4 Mon Sep 17 00:00:00 2001 From: Pinar Olguc Date: Fri, 27 Sep 2024 19:21:17 +0300 Subject: [PATCH 16/18] Fix inline comment --- Tests/GravatarTests/ImageDownloadServiceTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/GravatarTests/ImageDownloadServiceTests.swift b/Tests/GravatarTests/ImageDownloadServiceTests.swift index 86e42a30..5c4eb8d6 100644 --- a/Tests/GravatarTests/ImageDownloadServiceTests.swift +++ b/Tests/GravatarTests/ImageDownloadServiceTests.swift @@ -101,7 +101,7 @@ final class ImageDownloadServiceTests: XCTestCase { await task1.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) From 0c95518654436f8561be8f0795615fe3fd00a54e Mon Sep 17 00:00:00 2001 From: Andrew Montgomery Date: Fri, 27 Sep 2024 12:35:26 -0500 Subject: [PATCH 17/18] Move task cancelation tests up to where they were before --- .../ImageDownloadServiceTests.swift | 80 +++++++++---------- 1 file changed, 40 insertions(+), 40 deletions(-) diff --git a/Tests/GravatarTests/ImageDownloadServiceTests.swift b/Tests/GravatarTests/ImageDownloadServiceTests.swift index 5c4eb8d6..e74b4c43 100644 --- a/Tests/GravatarTests/ImageDownloadServiceTests.swift +++ b/Tests/GravatarTests/ImageDownloadServiceTests.swift @@ -15,46 +15,6 @@ final class ImageDownloadServiceTests: XCTestCase { XCTAssertNotNil(imageResponse.image) } - func testImageProcessingError() async throws { - 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 cache = TestImageCache() - let service = imageDownloadService(with: sessionMock, cache: cache) - - do { - _ = try await service.fetchImage(with: imageURL, processingMethod: .custom(processor: FailingImageProcessor())) - XCTFail() - } catch ImageFetchingError.imageProcessorFailed { - // success - } catch { - XCTFail() - } - } - - func testFetchCatchedImageWithURL() async throws { - let imageURL = "https://gravatar.com/avatar/HASH" - let response = HTTPURLResponse.successResponse(with: URL(string: imageURL)!) - let sessionMock = URLSessionMock(returnData: ImageHelper.testImageData, response: response) - let cache = TestImageCache() - let service = imageDownloadService(with: sessionMock, cache: cache) - - _ = try await service.fetchImage(with: URL(string: imageURL)!) - _ = try await service.fetchImage(with: URL(string: imageURL)!) - let imageResponse = try await service.fetchImage(with: URL(string: imageURL)!) - let setImageCallsCount = cache.setImageCallsCount - let setTaskCallCount = cache.setTaskCallsCount - let getImageCallsCount = cache.getImageCallsCount - let request = await sessionMock.request - let callsCount = await sessionMock.callsCount - XCTAssertEqual(setImageCallsCount, 1) - XCTAssertEqual(setTaskCallCount, 1) - XCTAssertEqual(getImageCallsCount, 3) - XCTAssertEqual(callsCount, 1) - XCTAssertEqual(request?.url?.absoluteString, "https://gravatar.com/avatar/HASH") - XCTAssertNotNil(imageResponse.image) - } - func testFetchImageCancel() async throws { let imageURL = try XCTUnwrap(URL(string: "https://gravatar.com/avatar/HASH")) let response = HTTPURLResponse.successResponse(with: imageURL) @@ -108,6 +68,46 @@ final class ImageDownloadServiceTests: XCTestCase { XCTAssertNotNil(result.image) } + func testImageProcessingError() async throws { + 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 cache = TestImageCache() + let service = imageDownloadService(with: sessionMock, cache: cache) + + do { + _ = try await service.fetchImage(with: imageURL, processingMethod: .custom(processor: FailingImageProcessor())) + XCTFail() + } catch ImageFetchingError.imageProcessorFailed { + // success + } catch { + XCTFail() + } + } + + func testFetchCatchedImageWithURL() async throws { + let imageURL = "https://gravatar.com/avatar/HASH" + let response = HTTPURLResponse.successResponse(with: URL(string: imageURL)!) + let sessionMock = URLSessionMock(returnData: ImageHelper.testImageData, response: response) + let cache = TestImageCache() + let service = imageDownloadService(with: sessionMock, cache: cache) + + _ = try await service.fetchImage(with: URL(string: imageURL)!) + _ = try await service.fetchImage(with: URL(string: imageURL)!) + let imageResponse = try await service.fetchImage(with: URL(string: imageURL)!) + let setImageCallsCount = cache.setImageCallsCount + let setTaskCallCount = cache.setTaskCallsCount + let getImageCallsCount = cache.getImageCallsCount + let request = await sessionMock.request + let callsCount = await sessionMock.callsCount + XCTAssertEqual(setImageCallsCount, 1) + XCTAssertEqual(setTaskCallCount, 1) + XCTAssertEqual(getImageCallsCount, 3) + XCTAssertEqual(callsCount, 1) + 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")! From 6380bdeec92b4113240078748cd3750acd04d6b8 Mon Sep 17 00:00:00 2001 From: Andrew Montgomery Date: Fri, 27 Sep 2024 14:34:59 -0500 Subject: [PATCH 18/18] Remove unnecessary expectation --- Tests/GravatarTests/ImageDownloadServiceTests.swift | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Tests/GravatarTests/ImageDownloadServiceTests.swift b/Tests/GravatarTests/ImageDownloadServiceTests.swift index e74b4c43..e360fbe0 100644 --- a/Tests/GravatarTests/ImageDownloadServiceTests.swift +++ b/Tests/GravatarTests/ImageDownloadServiceTests.swift @@ -124,8 +124,6 @@ final class ImageDownloadServiceTests: XCTestCase { 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: imageURL, forceRefresh: false) } @@ -141,9 +139,6 @@ final class ImageDownloadServiceTests: XCTestCase { let result4 = try await fetchTask4.value let result5 = try await fetchTask5.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)