-
Notifications
You must be signed in to change notification settings - Fork 32
[FSSDK-11900] fix: potential concurrent issue for cmab decision #608
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
base: master
Are you sure you want to change the base?
Conversation
- Introduce a lock array to prevent race conditions - Implement getLockIndex method for calculating lock index based on user and rule IDs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Oct 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)) |
Copilot uses AI. Check for mistakes.
return lock.withLock { | ||
var result: Result<CmabDecision, Error>! | ||
let semaphore = DispatchSemaphore(value: 0) |
Copilot
AI
Oct 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot uses AI. Check for mistakes.
Summary
Test plan
Issues