Merge pull request #2444 from darkbrewx/fix/task-add-overwrite

Fix non-atomic task creation for concurrent same-URL requests

Author: onevcat

Sources/Networking/ImageDownloader.swift

@@ -248,6 +248,8 @@ open class ImageDownloader: @unchecked Sendable {
     // The session bound to the downloader.
     private var session: URLSession
 
+    private let lock = NSLock()
+
     // MARK: Initializers
 
     /// Creates a downloader with a given name.
@@ -375,6 +377,9 @@ open class ImageDownloader: @unchecked Sendable {
         callback: SessionDataTask.TaskCallback
     ) -> DownloadTask
     {
+        lock.lock()
+        defer { lock.unlock() }
+
         // Ready to start download. Add it to session task manager (`sessionHandler`)
         let downloadTask: DownloadTask
         if let existingTask = sessionDelegate.task(for: context.url) {

Tests/KingfisherTests/ImageDownloaderTests.swift

@@ -684,6 +684,49 @@ class ImageDownloaderTests: XCTestCase {
         XCTAssertEqual(result.originalData, testImageData)
         XCTAssertEqual(result.url, url)
     }
+
+    func testConcurrentDownloadSameURL() {
+        let exp = expectation(description: #function)
+
+        // Given
+        let url = testURLs[0]
+        stub(url, data: testImageData)
+
+        let callbackLock = NSLock()
+        var callbackCount = 0
+        let expectedCount = 10
+        exp.expectedFulfillmentCount = expectedCount
+
+        // When
+        DispatchQueue.concurrentPerform(iterations: expectedCount) { index in
+            downloader.downloadImage(with: url) { result in
+                switch result {
+                case .success(let imageResult):
+                    XCTAssertNotNil(imageResult.image)
+                    XCTAssertEqual(imageResult.url, url)
+                    XCTAssertEqual(imageResult.originalData, testImageData)
+
+                    callbackLock.lock()
+                    callbackCount += 1
+                    callbackLock.unlock()
+
+                    exp.fulfill()
+
+                case .failure(let error):
+                    XCTFail("Download should succeed: \(error)")
+                    exp.fulfill()
+                }
+            }
+        }
+
+        // Then
+        waitForExpectations(timeout: 3) { _ in
+            callbackLock.lock()
+            let finalCount = callbackCount
+            callbackLock.unlock()
+            XCTAssertEqual(finalCount, expectedCount, "All \(expectedCount) concurrent requests should receive callbacks")
+        }
+    }
 }
 
 class URLNilDataModifier: ImageDownloaderDelegate {