Race Condition in ImageDownloader.addDownloadTask

Check List

Thanks for considering to open an issue. Before you submit your issue, please confirm these boxes are checked.

[x ] I have read the wiki page and cheat sheet, but there is no information I need.
[x ] I have searched in existing issues, but did not find a same one.
[x ] I want to report a problem instead of asking a question. It'd better to use kingfisher tag in Stack Overflow to ask a question.

Issue Description

Calling KingfisherManager.shared.retrieveImage(with:) multiple times on a concurrent queue results in a race condition in the addDownloadTask(context:,callback:) method.

What

It causes the if/then/else to fail much of the time leaving some attempts to add new tasks to fall on the floor. The underlying sessionDelegate.task(for:) does have a lock, but that does not help the expression being tested by the if statement.

Reproduce

The following SwiftUI view can be used to reproduce the problem. We've seen it fail to update completionCount to the correct value most of the time. Yes, this example code is verify contrived but it was derived from a much more complicated piece of code that we cannot share.

struct ContentView: View {
    let retrievalAttemptCount = 5
    @State var completionCount = 0

    var body: some View {
        VStack(spacing: 16) {
            Button("Clear cache & reset") {
                KingfisherManager.shared.cache.clearCache()
                completionCount = 0
            }
            Button("Download images") {
                completionCount = 0

                guard let imageURL = URL(string: "https://images.pexels.com/photos/96938/pexels-photo-96938.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=2")
                else {
                    return
                }

                for _ in 1...retrievalAttemptCount {
                    DispatchQueue.global(qos: .default).async {
                        KingfisherManager.shared.retrieveImage(with: imageURL) { result in
                            Task { @MainActor in
                                completionCount += 1
                            }
                        }
                    }
                }
            }

            Text("Completion count: \(completionCount)/\(retrievalAttemptCount)")

            Spacer()
        }
        .padding()
    }
}

Other Comment

I played around with a naive solution by adding the following code to the top of the addDownloadTask(context:,callback:) method and it seems to avoid the race condition.

        lock.lock()
        defer { lock.unlock() }

onevcat / Kingfisher