Simplify reusing http request Tasks in ImageDownloader

[crazytonyli]

@kean I am not sure if the special subscribe and unsubscribe code in ImageDownloader is necessary. I don't see any issue in removing that special code and using Task directly (await task.value to subscribe and task.cancel to unsubscribe).

I'm not sure if I have missed anything. Let me know what you think.

Regression Notes

1. Potential unintended areas of impact

2. What I did to test those areas of impact (or what existing automated tests I relied on)

3. What automated tests I added (or what prevented me from doing so)

PR submission checklist:

• I have completed the Regression Notes.
• I have considered adding unit tests for my changes.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

• WordPress.com sites and self-hosted Jetpack sites.
• Portrait and landscape orientations.
• Light and dark modes.
• Fonts: Larger, smaller and bold text.
• High contrast.
• VoiceOver.
• Languages with large words or with letters/accents not frequently used in English.
• Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
• iPhone and iPad.
• Multi-tasking: Split view and Slide over. (iPad)

[dangermattic]

	1 Warning
⚠️	This PR is assigned to the milestone 25.6. The due date for this milestone has already passed. Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr23817-dcf9997
	Version	25.4.2
	Bundle ID	org.wordpress.alpha
	Commit	dcf9997
	App Center Build	WPiOS - One-Offs #11116

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr23817-dcf9997
	Version	25.4.2
	Bundle ID	com.jetpack.alpha
	Commit	dcf9997
	App Center Build	jetpack-installable-builds #10156

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[kean]

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.

[crazytonyli]

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

[crazytonyli]

@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?

[kean]

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.

[crazytonyli]

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?

[kean]

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.

[crazytonyli]

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?

[kean]

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.

[crazytonyli]

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.

[wpmobilebot]

Version 25.6 has now entered code-freeze, so the milestone of this PR has been updated to 25.7.