Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify reusing http request Tasks in ImageDownloader #23817

Closed
wants to merge 5 commits into from
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 13 additions & 44 deletions WordPress/Classes/Utility/Media/ImageDownloader.swift
Original file line number Diff line number Diff line change
@@ -30,7 +30,7 @@ actor ImageDownloader {
)
}

private var tasks: [String: ImageDataTask] = [:]
private var tasks: [String: Task<Data, any Error>] = [:]

init(cache: MemoryCacheProtocol = MemoryCache.shared) {
self.cache = cache
@@ -115,32 +115,22 @@ actor ImageDownloader {

private func data(for request: URLRequest, options: ImageRequestOptions) async throws -> Data {
let requestKey = request.urlRequest?.url?.absoluteString ?? ""
let task = tasks[requestKey] ?? ImageDataTask(key: requestKey, Task {
try await self._data(for: request, options: options, key: requestKey)
})
task.downloader = self

let subscriptionID = UUID()
task.subscriptions.insert(subscriptionID)
tasks[requestKey] = task

return try await task.getData(subscriptionID: subscriptionID)
}

fileprivate nonisolated func unsubscribe(_ subscriptionID: UUID, key: String) {
Task {
await _unsubscribe(subscriptionID, key: key)
let theTask: Task<Data, any Error>
if let task = tasks[requestKey] {
theTask = task
} else {
theTask = Task {
try await self._data(for: request, options: options, key: requestKey)
}
tasks[requestKey] = theTask
}
}

private func _unsubscribe(_ subscriptionID: UUID, key: String) {
guard let task = tasks[key],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version implements coalescing of tasks for the same URLs, which is especially useful for prefetching. If there is an outstanding task, it adds a subscriber to the existing task. The task gets canceled only when there are no subscribers left. It would ideally also reuse decompression/resizing, but it's a bit more challenging to implement, especially if you want to reuse these separate from the downloads.

Background: ImageDownloader is an existing class that I updated a while ago by adding background decompression, resizing, coalescing, and stuff like that. I also removed a few other systems. The downloader is currently the primary API for fetching images.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, of course. I don't know what I was thinking... The HTTP task is only canceled if all subscribers are canceled.

task.subscriptions.remove(subscriptionID) != nil,
task.subscriptions.isEmpty else {
return
return try await withTaskCancellationHandler {
try await theTask.value
} onCancel: {
theTask.cancel()
}
task.task.cancel()
tasks[key] = nil
}

private func _data(for request: URLRequest, options: ImageRequestOptions, key: String) async throws -> Data {
@@ -161,27 +151,6 @@ actor ImageDownloader {
}
}

private final class ImageDataTask {
let key: String
var subscriptions = Set<UUID>()
let task: Task<Data, Error>
weak var downloader: ImageDownloader?

init(key: String, _ task: Task<Data, Error>) {
self.key = key
self.task = task
}

func getData(subscriptionID: UUID) async throws -> Data {
try await withTaskCancellationHandler {
try await task.value
} onCancel: { [weak self] in
guard let self else { return }
self.downloader?.unsubscribe(subscriptionID, key: self.key)
}
}
}

// MARK: - ImageDownloader (Closures)

extension ImageDownloader {
34 changes: 34 additions & 0 deletions WordPress/WordPressTest/ImageDownloaderTests.swift
Original file line number Diff line number Diff line change
@@ -125,6 +125,40 @@ class ImageDownloaderTests: CoreDataTestCase {
XCTAssertEqual(image.size, CGSize(width: 1024, height: 680))
}

func testReuseHTTPRequest() async throws {
// GIVEN
let httpRequestReceived = self.expectation(description: "HTTP request received")
let imageURL = try XCTUnwrap(URL(string: "https://example.files.wordpress.com/2023/09/image.jpg"))
stub(condition: { _ in true }, response: { _ in
httpRequestReceived.fulfill()

guard let sourceURL = try? XCTUnwrap(Bundle.test.url(forResource: "test-image", withExtension: "jpg")),
let data = try? Data(contentsOf: sourceURL) else {
return HTTPStubsResponse(error: URLError(.unknown))
}

return HTTPStubsResponse(data: data, statusCode: 200, headers: nil)
.responseTime(0.3)
})

// WHEN
let taskCompleted = self.expectation(description: "Image downloaded")
taskCompleted.expectedFulfillmentCount = 3
for _ in 1...3 {
try await Task.sleep(for: .milliseconds(50))
Task.detached {
do {
_ = try await self.sut.image(from: imageURL)
} catch {
XCTFail("Unexpected error: \(error)")
}
taskCompleted.fulfill()
}
}

await fulfillment(of: [httpRequestReceived, taskCompleted], timeout: 0.5)
}

// MARK: - Helpers

/// `Media` is hardcoded to work with a specific direcoty URL managed by `MediaFileManager`