[FSSDK-11900] fix: potential concurrent issue for cmab decision#608
Conversation
- Introduce a lock array to prevent race conditions - Implement getLockIndex method for calculating lock index based on user and rule IDs
muzahidul-opti
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a potential race condition in the CMAB decision-making process by implementing a lock array mechanism to ensure thread-safe access to decision operations.
- Introduces an array of 1000 NSLock objects to manage concurrent access
- Implements a hash-based lock selection strategy using user and rule IDs to distribute lock contention
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- Replace hash function with MurmurHash3 for improved performance - Calculate lockIndex directly instead of using intermediate variables
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| private func getLockIndex(userId: String, ruleId: String) -> Int { | ||
| let combinedKey = userId + ruleId | ||
| let hashValue = MurmurHash3.hash32(key: combinedKey) | ||
| let lockIndex = Int(hashValue) % Self.NUM_LOCKS |
There was a problem hiding this comment.
The conversion from UInt32 to Int could result in a negative value on platforms where Int is 32-bit, leading to a potential crash when accessing the locks array. Use Int(hashValue % UInt32(Self.NUM_LOCKS)) to ensure the modulo operation happens before conversion.
| let lockIndex = Int(hashValue) % Self.NUM_LOCKS | |
| let lockIndex = Int(hashValue % UInt32(Self.NUM_LOCKS)) |
| return lock.withLock { | ||
| var result: Result<CmabDecision, Error>! | ||
| let semaphore = DispatchSemaphore(value: 0) |
There was a problem hiding this comment.
Using a semaphore to block while holding a lock can lead to deadlock if the asynchronous callback in getDecision attempts to acquire the same lock. The lock should be released before waiting on the semaphore, or the async operation should be moved outside the lock scope.
Summary
Test plan
Issues