Skip to content

Commit 045aebc

Browse files
BenCodezBenCodez
authored
Merge pull request #337 from BenCodez/fix/atomic-user-cache-read
Make user cache reads atomic
2 parents c282772 + bc39cc2 commit 045aebc

4 files changed

Lines changed: 65 additions & 17 deletions

File tree

‎AdvancedCore/src/main/java/com/bencodez/advancedcore/api/user/UserData.java‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -167,10 +167,9 @@ public int getInt(UserStorage storage, String key, int def, UserDataFetchMode mo
167167
// preserve previous behavior
168168
user.cacheIfNeeded();
169169

170-
if (cache.isCached(key)) {
171-
DataValue cv = cache.getCache().get(key);
172-
if (cv != null) {
173-
if (cv.isInt()) {
170+
DataValue cv = cache.getIfPresent(key);
171+
if (cv != null) {
172+
if (cv.isInt()) {
174173
return cv.getInt();
175174
}
176175
String str = cv.getString();
@@ -181,7 +180,6 @@ public int getInt(UserStorage storage, String key, int def, UserDataFetchMode mo
181180
}
182181
}
183182
}
184-
}
185183
} else {
186184
user.cache();
187185
}
@@ -290,14 +288,13 @@ public String getString(UserStorage storage, String key, UserDataFetchMode mode)
290288
UserDataCache cache = user.getCache();
291289
sharedReadCache = cache;
292290
if (cache != null) {
293-
if (cache.isCached(key)) {
294-
DataValue cv = cache.getCache().get(key);
295-
if (cv != null) {
296-
String str = cv.getString();
297-
return (str != null) ? str : "";
298-
}
299-
return "";
300-
}
291+
java.util.Map.Entry<String, DataValue> cached = cache.getEntryIfPresent(key);
292+
if (cached != null) {
293+
DataValue cv = cached.getValue();
294+
if (cv == null) return "";
295+
String str = cv.getString();
296+
return (str != null) ? str : "";
297+
}
301298
} else {
302299
user.cache();
303300
}

‎AdvancedCore/src/main/java/com/bencodez/advancedcore/api/user/usercache/UserDataCache.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,6 +427,13 @@ public synchronized HashMap<String, DataValue> snapshot() {
427427
}
428428
public synchronized boolean hasChangesToProcess() { return cachedChanges != null && !cachedChanges.isEmpty(); }
429429
public synchronized boolean isCached(String key) { return cache != null && cache.containsKey(key); }
430+
/** Return one cache value while holding the same monitor used by retirement/replacement. */
431+
public synchronized DataValue getIfPresent(String key) { return cache == null ? null : cache.get(key); }
432+
/** Return a stable entry when the key is present, including a present null value. */
433+
public synchronized java.util.Map.Entry<String, DataValue> getEntryIfPresent(String key) {
434+
if (cache == null || !cache.containsKey(key)) return null;
435+
return new java.util.AbstractMap.SimpleImmutableEntry<>(key, cache.get(key));
436+
}
430437

431438
public synchronized void ensureNoLegacyBatchForSharedBinding() {
432439
if (sharedStorageWriter == null && inFlightBatches != 0) throw new IllegalStateException("Cannot attach shared storage during an active legacy batch");

‎AdvancedCore/src/test/java/com/bencodez/advancedcore/tests/storage/SqlUserDataFacadeTest.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,17 +50,16 @@ class SqlUserDataFacadeTest {
5050
Fixture f = new Fixture();
5151
UserDataCache cache = mock(UserDataCache.class);
5252
when(f.user.getCache()).thenReturn(cache);
53-
when(cache.isCached("Points")).thenReturn(true);
5453
HashMap<String, DataValue> userValues = new HashMap<>();
5554
userValues.put("Points", new DataValueInt(2));
56-
when(cache.getCache()).thenReturn(userValues);
55+
when(cache.getIfPresent("Points")).thenReturn(userValues.get("Points"));
5756
HashMap<String, DataValue> temp = new HashMap<>();
5857
temp.put("Points", new DataValueInt(3));
5958
f.data.setTempCache(temp);
6059
assertEquals(mode.allowTempCache() ? 3 : 2, f.data.getInt("Points", -1, mode));
6160
verifyNoInteractions(f.table);
6261
temp.clear();
63-
when(cache.isCached("Points")).thenReturn(false);
62+
when(cache.getIfPresent("Points")).thenReturn(null);
6463
when(f.table.getExact("initial-id")).thenReturn(new ArrayList<>(List.of(new Column("Points", new DataValueInt(4)))));
6564
assertEquals(mode.allowStorageLookup() ? 4 : -1, f.data.getInt("Points", -1, mode));
6665
verify(f.table, times(mode.allowStorageLookup() ? 1 : 0)).getExact("initial-id");

‎AdvancedCore/src/test/java/com/bencodez/advancedcore/tests/user/UserDataTest.java‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ public void deferredReadReturnsValuesPublishedAfterItsInitialCacheMiss() {
141141
when(users.getDataManager()).thenReturn(manager);
142142
when(manager.effectiveStorageType(UserStorage.SQLITE)).thenReturn(UserStorage.SQLITE);
143143
when(manager.mustDeferSharedStorageAccess()).thenReturn(true);
144-
when(cache.isCached(anyString())).thenReturn(false);
144+
when(cache.getIfPresent(anyString())).thenReturn(null);
145145
HashMap<String, DataValue> published = new HashMap<>();
146146
published.put("Points", new DataValueInt(42));
147147
published.put("PlayerName", new DataValueString("FreshName"));
@@ -151,4 +151,49 @@ public void deferredReadReturnsValuesPublishedAfterItsInitialCacheMiss() {
151151
assertEquals(42, data.getInt("Points", 17, UserDataFetchMode.DEFAULT));
152152
assertEquals("FreshName", data.getString("PlayerName", UserDataFetchMode.DEFAULT));
153153
}
154+
@Test
155+
public void cachedReadUsesAtomicCacheAccessor() {
156+
AdvancedCorePlugin plugin = mock(AdvancedCorePlugin.class);
157+
AdvancedCoreUser user = mock(AdvancedCoreUser.class);
158+
UserManager users = mock(UserManager.class);
159+
UserDataManager manager = mock(UserDataManager.class);
160+
UserDataCache cache = mock(UserDataCache.class);
161+
when(user.getPlugin()).thenReturn(plugin);
162+
when(user.getCache()).thenReturn(cache);
163+
when(plugin.getUserManager()).thenReturn(users);
164+
when(plugin.getStorageType()).thenReturn(UserStorage.SQLITE);
165+
when(users.getDataManager()).thenReturn(manager);
166+
when(manager.effectiveStorageType(UserStorage.SQLITE)).thenReturn(UserStorage.SQLITE);
167+
when(cache.getIfPresent("Points")).thenReturn(new DataValueInt(23));
168+
when(cache.getEntryIfPresent("PlayerName")).thenReturn(
169+
new java.util.AbstractMap.SimpleImmutableEntry<>("PlayerName", new DataValueString("Atomic")));
170+
171+
UserData data = new UserData(user);
172+
assertEquals(23, data.getInt("Points", 0, UserDataFetchMode.CACHE_ONLY));
173+
assertEquals("Atomic", data.getString("PlayerName", UserDataFetchMode.CACHE_ONLY));
174+
verify(cache).getIfPresent("Points");
175+
verify(cache).getEntryIfPresent("PlayerName");
176+
verify(cache, never()).getCache();
177+
}
178+
179+
@Test
180+
public void presentNullStringCacheEntryDoesNotFallThroughToStorage() {
181+
AdvancedCorePlugin plugin = mock(AdvancedCorePlugin.class);
182+
AdvancedCoreUser user = mock(AdvancedCoreUser.class);
183+
UserManager users = mock(UserManager.class);
184+
UserDataManager manager = mock(UserDataManager.class);
185+
UserDataCache cache = mock(UserDataCache.class);
186+
when(user.getPlugin()).thenReturn(plugin);
187+
when(user.getCache()).thenReturn(cache);
188+
when(plugin.getUserManager()).thenReturn(users);
189+
when(plugin.getStorageType()).thenReturn(UserStorage.SQLITE);
190+
when(users.getDataManager()).thenReturn(manager);
191+
when(manager.effectiveStorageType(UserStorage.SQLITE)).thenReturn(UserStorage.SQLITE);
192+
when(cache.getEntryIfPresent("PlayerName"))
193+
.thenReturn(new java.util.AbstractMap.SimpleImmutableEntry<>("PlayerName", null));
194+
195+
UserData data = new UserData(user);
196+
assertEquals("", data.getString("PlayerName", UserDataFetchMode.DEFAULT));
197+
}
198+
154199
}

0 commit comments

Comments
 (0)