Merge pull request #2548 from onevcat/fix/duplicate-download-finish-callback

Fix duplicate didFinishDownloadingImageForURL delegate callback

Author: onevcat

Sources/Networking/ImageDownloader.swift

@@ -332,15 +332,6 @@ open class ImageDownloader: @unchecked Sendable {
         sessionDelegate.onResponseReceived.delegate(on: self) { (self, response) in
             await (self.delegate ?? self).imageDownloader(self, didReceive: response)
         }
-        sessionDelegate.onDownloadingFinished.delegate(on: self) { (self, value) in
-            let (url, result) = value
-            do {
-                let value = try result.get()
-                self.delegate?.imageDownloader(self, didFinishDownloadingImageForURL: url, with: value, error: nil)
-            } catch {
-                self.delegate?.imageDownloader(self, didFinishDownloadingImageForURL: url, with: nil, error: error)
-            }
-        }
         sessionDelegate.onDidDownloadData.delegate(on: self) { (self, task) in
             (self.delegate ?? self).imageDownloader(self, didDownload: task.sharedData, with: task)
         }

Sources/Networking/SessionDelegate.swift

@@ -48,7 +48,6 @@ open class SessionDelegate: NSObject, @unchecked Sendable {
 
     let onValidStatusCode = Delegate<Int, Bool>()
     let onResponseReceived = Delegate<URLResponse, URLSession.ResponseDisposition>()
-    let onDownloadingFinished = Delegate<(URL, Result<URLResponse, KingfisherError>), Void>()
     let onDidDownloadData = Delegate<SessionDataTask, Data?>()
 
     let onReceiveSessionChallenge = Delegate<SessionChallengeFunc, (URLSession.AuthChallengeDisposition, URLCredential?)>()
@@ -199,18 +198,6 @@ extension SessionDelegate: URLSessionDataDelegate {
     open func urlSession(_ session: URLSession, task: URLSessionTask, didCompleteWithError error: (any Error)?) {
         guard let sessionTask = self.task(for: task) else { return }
 
-        if let url = sessionTask.originalURL {
-            let result: Result<URLResponse, KingfisherError>
-            if let error = error {
-                result = .failure(KingfisherError.responseError(reason: .URLSessionError(error: error)))
-            } else if let response = task.response {
-                result = .success(response)
-            } else {
-                result = .failure(KingfisherError.responseError(reason: .noURLResponse(task: sessionTask)))
-            }
-            onDownloadingFinished.call((url, result))
-        }
-
         let result: Result<(Data, URLResponse?), KingfisherError>
         if let error = error {
             result = .failure(KingfisherError.responseError(reason: .URLSessionError(error: error)))

Tests/KingfisherTests/ImageDownloaderTests.swift

@@ -125,6 +125,85 @@ class ImageDownloaderTests: XCTestCase {
         waitForExpectations(timeout: 5, handler: nil)
     }
 
+    // `imageDownloader(_:didFinishDownloadingImageForURL:with:error:)` must be called exactly once
+    // per download. Historically two paths invoked it for the same completion (`onDownloadingFinished`
+    // in `setupSessionHandler()` and `reportDidDownloadImageData` in `startDownloadTask`'s `onTaskDone`
+    // handler), so success and network-error downloads notified the delegate twice, while a
+    // data-modifying failure first reported a success (nil error) and then a failure.
+    func testDidFinishDownloadingDelegateCalledOncePerSuccessfulDownload() {
+        let exp = expectation(description: #function)
+
+        let url = testURLs[0]
+        stub(url, data: testImageData)
+
+        let delegate = DownloadFinishCountingDelegate()
+        downloader.delegate = delegate
+
+        downloader.downloadImage(with: url) { result in
+            XCTAssertNotNil(result.value)
+            XCTAssertEqual(delegate.finishedEvents.count, 1)
+            exp.fulfill()
+        }
+        waitForExpectations(timeout: 3, handler: nil)
+        _ = delegate // keep the weak delegate alive for the duration of the test
+    }
+
+    func testDidFinishDownloadingDelegateCalledOncePerFailedDownload() {
+        let exp = expectation(description: #function)
+
+        let url = testURLs[0]
+        stub(url, data: testImageData, statusCode: 404)
+
+        let delegate = DownloadFinishCountingDelegate()
+        downloader.delegate = delegate
+
+        downloader.downloadImage(with: url) { result in
+            XCTAssertNotNil(result.error)
+            XCTAssertEqual(delegate.finishedEvents.count, 1)
+            exp.fulfill()
+        }
+        waitForExpectations(timeout: 3, handler: nil)
+        _ = delegate
+    }
+
+    func testDidFinishDownloadingDelegateCalledOncePerErroredDownload() {
+        let exp = expectation(description: #function)
+
+        let url = testURLs[0]
+        stub(url, errorCode: NSURLErrorNotConnectedToInternet)
+
+        let delegate = DownloadFinishCountingDelegate()
+        downloader.delegate = delegate
+
+        downloader.downloadImage(with: url) { result in
+            XCTAssertNotNil(result.error)
+            XCTAssertEqual(delegate.finishedEvents.count, 1)
+            exp.fulfill()
+        }
+        waitForExpectations(timeout: 3, handler: nil)
+        _ = delegate
+    }
+
+    func testDidFinishDownloadingDelegateConsistentWhenDataModifyingFails() {
+        let exp = expectation(description: #function)
+
+        let url = testURLs[0]
+        stub(url, data: testImageData)
+
+        let delegate = NilModifierCountingDelegate()
+        downloader.delegate = delegate
+
+        downloader.downloadImage(with: url) { result in
+            XCTAssertNotNil(result.error)
+            XCTAssertEqual(delegate.finishedEvents.count, 1)
+            // The single event should report the failure, not a success.
+            XCTAssertNotNil(delegate.finishedEvents.first?.error)
+            exp.fulfill()
+        }
+        waitForExpectations(timeout: 3, handler: nil)
+        _ = delegate
+    }
+
     func testDownloadWithModifyingRequest() {
         let exp = expectation(description: #function)
 
@@ -794,6 +873,57 @@ class TaskResponseCompletion: ImageDownloaderDelegate {
     }
 }
 
+class DownloadFinishCountingDelegate: ImageDownloaderDelegate, @unchecked Sendable {
+    private let lock = NSLock()
+    private var _finishedEvents: [(url: URL, response: URLResponse?, error: (any Error)?)] = []
+
+    var finishedEvents: [(url: URL, response: URLResponse?, error: (any Error)?)] {
+        lock.lock()
+        defer { lock.unlock() }
+        return _finishedEvents
+    }
+
+    func imageDownloader(
+        _ downloader: ImageDownloader,
+        didFinishDownloadingImageForURL url: URL,
+        with response: URLResponse?,
+        error: (any Error)?)
+    {
+        lock.lock()
+        defer { lock.unlock() }
+        _finishedEvents.append((url, response, error))
+    }
+}
+
+// Implements the data-modifying method directly instead of subclassing
+// `DownloadFinishCountingDelegate`: a protocol requirement satisfied by a protocol extension in the
+// parent is not re-dispatched to a subclass override through the existential.
+class NilModifierCountingDelegate: ImageDownloaderDelegate, @unchecked Sendable {
+    private let lock = NSLock()
+    private var _finishedEvents: [(url: URL, response: URLResponse?, error: (any Error)?)] = []
+
+    var finishedEvents: [(url: URL, response: URLResponse?, error: (any Error)?)] {
+        lock.lock()
+        defer { lock.unlock() }
+        return _finishedEvents
+    }
+
+    func imageDownloader(
+        _ downloader: ImageDownloader,
+        didFinishDownloadingImageForURL url: URL,
+        with response: URLResponse?,
+        error: (any Error)?)
+    {
+        lock.lock()
+        defer { lock.unlock() }
+        _finishedEvents.append((url, response, error))
+    }
+
+    func imageDownloader(_ downloader: ImageDownloader, didDownload data: Data, with task: SessionDataTask) -> Data? {
+        return nil
+    }
+}
+
 final class URLModifier: ImageDownloadRequestModifier {
     let url: URL?
     init(url: URL?) {