Mark
Mark

Reputation: 18244

How can I achieve thread safety in an async method?

I am trying to read the keychain but the documentation warns me about the following:

SecItemCopyMatching blocks the calling thread, so it can cause your app’s UI to hang if called from the main thread. Instead, call SecItemCopyMatching from a background dispatch queue or async function.

Source

So I want to write an asynchronous method that runs in the background.

actor Keychain {
    public static let standard = Keychain()
    
    public enum Error: Swift.Error {
        case failed(String)
    }
    
    public func get(_ key: String) async throws -> Data? {
        let backgroundTask = Task(priority: .background) {
            var query: [String: Any] = [
                type(of: self).klass       : kSecClassGenericPassword,
                type(of: self).attrAccount : key,
                type(of: self).matchLimit  : kSecMatchLimitOne,
                type(of: self).returnData  : kCFBooleanTrue as CFBoolean
            ]
            
            var item: CFTypeRef?
            let status = SecItemCopyMatching(query as CFDictionary, &item)
            
            guard status == errSecSuccess else {
                if let errorMessage = SecCopyErrorMessageString(status, nil) {
                    throw Error.failed(String(errorMessage))
                } else {
                    throw Error.failed("unsupported")
                }
            }
            
            return item as? Data
        }
        return try await backgroundTask.value
    }
}

My question is.. will the actor already make it thread safe?

Normally I would add a NSLock to be safe.

public func get(_ key: String) async throws -> Data? {
    lock.lock()
    defer { lock.unlock() }
    
    (...)
    
    return try await task.value
}

However now I get a warning Instance method 'lock' is unavailable from asynchronous contexts; Use async-safe scoped locking instead; this is an error in Swift 6.

So how I am able to achieve this?

Upvotes: 9

Views: 4205

Answers (3)

Rob
Rob

Reputation: 438467

Yes, actors will make it thread-safe. No need for locks or the like. See WWDC 2021 video Protect mutable state with Swift actors.


BTW, since you've got this on an actor, you really don't need to make get(_:) an async method. The actor already runs on a background thread. So remove the async qualifier and then remove the Task:

actor Keychain {
    ...
    
    public func get(_ key: String) throws -> Data? {
        var query: [String: Any] = [
            type(of: self).klass       : kSecClassGenericPassword,
            type(of: self).attrAccount : key,
            type(of: self).matchLimit  : kSecMatchLimitOne,
            type(of: self).returnData  : kCFBooleanTrue as CFBoolean
        ]
        
        var item: CFTypeRef?
        let status = SecItemCopyMatching(query as CFDictionary, &item)
        
        guard status == errSecSuccess else {
            if let errorMessage = SecCopyErrorMessageString(status, nil) {
                throw Error.failed(String(errorMessage))
            } else {
                throw Error.failed("unsupported")
            }
        }
        
        return item as? Data
    }
}

As an aside, while locks are generally rendered unnecessary within Swift concurrency because we generally achieve similar synchronization with actors, there are edge cases where we might use them. In those rare cases, we just have to use them carefully. For example, the compiler will warn us that the following may not be permitted in Swift 6:

// Warning: Instance method 'lock' is unavailable from asynchronous contexts;
// Use async-safe scoped locking instead; this is an error in Swift 6

lock.lock()
someSynchronousStuff()
lock.unlock()

But this is safe, as the compiler can reason about the localized use of the lock:

// OK

lock.withLock {
    someSynchronousStuff()
}

But you cannot call asynchronous routines from within withLock because we never want to use locks across concurrency context boundaries:

// Error: Cannot pass function of type '() async -> ()' to parameter expecting
// synchronous function type

lock.withLock {
    await someSynchronousStuff()
}

E.g., here are the warnings/errors produced by Xcode 15.3:

enter image description here

Upvotes: 9

Truelies42
Truelies42

Reputation: 143

Why do you put Keychain operations in a async block that is not needed. You are good to go without it. Keychain operations are Thread safe and that is guaranteed by Apple. See this documentation. Apple

So do it without the async block and put all ui changes in the main thread (if it is not already). UI changes need to be in the main thread.

Upvotes: 0

Duncan C
Duncan C

Reputation: 131511

It looks like you are using all local variables (no instance or static variables) so you should be fine.

Upvotes: 0

Related Questions