Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Xcode thread sanitizer identifies race condition inside the onDataReceived method of the ImageLoadingProgressiveSideEffect class #2325

Closed
3 tasks
meisbedi opened this issue Nov 25, 2024 · 1 comment · Fixed by #2327

Comments

@meisbedi
Copy link

meisbedi commented Nov 25, 2024

Check List

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

Xcode 16.1 on macOs 15.1.1
Kingfisher Swift package 8.1.1

Using the KFImage struct to load multiple different images asynchronously results in a race condition inside the onDataReceived method of the ImageLoadingProgressiveSideEffect class.

The onDataReceived method checks the amount of data received with a direct access to the count property of the mutableData property of the SessionDataTask class. Running my iOS app in the simulator under iOS 18.1 with the thread sanitizer enabled reports this access as a potential race condition.

There is another race condition inside the SessionDelegate extension. The urlSession.didReceive method calls the task.didReceiveData method of the SessionDataTask class. The access to mutableData through this method is not thread safe, because a lock is missing inside the "didReceiveData" method.

Reproduce

Calling the following SwiftUI view concurrently several times will reproduce the issue:

struct ImageForView: View {

    var url: URL?
    
    @State private var failed = false
        
    var placeholder: String {
        return "record.circle"
    }
    
    var body: some View {
        if let imageUrl = self.url {
            if !self.failed {
                KFImage(imageUrl)
                    .placeholder { ProgressView() }
                    .cacheMemoryOnly()
                    .onFailure({ error in
                        Logger.error("Error loading image: \(error)")
                        self.failed = true
                    })
                    .resizable().scaledToFit()
            } else { Image(systemName: "exclamationmark.shield.fill").resizable().scaledToFit() }
        } else { Image(systemName: self.placeholder).resizable().scaledToFit() }   // if imageURL== nil
    }
}

Other Comment

Creating a computed property muatbleDataCount inside the SessionDataTask class with a lock for the access to the size of mutableData as well as adding a lock to the didReceiveData method fixes the problems

@onevcat
Copy link
Owner

onevcat commented Dec 1, 2024

Umm, thanks for catching and reporting this!

A fix is arriving!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants