CompensatingCounters exceeds actual statistics when calling DefaultCacheStatistics.clear() ?

[IamRyusei]

This is an issue of a behaviour that it seems not being covered by any test or documentation, therefore I'm not sure if its intended behaviour or not, but it sure seem weird if it is. If anyone could confirm me that this is a bug, I could work on a fix since it looks like a fairly easy task to fix.

The issue occurs in some of my unit test when trying to assert whatever a query causes a cache hit or miss. There is a bit of hacking going on with type casting and reflections, and i'm not sure it is the best way to access those statistics, but it worked so far: each individual test unit passes. the problem is when i run them togheter since the statistics are not updated correctly.

dependencies are:

• hibernate:*:5.4.33.Final
• javax.cache:cache-api:1.1.0
• org.ehcache:ehcache:3.10.8

private static Cache<Object, Object> entityCache;
private static CacheStatisticsMXBean entityCacheStats;

@SneakyThrows
private static CacheStatisticsMXBean obtainCacheStatisticsBean(Cache<?, ?> cache) {
        final Method method = MethodUtils.getMatchingMethod(cache.getClass(), "getStatisticsMBean");
        method.setAccessible(true);
        return (CacheStatisticsMXBean)method.invoke(cache);
}

@BeforeAll
static void beforeAll() {
    // Access Cache
    final EntityManagerFactory emf = EntityManagerFactoryProvider.getEntityManagerFactory(PERSISTENCE_UNIT);
    final EnabledCaching cache = (EnabledCaching)emf.getCache();
    final JCacheRegionFactory cacheRegionFactory = (JCacheRegionFactory)cache.getRegionFactory();
    final CacheManager cacheManager = cacheRegionFactory.getCacheManager();

    entityCache = cacheManager.getCache(ENTITY_CACHE_REGION_NAME);
    entityCacheStats = obtainCacheStatisticsBean(entityCache );
}

@Test
void test() {
    // HERE RUN ANY QUERY THAT CAUSES FOR EXAMPLE 1 CACHE MISS.
    // entityCacheStats.getCacheMisses() = 1
    // ((DefaultCacheStatistics) ((Eh107CacheStatisticsMXBean)entityCacheStats).cacheStatistics).compensatingCounters.cacheMisses = 0
    // ((DefaultCacheStatistics) ((Eh107CacheStatisticsMXBean)entityCacheStats).cacheStatistics).getMisses() = 1

    entityCacheStats.clear(); 
    // entityCacheStats.getCacheMisses() = 0
    // ((DefaultCacheStatistics) ((Eh107CacheStatisticsMXBean)entityCacheStats).cacheStatistics).compensatingCounters.cacheMisses = 1
    // ((DefaultCacheStatistics) ((Eh107CacheStatisticsMXBean)entityCacheStats).cacheStatistics).getMisses() = 1

    entityCacheStats.clear(); 
    // entityCacheStats.getCacheMisses() = 0
    // ((DefaultCacheStatistics) ((Eh107CacheStatisticsMXBean)entityCacheStats).cacheStatistics).compensatingCounters.cacheMisses = 2
    // ((DefaultCacheStatistics) ((Eh107CacheStatisticsMXBean)entityCacheStats).cacheStatistics).getMisses() = 1

    // HERE RUN ANY QUERY THAT CAUSES FOR EXAMPLE 1 CACHE MISS.
    // entityCacheStats.getCacheMisses() = 0
    // ((DefaultCacheStatistics) ((Eh107CacheStatisticsMXBean)entityCacheStats).cacheStatistics).compensatingCounters.cacheMisses = 3
    // ((DefaultCacheStatistics) ((Eh107CacheStatisticsMXBean)entityCacheStats).cacheStatistics).getMisses() = 2

    // INSTEAD, here I would expect entityCacheStats.getCacheMisses() = 1 since i just cleared the statistics before the query. This causes other test units to fail.
}

I identified this behaviour in the following method: org.ehcache.core.internal.statistics.DefaultCacheStatistics:clear() calls

CompensatingCounters:snapshot(DefaultCacheStatistics statistics) {
    return new CompensatingCounters(
        this.cacheHits + statistics.getHits(), 
        this.cacheMisses + statistics.getMisses(), 
        this.cacheGets + statistics.getCacheGets(), 
        this.cachePuts + statistics.getCachePuts(), 
        this.cacheRemovals + statistics.getCacheRemovals());
}

shouldn't it be

CompensatingCounters:snapshot(DefaultCacheStatistics statistics) {
    return new CompensatingCounters(
        statistics.getHits(), 
        statistics.getMisses(), 
        statistics.getCacheGets(), 
        statistics.getCachePuts(), 
        statistics.getCacheRemovals());
}

[chrisdennis]

I'll lead with the statement that I'm not really a fan of the way any of this is currently structured... that said I believe sticking to the established patterns the correct fix is:

diff --git a/ehcache-core/src/main/java/org/ehcache/core/internal/statistics/DefaultCacheStatistics.java b/ehcache-core/src/main/java/org/ehcache/core/internal/statistics/DefaultCacheStatistics.java
index 294d8c81f..4e55c6656 100644
--- a/ehcache-core/src/main/java/org/ehcache/core/internal/statistics/DefaultCacheStatistics.java
+++ b/ehcache-core/src/main/java/org/ehcache/core/internal/statistics/DefaultCacheStatistics.java
@@ -232,8 +232,8 @@ public class DefaultCacheStatistics implements CacheStatistics {

     CompensatingCounters snapshot(DefaultCacheStatistics statistics) {
       return new CompensatingCounters(
-        cacheHits + statistics.getHits(),
-        cacheMisses + statistics.getMisses(),
+        cacheHits + statistics.getCacheHits(),
+        cacheMisses + statistics.getCacheMisses(),
         cacheGets + statistics.getCacheGets(),
         cachePuts + statistics.getCachePuts(),
         cacheRemovals + statistics.getCacheRemovals());

In order for the existing logic to work, and to fit the pattern used here and in other places this compensation approach is used the method called needs to be the one that is itself compensated. 🤢

[chrisdennis]

Sorry... slip of the mouse closed the issue.

Let me know if you're interested in pushing this change through a PR, otherwise I'll get around to doing it in the next couple of days.

[IamRyusei]

Hey Chris,

I think your solution is correct. I run a quick local test and it works. However, if you want to have a cleaner solution, I would suggest to implement the getGets(), getPuts() and getRemovals() methods, in order to have a better "symmetry" with the getCacheHits() and getCacheMiss() that already have their own getHits() and getMiss() methods.

Something like this:

public long getCacheGets() {
    return normalize(this.getGets() - this.compensatingCounters.cacheGets);
}

public long getCachePuts() {
    return normalize(this.getPuts() - this.compensatingCounters.cachePuts);
}

public long getCacheRemovals() {
    return normalize(this.getRemovals() - this.compensatingCounters.cacheRemovals);
}

private long getGets() { 
    return this.getHits() + this.getMisses(); 
}

private long getPuts() { 
    return this.getBulkCount(BulkOps.PUT_ALL) 
    + this.put.sum(EnumSet.of(PutOutcome.PUT)) 
    + this.putIfAbsent.sum(EnumSet.of(PutIfAbsentOutcome.PUT)) 
    + this.replace.sum(EnumSet.of(ReplaceOutcome.HIT));
}

private long getRemovals() { 
    return this.getBulkCount(BulkOps.REMOVE_ALL) 
    + this.remove.sum(EnumSet.of(RemoveOutcome.SUCCESS)) 
    + this.conditionalRemove.sum(EnumSet.of(ConditionalRemoveOutcome.SUCCESS));
}

In this way you would be able to implement the fix as:

return new CompensatingCounters(
    statistics.getHits(),
    statistics.getMisses(),
    statistics.getGets(),
    statistics.getPuts(),
    statistics.getRemovals());

[IamRyusei]

given how simple is getGets() it could also be inlined. I'm not sure what style policy would be preferred.

[IamRyusei]

@chrisdennis any update on this one?