Fixed hangs on setting new deviceId on keychain using main thread (#137)

embrace-io · Dec 4, 2024 · dbcf4b2 · dbcf4b2
1 parent 25aee8c
commit dbcf4b2
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 24 deletions.
diff --git a/Sources/EmbraceCore/Utils/KeychainAccess.swift b/Sources/EmbraceCore/Utils/KeychainAccess.swift
@@ -31,17 +31,16 @@ class KeychainAccess {
 
         // generate new id
         let newId = UUID()
-        let status = keychain.setValue(
+        keychain.setValue(
             service: kEmbraceKeychainService as CFString,
             account: kEmbraceDeviceId as CFString,
-            value: newId.uuidString
-        )
-
-        if status != errSecSuccess {
-            if let err = SecCopyErrorMessageString(status, nil) {
-                Embrace.logger.error("Write failed: \(err)")
+            value: newId.uuidString) { status in
+                if status != errSecSuccess {
+                    if let err = SecCopyErrorMessageString(status, nil) {
+                        Embrace.logger.error("Write failed: \(err)")
+                    }
+                }
             }
-        }
 
         return newId
     }

diff --git a/Sources/EmbraceCore/Utils/KeychainInterface.swift b/Sources/EmbraceCore/Utils/KeychainInterface.swift
@@ -6,11 +6,17 @@ import Foundation
 
 protocol KeychainInterface {
     func valueFor(service: CFString, account: CFString) -> (value: String?, status: OSStatus)
-    func setValue(service: CFString, account: CFString, value: String) -> OSStatus
+    func setValue(service: CFString, account: CFString, value: String, completion: @escaping (OSStatus) -> Void)
     func deleteValue(service: CFString, account: CFString) -> OSStatus
 }
 
 class DefaultKeychainInterface: KeychainInterface {
+    private let queue: DispatchQueue
+
+    init(queue: DispatchQueue = DispatchQueue(label: "com.embrace.keychainAccess", qos: .userInitiated)) {
+        self.queue = queue
+    }
+
     func valueFor(service: CFString, account: CFString) -> (value: String?, status: OSStatus) {
         let keychainQuery: NSMutableDictionary = [
             kSecClass: kSecClassGenericPassword,
@@ -35,20 +41,43 @@ class DefaultKeychainInterface: KeychainInterface {
         return (contentsOfKeychain, status)
     }
 
-    func setValue(service: CFString, account: CFString, value: String) -> OSStatus {
-        guard let dataFromString = value.data(using: String.Encoding.utf8, allowLossyConversion: false) else {
-            return errSecParam
-        }
+    func setValue(
+        service: CFString,
+        account: CFString,
+        value: String,
+        completion: @escaping (OSStatus) -> Void
+    ) {
+        /*
 
-        let querry: NSMutableDictionary = [
-            kSecClass: kSecClassGenericPassword,
-            kSecAttrService: service,
-            kSecAttrAccount: account,
-            kSecValueData: dataFromString
-        ]
+         Why Async in `setValue` but not in `valueFor`?
+         -> The decision to make this method asynchronous, while keeping `valueFor` synchronous, is based on the nature of Keychain operations and their expected performance characteristics:
+
+         `setValue` (Write Operations):
+         * Writing to the Keychain (in this case using `SecItemAdd`) often requires coordination with the system's `securityd` process.
+         * These tasks can occasionally take time, especially under system load or network conditions, which can block the calling thread.
+         * Performing this on the Main Thread risks causing UI freezes or App Hangs, making it necessary to offload the operation to a background thread.
 
-        // Add the new keychain item
-        return SecItemAdd( querry, nil)
+         `valueFor` (Read Operations):
+         * Reading from the Keychain (in this case using `SecItemCopyMatching`) is generally faster because it doesn't modify the Keychain.
+         * The system can return cached results for some queries, making read operations more predictable in terms of performance.
+
+         */
+        queue.async {
+            guard let dataFromString = value.data(using: String.Encoding.utf8, allowLossyConversion: false) else {
+                completion(errSecParam)
+                return
+            }
+
+            let query: NSMutableDictionary = [
+                kSecClass: kSecClassGenericPassword,
+                kSecAttrService: service,
+                kSecAttrAccount: account,
+                kSecValueData: dataFromString
+            ]
+
+            // Add the new keychain item
+            completion(SecItemAdd(query, nil))
+        }
     }
 
     func deleteValue(service: CFString, account: CFString) -> OSStatus {

diff --git a/Tests/EmbraceCoreTests/Utils/KeychainAccessTest.swift b/Tests/EmbraceCoreTests/Utils/KeychainAccessTest.swift
@@ -12,10 +12,10 @@ class AlwaysSuccessfulKeychainInterface: KeychainInterface {
     func valueFor(service: CFString, account: CFString) -> (value: String?, status: OSStatus) {
         return (value, errSecSuccess)
     }
-
-    func setValue(service: CFString, account: CFString, value: String) -> OSStatus {
+    
+    func setValue(service: CFString, account: CFString, value: String, completion: (OSStatus) -> Void) {
         self.value = value
-        return errSecSuccess
+        completion(errSecSuccess)
     }
 
     func deleteValue(service: CFString, account: CFString) -> OSStatus {