Skip to content

Commit 7f7f988

Browse files
[FSSDK-11902] Fix concurrency bug in cmab service (#375)
* feat: Implement lock striping for decision retrieval in CMAB service * fix: Replace built-in hash method with MurmurHash3 for consistent lock index calculation
1 parent d789392 commit 7f7f988

File tree

2 files changed

+64
-2
lines changed

2 files changed

+64
-2
lines changed

lib/optimizely/cmab/cmab_service.rb

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
require 'digest'
2121
require 'json'
2222
require 'securerandom'
23+
require 'murmurhash3'
2324

2425
module Optimizely
2526
CmabDecision = Struct.new(:variation_id, :cmab_uuid, keyword_init: true)
@@ -35,13 +36,34 @@ class DefaultCmabService
3536
#
3637
# @raise [ArgumentError] If cmab_cache is not an instance of LRUCache.
3738
# @raise [ArgumentError] If cmab_client is not an instance of DefaultCmabClient.
39+
40+
NUM_LOCK_STRIPES = 1000
41+
3842
def initialize(cmab_cache, cmab_client, logger = nil)
3943
@cmab_cache = cmab_cache
4044
@cmab_client = cmab_client
4145
@logger = logger
46+
@locks = Array.new(NUM_LOCK_STRIPES) { Mutex.new }
4247
end
4348

4449
def get_decision(project_config, user_context, rule_id, options)
50+
lock_index = get_lock_index(user_context.user_id, rule_id)
51+
52+
@locks[lock_index].synchronize do
53+
get_decision_impl(project_config, user_context, rule_id, options)
54+
end
55+
end
56+
57+
private
58+
59+
def get_lock_index(user_id, rule_id)
60+
# Create a hash of user_id + rule_id for consistent lock selection
61+
hash_input = "#{user_id}#{rule_id}"
62+
hash_value = MurmurHash3::V32.str_hash(hash_input, 1) & 0xFFFFFFFF # Convert to unsigned 32-bit equivalent
63+
hash_value % NUM_LOCK_STRIPES
64+
end
65+
66+
def get_decision_impl(project_config, user_context, rule_id, options)
4567
# Retrieves a decision for a given user and rule, utilizing a cache for efficiency.
4668
#
4769
# This method filters user attributes, checks for various cache-related options,
@@ -85,8 +107,6 @@ def get_decision(project_config, user_context, rule_id, options)
85107
cmab_decision
86108
end
87109

88-
private
89-
90110
def fetch_decision(rule_id, user_id, attributes)
91111
# Fetches a decision for a given rule and user, along with user attributes.
92112
#

spec/cmab/cmab_service_spec.rb

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,4 +230,46 @@
230230
)
231231
end
232232
end
233+
234+
describe 'lock striping behavior' do
235+
describe '#get_lock_index' do
236+
it 'returns consistent lock index for same user/rule combination' do
237+
user_id = 'test_user'
238+
rule_id = 'test_rule'
239+
240+
# Get lock index multiple times
241+
index1 = cmab_service.send(:get_lock_index, user_id, rule_id)
242+
index2 = cmab_service.send(:get_lock_index, user_id, rule_id)
243+
index3 = cmab_service.send(:get_lock_index, user_id, rule_id)
244+
245+
# All should be the same
246+
expect(index1).to eq(index2), 'Same user/rule should always use same lock'
247+
expect(index2).to eq(index3), 'Same user/rule should always use same lock'
248+
end
249+
250+
it 'distributes different user/rule combinations across multiple locks' do
251+
test_cases = [
252+
%w[user1 rule1],
253+
%w[user2 rule1],
254+
%w[user1 rule2],
255+
%w[user3 rule3],
256+
%w[user4 rule4]
257+
]
258+
259+
lock_indices = Set.new
260+
test_cases.each do |user_id, rule_id|
261+
index = cmab_service.send(:get_lock_index, user_id, rule_id)
262+
263+
# Verify index is within expected range
264+
expect(index).to be >= 0, 'Lock index should be non-negative'
265+
expect(index).to be < Optimizely::DefaultCmabService::NUM_LOCK_STRIPES, 'Lock index should be less than NUM_LOCK_STRIPES'
266+
267+
lock_indices.add(index)
268+
end
269+
270+
# We should have multiple different lock indices (though not necessarily all unique due to hash collisions)
271+
expect(lock_indices.size).to be > 1, 'Different user/rule combinations should generally use different locks'
272+
end
273+
end
274+
end
233275
end

0 commit comments

Comments
 (0)