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

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

Merged
merged 1 commit into from
Dec 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions Sources/EmbraceCore/Utils/KeychainAccess.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
55 changes: 42 additions & 13 deletions Sources/EmbraceCore/Utils/KeychainInterface.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions Tests/EmbraceCoreTests/Utils/KeychainAccessTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading