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.

Sign up for GitHub

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

Simplify reusing http request Tasks in ImageDownloader #23817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8afc35c
Simplify reusing http request Tasks in ImageDownloader
crazytonyli Nov 17, 2024
6f64397
Add an unit test to verify reusing HTTP requests
crazytonyli Nov 17, 2024
660030d
Add an unit test for cancelling concurrent downloads
crazytonyli Nov 18, 2024
4076c07
Revert "Simplify reusing http request Tasks in ImageDownloader"
crazytonyli Nov 18, 2024
dcf9997
Use Combine Publisher to fix a cancelling subscriber issue in ImageDo…
crazytonyli Nov 21, 2024
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
67 changes: 23 additions & 44 deletions WordPress/Classes/Utility/Media/ImageDownloader.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import UIKit
import Combine

struct ImageRequestOptions {
/// Resize the thumbnail to the given size. By default, `nil`.
Expand Down Expand Up @@ -30,7 +31,7 @@ actor ImageDownloader {
)
}

private var tasks: [String: ImageDataTask] = [:]
private var tasks: [String: AnyPublisher<Result<Data, Error>, Never>] = [:]

init(cache: MemoryCacheProtocol = MemoryCache.shared) {
self.cache = cache
Expand Down Expand Up @@ -115,32 +116,31 @@ 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 task = tasks[requestKey] ?? {
let subject = CurrentValueSubject<Result<Data, Error>?, Never>(nil)
let t = Task {
do {
let data = try await self._data(for: request, options: options, key: requestKey)
subject.send(.success(data))
} catch {
subject.send(.failure(error))
}
subject.send(completion: .finished)
}

let subscriptionID = UUID()
task.subscriptions.insert(subscriptionID)
return subject
.handleEvents(receiveCancel: { t.cancel() })
.share()
.compactMap { $0 }
.eraseToAnyPublisher()
}()
tasks[requestKey] = task

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

fileprivate nonisolated func unsubscribe(_ subscriptionID: UUID, key: String) {
Task {
await _unsubscribe(subscriptionID, key: key)
}
}

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
if let result = await task.values.first(where: { _ in true }) {
return try result.get()
} else {
throw CancellationError()
}
task.task.cancel()
tasks[key] = nil
}

private func _data(for request: URLRequest, options: ImageRequestOptions, key: String) async throws -> Data {
Expand All @@ -161,27 +161,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 {
Expand Down
82 changes: 81 additions & 1 deletion WordPress/WordPressTest/ImageDownloaderTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,56 @@ class ImageDownloaderTests: CoreDataTestCase {
let _ = try await task.value
XCTFail()
} catch {
XCTAssertEqual((error as? URLError)?.code, .cancelled)
// XCTAssertEqual((error as? URLError)?.code, .cancelled)
XCTAssertTrue(error is CancellationError)
}
}

func testCancelOneOfManySubscribers() 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 there are concurrent calls to download the same image and one of those downloads is cancelled
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()
}
}

let taskCancelled = expectation(description: "Task is cancelled")
let taskToBeCancelled = Task.detached {
do {
_ = try await self.sut.image(from: imageURL)
XCTFail("Unexpected successful result.")
} catch {
taskCancelled.fulfill()
}
}
taskToBeCancelled.cancel()

await fulfillment(of: [httpRequestReceived, taskCompleted, taskCancelled], timeout: 0.5)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kean I added this new test case to verify cancelling one download task with other in-flight download tasks. I think this test case is legitimate and should pass. It doesn't pass though. Do you think that's a bug in the ImageDownloader?

Copy link
Contributor

@kean kean Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Task cancellation is cooperative. When you call cancel() nothing actually happens. In this case, there are still three more tasks subscribed to the same unit of work, so the download continues as normal and finishes. It works as designed.

What does happen is Task.isCancelled gets set to true. While you can never expect cancel() to actually cancel the underlying work in Swift Concurrency, you can reliably use Task.isCancelled from the same thread to stop the work that you control from happening. For example, you can use it to stop displaying the image in the case of an image view.

let taskToBeCancelled = Task.detached {
    do {
        _ = try await self.sut.image(from: imageURL)
        XCTAssertTrue(Task.isCancelled) // succeeds

.responseTime(0.3)

I'd suggest adding another expectation (somewhere in ImageDownloader) instead of relying on timing and unnecessity increasing the time the test takes to run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Task cancellation is cooperative. When you call cancel() nothing actually happens.

I believe the cancellation would be passed along to whatever "inner" task that's currently running? In this case, if you cancel taskToBeCancelled, this cancellation would be passed along to the await sut.image(for...) task, then the await data(for...), then the await URLSession.data.

Here is a test code:

import Foundation
import PlaygroundSupport

PlaygroundPage.current.needsIndefiniteExecution = true

func fetch() async throws -> String {
    _ = try await URLSession.shared.data(from: URL(string: "https://www.apple.com")!)
    return "Things"
}

func download() async throws -> String {
    try await fetch()
}

func use() async throws {
    _ = try await download()
}

let t = Task.detached {
    do {
        try await use()
        print("Success")
    } catch {
        print("Error: \(error)")
    }
}
t.cancel()

In this case, there are still three more tasks subscribed to the same unit of work, so the download continues as normal and finishes. It works as designed.

The test fails at XCTFail("Unexpected successful result."). The expectations set in the test case are: the three Task created first should finish successfully and the cancelled Task should fail with an error. If taskToBeCancelled is cancelled, then taskToBeCancelled should not return a successful result, shouldn't it?

Copy link
Contributor

@kean kean Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here is a test code:

This code throws the following error (URLError):

Error: Error Domain=NSURLErrorDomain Code=-999 "cancelled" UserInfo={NSErrorFailingURLStringKey=https://www.apple.com/, NSErrorFailingURLKey=https://www.apple.com/, _NSURLErrorRelatedURLSessionTaskErrorKey=(
    "LocalDataTask <338C255E-64EF-4162-B5EB-A0E72990C5DE>.<1>"
), _NSURLErrorFailingURLSessionTaskErrorKey=LocalDataTask <338C255E-64EF-4162-B5EB-A0E72990C5DE>.<1>, NSLocalizedDescription=cancelled}

It happens because the underlying URLSession task listens to cancellation and handles it in this way.

In the case of the ImageDownloader and the test testCancelOneOfManySubscribers, the download doesn't get cancelled (by design, due to coalescing). So, you do call cancel but nothing actually gets cancelled (again, by design).

If taskToBeCancelled is cancelled, then taskToBeCancelled should not return a successful result, shouldn't it?

I could argue both sides. The simplest implementation is to let the task finish and not throw an error. There is probably a way to update ImageDownloader to throw CancellationError if the task gets cancelled regardless of whether the underlying download gets cancelled or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could argue both sides. The simplest implementation is to let the task finish and not throw an error.

Hmm... I don't know. I'm not sure how it's okay for a cancelled task/operation to return a successful result. I can understand it if there is a race condition, where the cancellation happens right at the time when the task is completed. But this test case is not that.

Besides, the testCancellation test expects a cancelled task returns cancelled error.

I thought the whole point of keeping a subscriptions list is for deciding whether to cancel the HTTP request task or not. If it's okay to not cancel the HTTP request when all "awaiter Tasks" are cancelled, is subscriptions related code necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

This test testCancelOneOfManySubscribers cancels only one of four subscribers, so the download doesn't get cancelled. If you update it to cancel all four, which I did for testing, it cancels the download and finishes with URLError .cancelled. I'd suggest adding a test like that.

understand it if there is a race condition

This always is one because you typically cancel from main and the download is synchronized on a background thread. So if you want to reliably stop doing any work in the UI after calling cancel, you need to either check Task.isCancelled or do it in some other way and never rely on the error thrown from a task.

I don't think there are any simple ways of updating ImageDownloader to throw CancellationError or something like that only for tasks that get canceled that don't result in the cancellation of the underlying work. It doesn't really sound right also since cancel() doesn't do anything in that case – the task continues, and the image gets fetched. I think it's sound.

Copy link
Contributor Author

@crazytonyli crazytonyli Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This test testCancelOneOfManySubscribers cancels only one of four subscribers, so the download doesn't get cancelled.

No, the download task should not get cancelled. The XCTFail("Unexpected error: \(error)") assertions ensure that.

I don't think there are any simple ways of updating ImageDownloader to throw CancellationError or something like that only for tasks that get canceled that don't result in the cancellation of the underlying work.

dcf9997 (similar to #23823) should fix the issue. I'm not saying that's the only solution though.

Sorry I think I was confused about how the _subscriptions list is used. I don't think that's relevant in terms of cancelling subscriber tasks.


func testMemoryCache() async throws {
// GIVEN
let imageURL = try XCTUnwrap(URL(string: "https://example.files.wordpress.com/2023/09/image.jpg"))
Expand Down Expand Up @@ -125,6 +171,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`
Expand Down