Skip to content

Commit 54bafad

Browse files
FarhanAnjum-optiFarhanAnjum-opti
authored
[FSSDK-11899] update: Fix concurrency bug in cmab service (#585)
* fix: resolve concurrency issues in DefaultCmabService with lock striping * fix: update lock stripe configuration based on client engine type
1 parent b60ddec commit 54bafad

File tree

2 files changed

+117
-40
lines changed

2 files changed

+117
-40
lines changed

‎core-api/src/main/java/com/optimizely/ab/cmab/service/DefaultCmabService.java‎

Lines changed: 54 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@
2020
import java.util.List;
2121
import java.util.Map;
2222
import java.util.TreeMap;
23+
import java.util.concurrent.locks.ReentrantLock;
2324

25+
import com.optimizely.ab.event.internal.ClientEngineInfo;
2426
import org.slf4j.Logger;
2527
import org.slf4j.LoggerFactory;
2628

@@ -37,10 +39,13 @@
3739
public class DefaultCmabService implements CmabService {
3840
public static final int DEFAULT_CMAB_CACHE_SIZE = 10000;
3941
public static final int DEFAULT_CMAB_CACHE_TIMEOUT_SECS = 30*60; // 30 minutes
42+
private static final boolean IS_ANDROID = ClientEngineInfo.getClientEngineName().toLowerCase().contains("android");
43+
private static final int NUM_LOCK_STRIPES = IS_ANDROID ? 100 : 1000;
4044

4145
private final Cache<CmabCacheValue> cmabCache;
4246
private final CmabClient cmabClient;
4347
private final Logger logger;
48+
private final ReentrantLock[] locks;
4449

4550
public DefaultCmabService(CmabClient cmabClient, Cache<CmabCacheValue> cmabCache) {
4651
this(cmabClient, cmabCache, null);
@@ -50,52 +55,64 @@ public DefaultCmabService(CmabClient cmabClient, Cache<CmabCacheValue> cmabCache
5055
this.cmabCache = cmabCache;
5156
this.cmabClient = cmabClient;
5257
this.logger = logger != null ? logger : LoggerFactory.getLogger(DefaultCmabService.class);
58+
this.locks = new ReentrantLock[NUM_LOCK_STRIPES];
59+
for (int i = 0; i < NUM_LOCK_STRIPES; i++) {
60+
this.locks[i] = new ReentrantLock();
61+
}
5362
}
5463

5564
@Override
5665
public CmabDecision getDecision(ProjectConfig projectConfig, OptimizelyUserContext userContext, String ruleId, List<OptimizelyDecideOption> options) {
5766
options = options == null ? Collections.emptyList() : options;
5867
String userId = userContext.getUserId();
59-
Map<String, Object> filteredAttributes = filterAttributes(projectConfig, userContext, ruleId);
6068

61-
if (options.contains(OptimizelyDecideOption.IGNORE_CMAB_CACHE)) {
62-
logger.debug("Ignoring CMAB cache for user '{}' and rule '{}'", userId, ruleId);
63-
return fetchDecision(ruleId, userId, filteredAttributes);
64-
}
69+
int lockIndex = getLockIndex(userId, ruleId);
70+
ReentrantLock lock = locks[lockIndex];
71+
lock.lock();
72+
try {
73+
Map<String, Object> filteredAttributes = filterAttributes(projectConfig, userContext, ruleId);
6574

66-
if (options.contains(OptimizelyDecideOption.RESET_CMAB_CACHE)) {
67-
logger.debug("Resetting CMAB cache for user '{}' and rule '{}'", userId, ruleId);
68-
cmabCache.reset();
69-
}
75+
if (options.contains(OptimizelyDecideOption.IGNORE_CMAB_CACHE)) {
76+
logger.debug("Ignoring CMAB cache for user '{}' and rule '{}'", userId, ruleId);
77+
return fetchDecision(ruleId, userId, filteredAttributes);
78+
}
7079

71-
String cacheKey = getCacheKey(userContext.getUserId(), ruleId);
72-
if (options.contains(OptimizelyDecideOption.INVALIDATE_USER_CMAB_CACHE)) {
73-
logger.debug("Invalidating CMAB cache for user '{}' and rule '{}'", userId, ruleId);
74-
cmabCache.remove(cacheKey);
75-
}
80+
if (options.contains(OptimizelyDecideOption.RESET_CMAB_CACHE)) {
81+
logger.debug("Resetting CMAB cache for user '{}' and rule '{}'", userId, ruleId);
82+
cmabCache.reset();
83+
}
84+
85+
String cacheKey = getCacheKey(userContext.getUserId(), ruleId);
86+
if (options.contains(OptimizelyDecideOption.INVALIDATE_USER_CMAB_CACHE)) {
87+
logger.debug("Invalidating CMAB cache for user '{}' and rule '{}'", userId, ruleId);
88+
cmabCache.remove(cacheKey);
89+
}
7690

77-
CmabCacheValue cachedValue = cmabCache.lookup(cacheKey);
91+
CmabCacheValue cachedValue = cmabCache.lookup(cacheKey);
7892

79-
String attributesHash = hashAttributes(filteredAttributes);
93+
String attributesHash = hashAttributes(filteredAttributes);
8094

81-
if (cachedValue != null) {
82-
if (cachedValue.getAttributesHash().equals(attributesHash)) {
83-
logger.debug("CMAB cache hit for user '{}' and rule '{}'", userId, ruleId);
84-
return new CmabDecision(cachedValue.getVariationId(), cachedValue.getCmabUuid());
95+
if (cachedValue != null) {
96+
if (cachedValue.getAttributesHash().equals(attributesHash)) {
97+
logger.debug("CMAB cache hit for user '{}' and rule '{}'", userId, ruleId);
98+
return new CmabDecision(cachedValue.getVariationId(), cachedValue.getCmabUuid());
99+
} else {
100+
logger.debug("CMAB cache attributes mismatch for user '{}' and rule '{}', fetching new decision", userId, ruleId);
101+
cmabCache.remove(cacheKey);
102+
}
85103
} else {
86-
logger.debug("CMAB cache attributes mismatch for user '{}' and rule '{}', fetching new decision", userId, ruleId);
87-
cmabCache.remove(cacheKey);
104+
logger.debug("CMAB cache miss for user '{}' and rule '{}'", userId, ruleId);
88105
}
89-
} else {
90-
logger.debug("CMAB cache miss for user '{}' and rule '{}'", userId, ruleId);
91-
}
92106

93-
CmabDecision cmabDecision = fetchDecision(ruleId, userId, filteredAttributes);
94-
logger.debug("CMAB decision is {}", cmabDecision);
95-
96-
cmabCache.save(cacheKey, new CmabCacheValue(attributesHash, cmabDecision.getVariationId(), cmabDecision.getCmabUUID()));
107+
CmabDecision cmabDecision = fetchDecision(ruleId, userId, filteredAttributes);
108+
logger.debug("CMAB decision is {}", cmabDecision);
97109

98-
return cmabDecision;
110+
cmabCache.save(cacheKey, new CmabCacheValue(attributesHash, cmabDecision.getVariationId(), cmabDecision.getCmabUUID()));
111+
112+
return cmabDecision;
113+
} finally {
114+
lock.unlock();
115+
}
99116
}
100117

101118
private CmabDecision fetchDecision(String ruleId, String userId, Map<String, Object> attributes) {
@@ -192,6 +209,13 @@ private String hashAttributes(Map<String, Object> attributes) {
192209
return Integer.toHexString(hash);
193210
}
194211

212+
private int getLockIndex(String userId, String ruleId) {
213+
// Create a hash of userId + ruleId for consistent lock selection
214+
String combined = userId + ruleId;
215+
int hash = MurmurHash3.murmurhash3_x86_32(combined, 0, combined.length(), 0);
216+
return Math.abs(hash) % NUM_LOCK_STRIPES;
217+
}
218+
195219
public static Builder builder() {
196220
return new Builder();
197221
}

‎core-api/src/test/java/com/optimizely/ab/cmab/DefaultCmabServiceTest.java‎

Lines changed: 63 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,18 +15,14 @@
1515
*/
1616
package com.optimizely.ab.cmab;
1717

18-
import java.util.Arrays;
19-
import java.util.Collections;
20-
import java.util.HashMap;
21-
import java.util.LinkedHashMap;
22-
import java.util.List;
23-
import java.util.Map;
24-
25-
import static org.junit.Assert.assertEquals;
26-
import static org.junit.Assert.assertNotNull;
18+
import java.lang.reflect.Method;
19+
import java.util.*;
20+
2721
import org.junit.Before;
2822
import org.junit.Test;
2923
import org.mockito.ArgumentCaptor;
24+
25+
import static org.junit.Assert.*;
3026
import static org.mockito.Matchers.any;
3127
import static org.mockito.Matchers.anyString;
3228
import static org.mockito.Matchers.eq;
@@ -375,4 +371,61 @@ public void testAttributeOrderDoesNotMatterForCaching() {
375371
assertNotNull(decision.getCmabUUID());
376372
verify(mockCmabCache).save(eq(cacheKey), any(CmabCacheValue.class));
377373
}
378-
}
374+
@Test
375+
public void testLockStripingDistribution() {
376+
// Test different combinations to ensure they get different lock indices
377+
String[][] testCases = {
378+
{"user1", "rule1"},
379+
{"user2", "rule1"},
380+
{"user1", "rule2"},
381+
{"user3", "rule3"},
382+
{"user4", "rule4"}
383+
};
384+
385+
Set<Integer> lockIndices = new HashSet<>();
386+
for (String[] testCase : testCases) {
387+
String userId = testCase[0];
388+
String ruleId = testCase[1];
389+
390+
// Use reflection to access the private getLockIndex method
391+
try {
392+
Method getLockIndexMethod = DefaultCmabService.class.getDeclaredMethod("getLockIndex", String.class, String.class);
393+
getLockIndexMethod.setAccessible(true);
394+
395+
int index = (Integer) getLockIndexMethod.invoke(cmabService, userId, ruleId);
396+
397+
// Verify index is within expected range
398+
assertTrue("Lock index should be non-negative", index >= 0);
399+
assertTrue("Lock index should be less than NUM_LOCK_STRIPES", index < 1000);
400+
401+
lockIndices.add(index);
402+
} catch (Exception e) {
403+
fail("Failed to invoke getLockIndex method: " + e.getMessage());
404+
}
405+
}
406+
407+
assertTrue("Different user/rule combinations should generally use different locks", lockIndices.size() > 1);
408+
}
409+
410+
@Test
411+
public void testSameUserRuleCombinationUsesConsistentLock() {
412+
String userId = "test_user";
413+
String ruleId = "test_rule";
414+
415+
try {
416+
Method getLockIndexMethod = DefaultCmabService.class.getDeclaredMethod("getLockIndex", String.class, String.class);
417+
getLockIndexMethod.setAccessible(true);
418+
419+
// Get lock index multiple times
420+
int index1 = (Integer) getLockIndexMethod.invoke(cmabService, userId, ruleId);
421+
int index2 = (Integer) getLockIndexMethod.invoke(cmabService, userId, ruleId);
422+
int index3 = (Integer) getLockIndexMethod.invoke(cmabService, userId, ruleId);
423+
424+
// All should be the same
425+
assertEquals("Same user/rule should always use same lock", index1, index2);
426+
assertEquals("Same user/rule should always use same lock", index2, index3);
427+
} catch (Exception e) {
428+
fail("Failed to invoke getLockIndex method: " + e.getMessage());
429+
}
430+
}
431+
}

0 commit comments

Comments
 (0)