Skip to content

Commit

Permalink
Remove CacheMap#containsKey before #get
Browse files Browse the repository at this point in the history
Currently, when accessing values from the `DataLoader`'s `CacheMap`, we
first check `#containsKey` before invoking `#get`. This is problematic
for two reasons:

 - the underlying cache's metrics are skewed (it will have a 100% hit
   rate).
 - if the cache has an automatic expiration policy, it is possible a
   value will expire between the `containsKey` and `get` invocations
   leading to an incorrect result.

To ameliorate this, we always invoke `#get` and check if it is `null` to
deterine whether the key is present. We wrap the invocation in a
`try`/`catch` as the `#get` method is allowed to through per its
documentation.
  • Loading branch information
AlexandreCarlton committed Apr 29, 2024
1 parent bd101a2 commit 12a73d3
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 9 deletions.
3 changes: 1 addition & 2 deletions src/main/java/org/dataloader/CacheMap.java
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,7 @@ static <K, V> CacheMap<K, V> simpleMap() {
/**
* Gets the specified key from the cache map.
* <p>
* May throw an exception if the key does not exist, depending on the cache map implementation that is used,
* so be sure to check {@link CacheMap#containsKey(Object)} first.
* May throw an exception if the key does not exist, depending on the cache map implementation that is used.
*
* @param key the key to retrieve
*
Expand Down
22 changes: 15 additions & 7 deletions src/main/java/org/dataloader/DataLoaderHelper.java
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,13 @@ Optional<CompletableFuture<V>> getIfPresent(K key) {
boolean cachingEnabled = loaderOptions.cachingEnabled();
if (cachingEnabled) {
Object cacheKey = getCacheKey(nonNull(key));
if (futureCache.containsKey(cacheKey)) {
stats.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(key));
return Optional.of(futureCache.get(cacheKey));
try {
CompletableFuture<V> cacheValue = futureCache.get(cacheKey);
if (cacheValue != null) {
stats.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(key));
return Optional.of(cacheValue);
}
} catch (Exception ignored) {
}
}
}
Expand Down Expand Up @@ -307,10 +311,14 @@ private void possiblyClearCacheEntriesOnExceptions(List<K> keys) {
private CompletableFuture<V> loadFromCache(K key, Object loadContext, boolean batchingEnabled) {
final Object cacheKey = loadContext == null ? getCacheKey(key) : getCacheKeyWithContext(key, loadContext);

if (futureCache.containsKey(cacheKey)) {
// We already have a promise for this key, no need to check value cache or queue up load
stats.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(key, loadContext));
return futureCache.get(cacheKey);
try {
CompletableFuture<V> cacheValue = futureCache.get(cacheKey);
if (cacheValue != null) {
// We already have a promise for this key, no need to check value cache or queue up load
stats.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(key, loadContext));
return cacheValue;
}
} catch (Exception ignored) {
}

CompletableFuture<V> loadCallFuture = queueOrInvokeLoader(key, loadContext, batchingEnabled, true);
Expand Down

0 comments on commit 12a73d3

Please sign in to comment.