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

Possible Thread Safe Issue #187

Open
Shouheng88 opened this issue Jul 10, 2024 · 0 comments
Open

Possible Thread Safe Issue #187

Shouheng88 opened this issue Jul 10, 2024 · 0 comments

Comments

@Shouheng88
Copy link

Shouheng88 commented Jul 10, 2024

This is a good library and help me a lot. When I use this library in my project I meet a thread safe issue.

If the library is used in multi thread environment it may cause EXC_BAD_ACCESS error in code:

// https://github.com/amosavian/FileProvider/blob/master/Sources/RemoteSession.swift

internal var completionHandlersForTasks = [String: [Int: SimpleCompletionHandler]]()
internal var downloadCompletionHandlersForTasks = [String: [Int: (URL) -> Void]]()
internal var dataCompletionHandlersForTasks = [String: [Int: (Data) -> Void]]()
internal var responseCompletionHandlersForTasks = [String: [Int: (URLResponse) -> Void]]()

internal func removeSessionHandler(for uuid: String) {
    _ = completionHandlersForTasks.removeValue(forKey: uuid)
    _ = downloadCompletionHandlersForTasks.removeValue(forKey: uuid)
    _ = dataCompletionHandlersForTasks.removeValue(forKey: uuid) // Here
    _ = responseCompletionHandlersForTasks.removeValue(forKey: uuid)
}

The reason is the Dictionary data structure is not thread safe in Swift. The dataCompletionHandlersForTasks is global, so it has the risk of causing EXC_BAD_ACCESS error.

Since for most of cases we call FileProvider in main thread, and all options to dataCompletionHandlersForTasks happens on main thread, so we don't have the crash. But in multi-thread environment it occurs sometimes.

I think it may not be a good design to use a global data structure to hold callbacks. We could use an instance to hold them and pass it to SessionDelegate. For example,

public class SessionHandlerHolder {
    
    var completionHandlersForTasks = [String: [Int: SimpleCompletionHandler]]()
    var downloadCompletionHandlersForTasks = [String: [Int: (URL) -> Void]]()
    var dataCompletionHandlersForTasks = [String: [Int: (Data) -> Void]]()
    var responseCompletionHandlersForTasks = [String: [Int: (URLResponse) -> Void]]()

    func initEmptySessionHandler(_ uuid: String) {
        completionHandlersForTasks[uuid] = [:]
        downloadCompletionHandlersForTasks[uuid] = [:]
        dataCompletionHandlersForTasks[uuid] = [:]
        responseCompletionHandlersForTasks[uuid] = [:]
    }

    func removeSessionHandler(for uuid: String) {
        _ = completionHandlersForTasks.removeValue(forKey: uuid)
        _ = downloadCompletionHandlersForTasks.removeValue(forKey: uuid)
        _ = dataCompletionHandlersForTasks.removeValue(forKey: uuid)
        _ = responseCompletionHandlersForTasks.removeValue(forKey: uuid)
    }
}

I hope this issue could make this library better!
Thanks!

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

No branches or pull requests

1 participant