bug: duplicate cacheHits & cacheMisses in CompensatingCounters after multiple clear()

[J. David Beutel]

Hi Ehcache developers,

I submitted a "New conversation" for this on https://groups.google.com/g/ehcache-dev earlier today, but it hasn't appeared yet, so I'll try using email now.

I think I found a bug in DefaultCacheStatistics.CompensatingCounters: when it has cacheHits or cacheMisses, snapshot() duplicates them.  So, from the second time that DefaultCacheStatistics.clear() is called, they double, at least.  After that, getCacheHits() and getCacheMisses() results would be negative, if normalize() didn't limit them to 0, until getHits() and getMisses() double, at least.

This bug can be reproduced by simply adding a third innerClear() call to testClearingStats() in CacheCalculationTest and JCacheCalculationTest, which then fail like this:

[Test worker] ERROR org.ehcache.integration.statistics.JCacheCalculationTest - Counters at time of assertion:  (H=0 M=0 P=3 R=2 Ev=0 Ex=0)
org.assertj.core.api.SoftAssertionError: 
The following 2 assertions failed:
1) [Hits] expected:<[1]L> but was:<[0]L>
2) [Misses] expected:<[4]L> but was:<[0]L>

This can be fixed with the following change, for consistency:

     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());

Or, I think the following change would work as well:

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

It looks like the bug was introduced about 9 years ago, by the following commit, which was to stop bulks from being calculated twice.

https://github.com/ehcache/ehcache3/commit/ff0cfb8db7556173593a80c89c3f0795c3fa0b69

So, it seems like it's been this way for a long time.  Nevertheless, is this a bug, and a good fix for it?  Should I submit a pull request on the 3.10, 3.11, and master branches?

I'm not familiar with the project, and would need help to run all of the tests (some of which seem to try to connect to another host), and to load the master branch as a project in IntelliJ IDEA (which got a lot of dependency errors, even using JDK 17).  It looks like DefaultCacheStatistics currently in org.ehcache.core.internal.statistics was copied to org.ehcache.management.statistics in 2020, to "truly hide the statistics library uses", and both copies have this issue.  The latter isn't tested directly?

I put my suggested fix in a commit, for discussion, on the 3.10 branch:

https://github.com/jdbeutel/ehcache3/commit/fae67ac01f5b3633207e2bce59416403f4988cd2

Cheers,
11011011