Make provider loads cancellable via Swift concurrency#2519
Conversation
Add `data() async throws -> Data` as a requirement on `ImageDataProvider`, sitting next to the existing `data(handler:)`. Both come with default bridges, so existing providers compile unchanged and new providers can implement either one. Overriding the async method is how providers opt into cooperative cancellation. Also add `RequestErrorReason.dataProviderCancelled(provider:)` so a cancelled provider load can surface the same way a cancelled network source does today. Related to #2511. Co-authored-by: Friday <friday@yuusann.com>
Wrap the provider load in a `Task` that calls `provider.data()` and hop to the processing queue inside the same task context. Check `Task.isCancelled` at each suspension point; on cancel, deliver `.dataProviderCancelled` instead of letting a late `.success` leak through. Carry the task through `WrappedTask.dataProviding(_)` so that `DownloadTask.cancel()` and `imageView.kf.cancelDownloadTask()` cancel it. The cancel signal then propagates into provider implementations that honor Swift concurrency cancellation (`URLSession.data(for:)` and friends, `try Task.checkCancellation()`, `withTaskCancellationHandler`, etc.). Providers that only implement the callback-based `data(handler:)` fall back to the default async bridge. Their work still runs to completion in the background, but the post-await `Task.isCancelled` check intercepts the result so the caller only sees `.dataProviderCancelled`, matching how a cancelled network source behaves. Fixes #2511. Co-authored-by: Friday <friday@yuusann.com>
Five scenarios: - baseline: non-cancelled load delivers `.success` - direct `task.cancel()` on the returned `DownloadTask` delivers `.dataProviderCancelled` - `imageView.kf.cancelDownloadTask()` end-to-end through the extension path - a cooperative async provider (overrides `data() async throws`, polls `Task.checkCancellation()` / `Task.sleep`) stops mid-loop on cancel and exposes the cancel signal to its own implementation - a non-cooperative callback provider still runs its handler in the background after cancel, but the caller only sees `.dataProviderCancelled` Co-authored-by: Friday <friday@yuusann.com>
friday-refined
left a comment
There was a problem hiding this comment.
Thanks for picking this up and reframing it around concurrency — having the cancel signal propagate into the provider itself (instead of only suppressing the late .success like #2517 did) is clearly the better long-term shape. Reviewed the diff end-to-end against the failure mode in #2511; behavior, error semantics and tests all line up. A few notes below, mostly nits / heads-ups for maintainer follow-up.
What I like
- Protocol-level
data() async throwswith bi-directional default bridges keeps source compatibility for every existing provider while letting new ones opt into real cooperative cancellation. The doc note about not implementing neither (mutual recursion) is the right call; would be hard to catch otherwise. provideImageis now genuinely cancellable end-to-end: the post-awaitTask.isCancelledchecks before processing and before final delivery close the obvious races where data finished a hair beforecancel()arrived.- Error model: the new
dataProviderCancelled(provider:)mirrors the cancelled-network-source semantics, which is exactly what callers (incl.imageView.kf.cancelDownloadTask()) already expect. Distinguishing it fromtaskCancelledis useful for diagnostics. - Tests are the right ones. The cooperative-async case in particular is what I wanted to see: it proves the cancel signal reaches the implementation, not just the callback. The non-cooperative test pinning down the "late
.successis suppressed" contract is good regression insurance.
Minor things worth a second look
-
WrappedTask.valuereturns a freshDownloadTaskeach callcase .dataProviding(let task): guard let task else { return nil } return DownloadTask(providerTask: task)
Each access to
.valueallocates a newDownloadTaskwrapping the same underlyingTask. Cancellation still works (they all forward to the sameproviderTask.cancel()), but identity / equality of the returned task is no longer stable across calls. If anything downstream ever comparesDownloadTaskinstances (or stashes one and expects later lookups to match), it'll silently misbehave. Cheap fix is to memoize the wrapper inside the enum case, e.g. lazily create it once when the.dataProvidingcase is constructed and store it alongside theTask?. -
Error code table
dataProviderCancelledgets code1006. If the project keeps an external error-code reference (docs site / migration guide / public table), please double-check it gets the new entry — easy to miss. -
Docstring nit on the default
data(handler:)bridge/// Default callback-based entry point. Bridges to ``data()`` by spawning a `Task`. func data(handler: @escaping @Sendable (Result<Data, any Error>) -> Void) { Task { ... } }
Worth calling out explicitly that this
Taskis detached from any caller context and won't be cancelled by anything outside the closure — i.e. a pre-existing legacy provider that only implementeddata(handler:)and now relies on this default keeps the same "fire and forget" semantics it had before. Currently a careful reader can infer it from the "implement at least one" note, but stating it directly avoids surprise. -
asyncTaskContextCancelledvsdataProviderCancelledJust flagging for awareness: there are now two cancel-shaped reasons under
RequestErrorReasonplus the existingtaskCancelled/livePhotoTaskCancelled. The semantics are all distinct and the new one is well-scoped, so I'm not asking for consolidation — just worth a sentence in the changelog so downstreamswitchexhaustiveness updates are obvious to integrators.
Behavior parity check vs. #2517
For the original repro in #2511 (rapid setImage(with: .provider(...)) then cancel before the provider finishes):
- #2517 outcome: callback was suppressed via an internal cancel token, but the provider's own work still ran to completion in the background.
- #2519 outcome (this PR): same caller-visible behavior for callback-only providers (suppressed +
.dataProviderCancelled), plus providers that adoptdata() async throwsactually stop their underlying work.
That's a strict superset of what #2517 fixed, so I'm fully on board with closing #2517 in favor of this. Net result for users of the affected app pattern in #2511 is what they wanted.
Verdict
LGTM. The structural notes above (WrappedTask.value identity, error-code table, docstring clarification) are non-blocking — happy to see this go in as-is and follow up in a small cleanup PR if you'd rather not stack changes here.
Previously `WrappedTask.dataProviding` held the underlying `Task<Void, Never>` and `.value` wrapped a fresh `DownloadTask` around it on every access. That worked because `.value` is only read once per load in practice, but the identity of the returned `DownloadTask` was not stable across repeated reads and the shape of the enum case differed from `.download(DownloadTask)`. Create the `DownloadTask` once in `loadAndCacheImage` and carry it through the enum, so both cases of `WrappedTask` hold a `DownloadTask` and `.value` is a pure accessor. Behaviour is unchanged. Also document on the default `data(handler:)` bridge that the `Task` it opens is unstructured and does not inherit cancellation from any surrounding parent task. Kingfisher's own provider loads drive the async method directly so this is only informational for callers outside the framework.
|
@FaizanDurrani when you have a moment, would love a second pair of eyes on this. It's a different take on #2511 from what #2517 did. Rather than a cancel token, this adds Just want to make sure the shape actually fits the pattern you had in mind. |
|
Yep this looks good to me, originally I wanted an explicit cancel method on the provider so work cancellation could be handled by the provider while keeping existing api unchanged, something like: final class MyProvider: ImageDataProvider, @unchecked Sendable {
let myTask: Task<Void, Never>!
func data(handler: @escaping @Sendable (Result<Data, any Error>) -> Void) {
myTask = Task {
do {
// do some work
try Task.checkCancellation()
handler(.success(/* result */))
} catch {
handler(.failure(error))
}
}
}
func cancel() {
myTask.cancel()
}
}But this PR implementation is functionally the same, so I don't have any issues. |
Fixes #2511.
Provider-backed loads now run inside a
Task. Callingcancel()on the returnedDownloadTask(orimageView.kf.cancelDownloadTask()) cancels that task, and the completion fires with.dataProviderCancelledinstead of a late.success.ImageDataProvidergainsdata() async throws -> Datanext to the existingdata(handler:). Both come with default bridges, so existing providers compile unchanged. Providers that override the async one pick up Swift concurrency cancellation:URLSession.data(for:)stops,try Task.checkCancellation()works,withTaskCancellationHandlerworks.Providers that keep using the callback API will still finish their work in the background after a cancel. The caller no longer sees a late
.successthough, which matches how a cancelled network source behaves today.Tests
New
ImageDataProviderCancellationTestscovers:.success)task.cancel()→.dataProviderCancelledimageView.kf.cancelDownloadTask()→.dataProviderCancelledResults:
Credit
Built on top of @friday-refined's #2517, which pinned down exactly where the cancel signal was getting dropped (the
nilreturn, theWrappedTaskno-op, the imageView path). I went with a protocol-leveldata() async throwsrather than an internal cancel token so providers can stop their own work, not just have the callback suppressed.Closes #2517.