Fix downloader concurrency issue

Sources/General/KingfisherManager.swift

@@ -545,7 +545,7 @@ public class KingfisherManager: @unchecked Sendable {
             //
             // return task.map(DownloadTask.WrappedTask.download)
 
-            if let task = task {
+            if task.isInitialized {
                 return .download(task)
             } else {
                 return nil

Sources/Networking/ImageDownloader.swift

@@ -62,26 +62,40 @@ public struct ImageLoadingResult: Sendable {
 /// When a download starts in Kingfisher, the involved methods always return you an instance of ``DownloadTask``. If you
 /// need to cancel the task during the download process, you can keep a reference to the instance and call ``cancel()``
 /// on it.
-public final class DownloadTask: Sendable {
+public final class DownloadTask: @unchecked Sendable {
+    
+    private let propertyQueue = DispatchQueue(label: "com.onevcat.Kingfisher.DownloadTaskPropertyQueue")
     
     init(sessionTask: SessionDataTask, cancelToken: SessionDataTask.CancelToken) {
-        self.sessionTask = sessionTask
-        self.cancelToken = cancelToken
+        _sessionTask = sessionTask
+        _cancelToken = cancelToken
     }
+    
+    init() { }
 
+    private var _sessionTask: SessionDataTask? = nil
+    
     /// The ``SessionDataTask`` object associated with this download task. Multiple `DownloadTask`s could refer to the
     /// same `sessionTask`. This is an optimization in Kingfisher to prevent multiple downloading tasks for the same
     /// URL resource simultaneously.
     ///
     /// When you call ``DownloadTask/cancel()``, this ``SessionDataTask`` and its cancellation token will be passed
     /// along. You can use them to identify the cancelled task.
-    public let sessionTask: SessionDataTask
+    public private(set) var sessionTask: SessionDataTask? {
+        get { propertyQueue.sync { _sessionTask! } }
+        set { propertyQueue.sync { _sessionTask = newValue } }
+    }
 
+    private var _cancelToken: SessionDataTask.CancelToken? = nil
+    
     /// The cancellation token used to cancel the task.
     ///
     /// This is solely for identifying the task when it is cancelled. To cancel a ``DownloadTask``, call
     ///  ``DownloadTask/cancelToken``.
-    public let cancelToken: SessionDataTask.CancelToken
+    public private(set) var cancelToken: SessionDataTask.CancelToken? {
+        get { propertyQueue.sync { _cancelToken } }
+        set { propertyQueue.sync { _cancelToken = newValue } }
+    }
 
     /// Cancel this single download task if it is running.
     ///
@@ -100,8 +114,20 @@ public final class DownloadTask: Sendable {
     /// ``ImageDownloader/cancel(url:)``. If you need to cancel all downloading tasks of an ``ImageDownloader``, 
     /// use ``ImageDownloader/cancelAll()``.
     public func cancel() {
+        guard let sessionTask, let cancelToken else { return }
         sessionTask.cancel(token: cancelToken)
     }
+    
+    public var isInitialized: Bool {
+        propertyQueue.sync {
+            _sessionTask != nil && _cancelToken != nil
+        }
+    }
+    
+    func linkToTask(_ task: DownloadTask) {
+        self.sessionTask = task.sessionTask
+        self.cancelToken = task.cancelToken
+    }
 }
 
 actor CancellationDownloadTask {
@@ -294,7 +320,7 @@ open class ImageDownloader: @unchecked Sendable {
     private func createDownloadContext(
         with url: URL,
         options: KingfisherParsedOptionsInfo,
-        done: @escaping ((Result<DownloadingContext, KingfisherError>) -> Void)
+        done: @escaping (@Sendable (Result<DownloadingContext, KingfisherError>) -> Void)
     )
     {
         @Sendable func checkRequestAndDone(r: URLRequest) {
@@ -392,8 +418,7 @@ open class ImageDownloader: @unchecked Sendable {
     {
         let downloadTask = addDownloadTask(context: context, callback: callback)
 
-        let sessionTask = downloadTask.sessionTask
-        guard !sessionTask.started else {
+        guard let sessionTask = downloadTask.sessionTask, !sessionTask.started else {
             return downloadTask
         }
 
@@ -452,9 +477,9 @@ open class ImageDownloader: @unchecked Sendable {
     open func downloadImage(
         with url: URL,
         options: KingfisherParsedOptionsInfo,
-        completionHandler: ((Result<ImageLoadingResult, KingfisherError>) -> Void)? = nil) -> DownloadTask?
+        completionHandler: (@Sendable (Result<ImageLoadingResult, KingfisherError>) -> Void)? = nil) -> DownloadTask
     {
-        var downloadTask: DownloadTask?
+        let downloadTask = DownloadTask()
         createDownloadContext(with: url, options: options) { result in
             switch result {
             case .success(let context):
@@ -463,10 +488,11 @@ open class ImageDownloader: @unchecked Sendable {
                 // `AsyncImageDownloadRequestModifier` is used the returned `downloadTask` of this method will be `nil`
                 // and the actual "delayed" task is given in `AsyncImageDownloadRequestModifier.onDownloadTaskStarted`
                 // callback.
-                downloadTask = self.startDownloadTask(
+                let actualDownloadTask = self.startDownloadTask(
                     context: context,
                     callback: self.createTaskCallback(completionHandler, options: options)
                 )
+                downloadTask.linkToTask(actualDownloadTask)
                 if let modifier = options.requestModifier {
                     modifier.onDownloadTaskStarted?(downloadTask)
                 }
@@ -521,7 +547,7 @@ open class ImageDownloader: @unchecked Sendable {
     open func downloadImage(
         with url: URL,
         options: KingfisherOptionsInfo? = nil,
-        completionHandler: ((Result<ImageLoadingResult, KingfisherError>) -> Void)? = nil) -> DownloadTask?
+        completionHandler: ((Result<ImageLoadingResult, KingfisherError>) -> Void)? = nil) -> DownloadTask
     {
         downloadImage(
             with: url,
@@ -552,7 +578,7 @@ extension ImageDownloader {
                     continuation.resume(with: result)
                 }
                 if Task.isCancelled {
-                    downloadTask?.cancel()
+                    downloadTask.cancel()
                 } else {
                     Task {
                         await task.setTask(downloadTask)

Tests/KingfisherTests/ImageDownloaderTests.swift

@@ -139,7 +139,7 @@ class ImageDownloaderTests: XCTestCase {
             XCTAssertEqual(result.value?.url, url)
             exp.fulfill()
         }
-        XCTAssertNotNil(task)
+        XCTAssertTrue(task.isInitialized)
         waitForExpectations(timeout: 3, handler: nil)
     }
 
@@ -169,7 +169,7 @@ class ImageDownloaderTests: XCTestCase {
             }
         }
         // The returned task is nil since the download is not starting immediately.
-        XCTAssertNil(task)
+        XCTAssertFalse(task.isInitialized)
         waitForExpectations(timeout: 3, handler: nil)
     }
 
@@ -309,7 +309,7 @@ class ImageDownloaderTests: XCTestCase {
             group.leave()
         }
 
-        task1?.cancel()
+        task1.cancel()
         delay(0.1) { _ = stub.go() }
         group.notify(queue: .main) {
             delay(0.1) { exp.fulfill() }
@@ -433,10 +433,10 @@ class ImageDownloaderTests: XCTestCase {
         waitForExpectations(timeout: 3, handler: nil)
     }
     
-    func testDownloadTaskNil() {
+    func testDownloadTaskNilWithNilURL() {
         let modifier = URLModifier(url: nil)
         let downloadTask = downloader.downloadImage(with: URL(string: "url")!, options: [.requestModifier(modifier)])
-        XCTAssertNil(downloadTask)
+        XCTAssertFalse(downloadTask.isInitialized)
     }
     
     func testDownloadWithProcessor() {
@@ -486,7 +486,7 @@ class ImageDownloaderTests: XCTestCase {
         }
 
         XCTAssertNotNil(task1)
-        XCTAssertEqual(task1?.sessionTask.task, task2?.sessionTask.task)
+        XCTAssertEqual(task1.sessionTask?.task, task2.sessionTask?.task)
 
         _ = stub.go()
         
@@ -580,7 +580,7 @@ class ImageDownloaderTests: XCTestCase {
             _ in
             exp.fulfill()
         }
-        XCTAssertEqual(task?.sessionTask.task.priority, URLSessionTask.highPriority)
+        XCTAssertEqual(task.sessionTask?.task.priority, URLSessionTask.highPriority)
         waitForExpectations(timeout: 3, handler: nil)
     }
     

Tests/KingfisherTests/KingfisherManagerTests.swift

@@ -976,7 +976,7 @@ class KingfisherManagerTests: XCTestCase {
           options: [.alternativeSources([.network(url)])],
           downloadTaskUpdated: { newTask in
             downloadTaskUpdatedCount += 1
-            XCTAssertEqual(newTask?.sessionTask.task.currentRequest?.url, url)
+            XCTAssertEqual(newTask?.sessionTask?.task.currentRequest?.url, url)
           })
           {
             result in
@@ -984,7 +984,7 @@ class KingfisherManagerTests: XCTestCase {
             exp.fulfill()
         }
 
-        XCTAssertEqual(task?.sessionTask.task.currentRequest?.url, brokenURL)
+        XCTAssertEqual(task?.sessionTask?.task.currentRequest?.url, brokenURL)
 
         waitForExpectations(timeout: 1, handler: nil)
     }