Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions Kingfisher.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
2F6023D252BE169414B01D8B /* CancellationToken.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1FAE088D96F2BF3FACCBD355 /* CancellationToken.swift */; };
388F37382B4D9CDB0089705C /* DisplayLink.swift in Sources */ = {isa = PBXBuildFile; fileRef = 388F37372B4D9CDB0089705C /* DisplayLink.swift */; };
38D5D3A32C5C757E00BF1D01 /* PixelFormatDecodingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 38D5D3A42C5C757E00BF1D01 /* PixelFormatDecodingTests.swift */; };
38D5D4A12C5C757E00BF1D01 /* ImageDataProviderCancellationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 38D5D4A22C5C757E00BF1D01 /* ImageDataProviderCancellationTests.swift */; };
38D5D3C12C5C7A1800BF1D01 /* gradient-8b-srgb-opaque.png in Resources */ = {isa = PBXBuildFile; fileRef = 38D5D3AC2C5C784700BF1D01 /* gradient-8b-srgb-opaque.png */; };
38D5D3C22C5C7A1800BF1D01 /* gradient-8b-srgb-alpha.png in Resources */ = {isa = PBXBuildFile; fileRef = 38D5D3AD2C5C784700BF1D01 /* gradient-8b-srgb-alpha.png */; };
38D5D3C32C5C7A1800BF1D01 /* gradient-8b-displayp3-alpha.png in Resources */ = {isa = PBXBuildFile; fileRef = 38D5D3AE2C5C784700BF1D01 /* gradient-8b-displayp3-alpha.png */; };
Expand Down Expand Up @@ -177,6 +178,7 @@
22FDCE0D2700078B0044D11E /* CPListItem+Kingfisher.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "CPListItem+Kingfisher.swift"; sourceTree = "<group>"; };
388F37372B4D9CDB0089705C /* DisplayLink.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DisplayLink.swift; sourceTree = "<group>"; };
38D5D3A42C5C757E00BF1D01 /* PixelFormatDecodingTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PixelFormatDecodingTests.swift; sourceTree = "<group>"; };
38D5D4A22C5C757E00BF1D01 /* ImageDataProviderCancellationTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ImageDataProviderCancellationTests.swift; sourceTree = "<group>"; };
38D5D3AC2C5C784700BF1D01 /* gradient-8b-srgb-opaque.png */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = "gradient-8b-srgb-opaque.png"; sourceTree = "<group>"; };
38D5D3AD2C5C784700BF1D01 /* gradient-8b-srgb-alpha.png */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = "gradient-8b-srgb-alpha.png"; sourceTree = "<group>"; };
38D5D3AE2C5C784700BF1D01 /* gradient-8b-displayp3-alpha.png */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = "gradient-8b-displayp3-alpha.png"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -533,6 +535,7 @@
D12E0C461C47F23500AC98AD /* ImageDownloaderTests.swift */,
D12E0C471C47F23500AC98AD /* ImageExtensionTests.swift */,
38D5D3A42C5C757E00BF1D01 /* PixelFormatDecodingTests.swift */,
38D5D4A22C5C757E00BF1D01 /* ImageDataProviderCancellationTests.swift */,
D186696C21834261002B502E /* ImageDrawingTests.swift */,
D9638BA41C7DC71F0046523D /* ImagePrefetcherTests.swift */,
D12E0C481C47F23500AC98AD /* ImageViewExtensionTests.swift */,
Expand Down Expand Up @@ -999,6 +1002,7 @@
D16FEA3F23078C63006E67D5 /* LSHTTPRequestDiff.m in Sources */,
D186696D21834261002B502E /* ImageDrawingTests.swift in Sources */,
38D5D3A32C5C757E00BF1D01 /* PixelFormatDecodingTests.swift in Sources */,
38D5D4A12C5C757E00BF1D01 /* ImageDataProviderCancellationTests.swift in Sources */,
D16FEA4323078C63006E67D5 /* NSURLRequest+DSL.m in Sources */,
D16FEA5323078C63006E67D5 /* LSStubResponseDSL.m in Sources */,
D16FEA4C23078C63006E67D5 /* LSDataMatcher.m in Sources */,
Expand Down
10 changes: 10 additions & 0 deletions Sources/General/KingfisherError.swift
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,13 @@ public enum KingfisherError: Error {
/// Error Code: 1004
case livePhotoTaskCancelled(source: LivePhotoSource)

/// The loading task of an ``ImageDataProvider`` is canceled by the user.
///
/// - Parameter provider: The image data provider whose load was canceled.
///
/// Error Code: 1006
case dataProviderCancelled(provider: any ImageDataProvider)

case asyncTaskContextCancelled
}

Expand Down Expand Up @@ -502,6 +509,8 @@ extension KingfisherError.RequestErrorReason {
return "The session task was cancelled. Task: \(task), cancel token: \(token)."
case .livePhotoTaskCancelled(let source):
return "The live photo download task was cancelled. Source: \(source)"
case .dataProviderCancelled(let provider):
return "The image data provider task was cancelled. Provider cache key: \(provider.cacheKey)"
case .asyncTaskContextCancelled:
return "The async task context was cancelled. This usually happens when the task is cancelled before it starts."
}
Expand All @@ -514,6 +523,7 @@ extension KingfisherError.RequestErrorReason {
case .taskCancelled: return 1003
case .livePhotoTaskCancelled: return 1004
case .asyncTaskContextCancelled: return 1005
case .dataProviderCancelled: return 1006
}
}
}
Expand Down
22 changes: 20 additions & 2 deletions Sources/General/KingfisherManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -469,13 +469,30 @@ public class KingfisherManager: @unchecked Sendable {
func provideImage(
provider: any ImageDataProvider,
options: KingfisherParsedOptionsInfo,
cancelToken: DataProvidingCancelToken? = nil,
completionHandler: (@Sendable (Result<ImageLoadingResult, KingfisherError>) -> Void)?)
{
guard let completionHandler = completionHandler else { return }
provider.data { result in
if let cancelToken, cancelToken.isCancelled {
options.callbackQueue.execute {
completionHandler(.failure(
KingfisherError.requestError(reason: .dataProviderCancelled(provider: provider))
))
}
return
}
switch result {
case .success(let data):
(options.processingQueue ?? self.processingQueue).execute {
if let cancelToken, cancelToken.isCancelled {
options.callbackQueue.execute {
completionHandler(.failure(
KingfisherError.requestError(reason: .dataProviderCancelled(provider: provider))
))
}
return
}
let processor = options.processor
let processingItem = ImageProcessItem.data(data)
guard let image = processor.process(item: processingItem, options: options) else {
Expand Down Expand Up @@ -606,8 +623,9 @@ public class KingfisherManager: @unchecked Sendable {
}

case .provider(let provider):
provideImage(provider: provider, options: options, completionHandler: _cacheImage)
return .dataProviding
let token = DataProvidingCancelToken()
provideImage(provider: provider, options: options, cancelToken: token, completionHandler: _cacheImage)
return .dataProviding(token)
}
}

Expand Down
46 changes: 43 additions & 3 deletions Sources/Networking/ImageDownloader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,19 @@ public final class DownloadTask: @unchecked Sendable {

init() { }

/// Internal initializer used for non-network sources (e.g. ``ImageDataProvider``).
/// The returned task carries only a ``DataProvidingCancelToken`` and cancels the provider
/// load when ``cancel()`` is called.
init(providerCancelToken: DataProvidingCancelToken) {
_providerCancelToken = providerCancelToken
}

private var _providerCancelToken: DataProvidingCancelToken? = nil
var providerCancelToken: DataProvidingCancelToken? {
get { propertyQueue.sync { _providerCancelToken } }
set { propertyQueue.sync { _providerCancelToken = newValue } }
}

private var _sessionTask: SessionDataTask? = nil

/// The ``SessionDataTask`` object associated with this download task. Multiple `DownloadTask`s could refer to the
Expand Down Expand Up @@ -119,6 +132,10 @@ public final class DownloadTask: @unchecked Sendable {
/// ``ImageDownloader/cancel(url:)``. If you need to cancel all downloading tasks of an ``ImageDownloader``,
/// use ``ImageDownloader/cancelAll()``.
public func cancel() {
if let providerCancelToken {
providerCancelToken.cancel()
return
}
guard let sessionTask, let cancelToken else { return }
sessionTask.cancel(token: cancelToken)
}
Expand All @@ -142,22 +159,45 @@ actor CancellationDownloadTask {
}
}

/// A cancellation handle for an in-flight ``ImageDataProvider`` loading task.
///
/// `ImageDataProvider` itself does not require providers to be interruptible — a provider's
/// `data(handler:)` call may still complete in the background after a cancel. This token simply
/// flags the Kingfisher side to stop propagating the result up the completion chain and to fire
/// a `.dataProviderCancelled` error instead, matching the behavior of cancelled network sources.
final class DataProvidingCancelToken: @unchecked Sendable {
private let lock = NSLock()
private var _isCancelled = false

var isCancelled: Bool {
lock.lock(); defer { lock.unlock() }
return _isCancelled
}

func cancel() {
lock.lock(); defer { lock.unlock() }
_isCancelled = true
}
}

extension DownloadTask {
enum WrappedTask {
case download(DownloadTask)
case dataProviding
case dataProviding(DataProvidingCancelToken?)

func cancel() {
switch self {
case .download(let task): task.cancel()
case .dataProviding: break
case .dataProviding(let token): token?.cancel()
}
}

var value: DownloadTask? {
switch self {
case .download(let task): return task
case .dataProviding: return nil
case .dataProviding(let token):
guard let token else { return nil }
return DownloadTask(providerCancelToken: token)
}
}
}
Expand Down
115 changes: 115 additions & 0 deletions Tests/KingfisherTests/ImageDataProviderCancellationTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// ImageDataProviderCancellationTests.swift
// Regression tests for issue #2511:
// "Images loaded with ImageDataProvider cannot be cancelled".
//
// Before the fix, calling `DownloadTask.cancel()` or
// `imageView.kf.cancelDownloadTask()` on a provider-backed load was a no-op,
// and the completion handler still fired with `.success` after the provider
// eventually delivered. After the fix, cancelling suppresses the success
// callback and delivers `.failure(.requestError(.dataProviderCancelled))`,
// matching the network source's behavior.

import XCTest
@testable import Kingfisher

private final class SlowDataProvider: ImageDataProvider, @unchecked Sendable {
let cacheKey: String
let delay: TimeInterval
let payload: Data

init(
cacheKey: String = "slow-provider-\(UUID().uuidString)",
delay: TimeInterval,
payload: Data
) {
self.cacheKey = cacheKey
self.delay = delay
self.payload = payload
}

func data(handler: @escaping @Sendable (Result<Data, any Error>) -> Void) {
let payload = self.payload
DispatchQueue.global().asyncAfter(deadline: .now() + delay) {
handler(.success(payload))
}
}
}

final class ImageDataProviderCancellationTests: XCTestCase {

private func makeManager() -> KingfisherManager {
KingfisherManager(
downloader: .default,
cache: ImageCache(name: "provider-cancel-\(UUID().uuidString.prefix(8))")
)
}

// Baseline: without cancel, the callback fires with .success.
func testProviderDeliversWhenNotCancelled() {
let manager = makeManager()
let provider = SlowDataProvider(delay: 0.2, payload: testImageData)
let done = expectation(description: "manager completion")

_ = manager.retrieveImage(with: .provider(provider), options: nil) { result in
if case .failure(let error) = result {
XCTFail("unexpected failure: \(error)")
}
done.fulfill()
}
wait(for: [done], timeout: 2.0)
}

// The `DownloadTask` returned for a provider source must now be non-nil
// and its `cancel()` must actually stop the completion chain.
func testCancellingProviderTaskDeliversCancelledError() {
let manager = makeManager()
let provider = SlowDataProvider(delay: 0.3, payload: testImageData)
let done = expectation(description: "manager completion")

let task = manager.retrieveImage(with: .provider(provider), options: nil) { result in
switch result {
case .success:
XCTFail("provider load should have been cancelled, but got .success")
case .failure(let error):
if case .requestError(reason: .dataProviderCancelled) = error {
// expected
} else {
XCTFail("expected .dataProviderCancelled, got: \(error)")
}
}
done.fulfill()
}

XCTAssertNotNil(task, "retrieveImage should now return a cancellable task for provider sources (#2511)")
task?.cancel()

wait(for: [done], timeout: 2.0)
}

// End-to-end through the UIImageView/NSImageView extension path:
// `imageView.kf.cancelDownloadTask()` must cancel the provider load and
// deliver `.dataProviderCancelled` to the setImage completion.
@MainActor
func testImageViewCancelDownloadTaskCancelsProvider() {
let imageView = KFCrossPlatformImageView()
let provider = SlowDataProvider(delay: 0.3, payload: testImageData)
let done = expectation(description: "setImage completion")

imageView.kf.setImage(with: .provider(provider)) { result in
switch result {
case .success:
XCTFail("setImage should have been cancelled, but got .success")
case .failure(let error):
if case .requestError(reason: .dataProviderCancelled) = error {
// expected
} else {
XCTFail("expected .dataProviderCancelled, got: \(error)")
}
}
done.fulfill()
}

imageView.kf.cancelDownloadTask()
wait(for: [done], timeout: 2.0)
}
}
Loading