Make ImageDataProvider loads cancellable#2517
Make ImageDataProvider loads cancellable#2517friday-refined wants to merge 1 commit intoonevcat:masterfrom
Conversation
Before this change, the `DownloadTask` returned by `KingfisherManager.retrieveImage(with: .provider(...))` was always `nil`, and `.dataProviding` cases of `DownloadTask.WrappedTask.cancel()` were no-ops. Once a provider call had been kicked off, there was no way to stop the success callback from firing \u2014 `imageView.kf.cancelDownloadTask()` silently did nothing on provider sources. This commit threads a lightweight cancellation token through the provider load: - `WrappedTask.dataProviding` carries an optional `DataProvidingCancelToken`. - `KingfisherManager.loadAndCacheImage` creates a token and passes it through to `provideImage`. - `provideImage` checks the token before dispatching into the processing queue and again before invoking the user completion handler. If the token is cancelled, it fires `.failure(.requestError(.dataProviderCancelled))` instead of `.success`, matching the behavior of cancelled network sources. - `DownloadTask` gains an internal initializer that wraps a provider cancel token so `WrappedTask.value` can vend a real, cancellable `DownloadTask` up to callers. - Adds a new error case `RequestErrorReason.dataProviderCancelled(provider:)` with error code 1006. The public `ImageDataProvider` protocol is unchanged. Providers are not required to be interruptible; the provider's own `data(handler:)` call may still complete in the background \u2014 this fix just ensures Kingfisher stops propagating that result once the caller has asked to cancel. Adds a new test suite (`ImageDataProviderCancellationTests`) covering: - the baseline non-cancel path, - direct cancellation through `manager.retrieveImage(...).cancel()`, - and cancellation through `imageView.kf.cancelDownloadTask()`. Verified: - macOS (macOS 26.4 SDK): 307 / 307 tests pass. - iOS 26.3.1 Simulator (iPhone 17 Pro): 324 / 324 tests pass. Fixes onevcat#2511 Signed-off-by: Friday <friday@yuusann.com>
|
There was a problem hiding this comment.
@enums @friday-refined I reviewed this change end-to-end from an independent reviewer perspective. Conclusion: the implementation direction is correct, the boundaries are clear, and the test coverage is solid — this is good to merge.
I focused on three areas:
- Whether cancellation semantics are fully wired through
- The
.providerpath now creates aDataProvidingCancelTokenand exposes a cancellableDownloadTaskviaWrappedTask. DownloadTask.cancel()now actually triggers cancellation for the provider branch instead of being a no-op.
- Whether callback behavior is aligned with network cancellation
provideImageperforms cancellation checks both after provider callback and before entering the processing queue.- Once cancelled, it consistently returns
.requestError(.dataProviderCancelled)and does not continue bubbling success downstream.
- Regression risk and maintainability
- The public
ImageDataProviderprotocol is unchanged, so this is an incremental improvement. - The new error branch is a reasonable extension, and tests cover the three key paths: no cancel, direct cancel, and
imageView.kf.cancelDownloadTask().
I did not find any blocking issues. Recommend Approve.
|
This implementation only suppresses the success callback as far as I can tell. The cancel event should be propagated to the provider as well to cancel any ongoing processes. As it currently stands, you still have to keep a reference to the original provider if you want to cancel ongoing tasks. |
|
@FaizanDurrani Thanks for calling this out — you’re absolutely right to separate two concerns here. The current change makes provider-backed loads cancellable from the Kingfisher task chain (so callers receive That part is intentional: So after this PR:
I re-reviewed the implementation with this boundary in mind and did not find a blocking issue in the current approach. @enums From an independent reviewer perspective, I think this is good to merge as an additive fix for #2511, and we can discuss a protocol-level cancellable provider API separately if we want true source interruption as a follow-up. |
|
To summarize the tradeoff so @onevcat can make the call: The current PR wires cancellation through the Kingfisher task chain, but If we want Kingfisher to also interrupt the provider's internal work, it has to go through the protocol. Two options: Option 1 — Optional
|
|
An optional method on the protocol is what I originally had in mind, yes. Personally, I wouldn't really consider #2511 as fixed without it as you would still need to hold a reference to the provider to cancel any ongoing process. |
|
let's pause here. There is another solution on going. |
|
Thanks for the work on this one. You identified exactly where cancel was getting dropped (the After thinking about it more, I went with cooperative cancellation through Swift concurrency instead of a cancel token. Opened #2519 with that approach. Kept you as co-author on all three commits since the problem surface and tests came from your work here. Going to close this one in favor of #2519. Thanks again! |
Summary
Fixes #2511.
Before this change,
ImageDataProvider-backed loads were uncancellable on the Kingfisher side:KingfisherManager.retrieveImage(with: .provider(...))always returnednilfor theDownloadTask, so callers had nothing to cancel.DownloadTask.WrappedTask.cancel()treated the.dataProvidingcase as a no-op.imageView.kf.cancelDownloadTask()silently did nothing on provider sources — the provider'sdata(handler:)would still run to completion and the success callback would still fire.As the reporter pointed out, the only workaround today is to implement a custom
cancel()on the provider and hold a reference to it at every call site, which defeats the point ofcancelDownloadTask()and is easy to forget.Approach
Thread a lightweight cancellation token through the provider load, while leaving the public
ImageDataProviderprotocol untouched.DataProvidingCancelToken(simple lock +isCancelledflag).DownloadTask.WrappedTask.dataProvidingnow carries an optionalDataProvidingCancelToken, and itscancel()flips the token.KingfisherManager.loadAndCacheImagemints a token for the.providerbranch and forwards it toprovideImage.provideImagechecks the token before dispatching into the processing queue and again before invoking the user completion handler. If cancelled, it fires.failure(.requestError(.dataProviderCancelled))instead of.success, matching the behavior of cancelled network sources.DownloadTaskgains an internalinit(providerCancelToken:)soWrappedTask.valuecan vend a real, cancellableDownloadTaskup to callers;DownloadTask.cancel()prefers the provider token when present.RequestErrorReason.dataProviderCancelled(provider:)(error code 1006) alongside existing.taskCancelled/.livePhotoTaskCancelled.What this does not do
The
ImageDataProviderprotocol still doesn't require providers to be interruptible. A long-runningdata(handler:)call (e.g. a thumbnail generation or a custom network fetch) may still complete in the background after cancel. This PR only ensures Kingfisher stops propagating that result up the completion chain — the user-visible behavior (setImagecompletion,retrieveImagecompletion) becomes consistent with what happens for a cancelled.networksource.Providers that can be interrupted (custom network fetches, etc.) continue to be free to implement their own
cancel()on the side; this PR is additive and doesn't conflict with that pattern.Test
Adds
ImageDataProviderCancellationTestswith three cases, all of which fail onmasterand pass on this branch:testProviderDeliversWhenNotCancelled— baseline, non-cancel path still delivers.success.testCancellingProviderTaskDeliversCancelledError—manager.retrieveImage(...)now returns a non-nil task, and calling.cancel()delivers.failure(.requestError(.dataProviderCancelled)).testImageViewCancelDownloadTaskCancelsProvider— end-to-end throughimageView.kf.setImage(with: .provider(...))+imageView.kf.cancelDownloadTask(), which now correctly cancels.platform=macOS,arch=arm64API surface
ImageDataProviderprotocol, all publicKingfisherManager/ImageCache/DownloadTaskmethods and their signatures.KingfisherError.RequestErrorReason.dataProviderCancelled(provider:)— new enum case with its own error code (1006). Existingswitches are still valid because the enum wasn't@frozen; any non-exhaustive handlers just fall through to theirdefaultbranch.DataProvidingCancelToken,DownloadTask.init(providerCancelToken:), an associated value onWrappedTask.dataProviding.Happy to split this further or adjust naming / error code if preferred.