Skip to content

Commit

Permalink
Fix reference cache triggering maintenance after a write (fixed #111)
Browse files Browse the repository at this point in the history 
This oversight caused write methods to not try to clean-up collected
entries for caching using only the weak/soft reference configurations.
The maintenance would be triggered after enough reads or if manually
calling cleanUp().

Most of the changes are improvements to the unit tests, as test flaws
allowed this bug to be introduced. Primarily the cause was due to not
being strict enough when evaluating the removal notifications. The tests
now highlight a slight difference in Guava & Caffeine regarding if
replacing a value with the same instance triggers a notification. Other
small mistakes in the reference cache tests were uncovered and fixed.
  • Loading branch information
@ben-manes
ben-manes committed Aug 11, 2016
1 parent 3713aa8 commit d26d0c9
Show file tree
Hide file tree
Showing 11 changed files with 199 additions and 114 deletions.
5 changes: 5 additions & 0 deletions caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,8 @@ void afterWrite(@Nullable Node<K, V> node, Runnable task, long now) {
} catch (RuntimeException e) {
logger.log(Level.SEVERE, "Exception thrown when performing the maintenance task", e);
}
} else {
scheduleAfterWrite();
}
}

Expand Down Expand Up @@ -2187,6 +2189,9 @@ V remap(K key, Object keyRef, BiFunction<? super K, ? super V, ? extends V> rema
afterWrite(node, new UpdateTask(node, weightedDifference), now);
} else {
afterRead(node, now, /* recordHit */ false);
if (cause[0] == RemovalCause.COLLECTED) {
scheduleDrainBuffers();
}
}
}

Expand Down
63 changes: 46 additions & 17 deletions caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsMapTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,11 @@ public void put_replace_sameValue(Map<Integer, Integer> map, CacheContext contex
}
int count = context.firstMiddleLastKeys().size();
assertThat(map.size(), is(context.original().size()));
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.REPLACED));
if (context.isGuava() || context.isAsync()) {
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.REPLACED));
} else {
assertThat(context.consumedNotifications(), hasSize(0));
}
}

@CheckNoStats
Expand Down Expand Up @@ -443,20 +447,23 @@ public void putAll_replace(Map<Integer, Integer> map, CacheContext context) {
@CacheSpec(population = { Population.PARTIAL, Population.FULL },
removalListener = { Listener.DEFAULT, Listener.CONSUMING })
public void putAll_mixed(Map<Integer, Integer> map, CacheContext context) {
Map<Integer, Integer> expect = new HashMap<>(context.original());
Map<Integer, Integer> entries = new HashMap<>();
for (int i = 0; i < 2 * context.original().size(); i++) {
int value = ((i % 2) == 0) ? i : (i + 1);
entries.put(i, value);
}
expect.putAll(entries);
Map<Integer, Integer> replaced = new HashMap<>();
context.original().forEach((key, value) -> {
if ((key % 2) == 0) {
value++;
replaced.put(key, value);
}
entries.put(key, value);
});

map.putAll(entries);
assertThat(map, is(equalTo(expect)));
assertThat(map, hasRemovalNotifications(context, entries.size() / 2, RemovalCause.REPLACED));
assertThat(map, is(equalTo(entries)));
Map<Integer, Integer> expect = (context.isGuava() || context.isAsync()) ? entries : replaced;
assertThat(map, hasRemovalNotifications(context, expect.size(), RemovalCause.REPLACED));

verifyWriter(context, (verifier, writer) -> {
verifier.wroteAll(entries);
verifier.wroteAll(replaced);
});
}

Expand Down Expand Up @@ -694,8 +701,12 @@ public void replace_sameValue(Map<Integer, Integer> map, CacheContext context) {
}
assertThat(map.size(), is(context.original().size()));

int count = context.firstMiddleLastKeys().size();
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.REPLACED));
if (context.isGuava() || context.isAsync()) {
int count = context.firstMiddleLastKeys().size();
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.REPLACED));
} else {
assertThat(context.consumedNotifications(), hasSize(0));
}
}

@CheckNoStats
Expand Down Expand Up @@ -814,8 +825,12 @@ public void replaceConditionally_sameValue(Map<Integer, Integer> map, CacheConte
}
assertThat(map.size(), is(context.original().size()));

int count = context.firstMiddleLastKeys().size();
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.REPLACED));
if (context.isGuava() || context.isAsync()) {
int count = context.firstMiddleLastKeys().size();
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.REPLACED));
} else {
assertThat(context.consumedNotifications(), hasSize(0));
}
}

@CheckNoStats
Expand Down Expand Up @@ -870,7 +885,12 @@ public void replaceAll_writerFails(Map<Integer, Integer> map, CacheContext conte
public void replaceAll_sameValue(Map<Integer, Integer> map, CacheContext context) {
map.replaceAll((key, value) -> value);
assertThat(map, is(equalTo(context.original())));
assertThat(map, hasRemovalNotifications(context, map.size(), RemovalCause.REPLACED));

if (context.isGuava() || context.isAsync()) {
assertThat(map, hasRemovalNotifications(context, map.size(), RemovalCause.REPLACED));
} else {
assertThat(context.consumedNotifications(), hasSize(0));
}
}

@CacheSpec
Expand Down Expand Up @@ -1204,7 +1224,12 @@ public void compute_sameValue(Map<Integer, Integer> map, CacheContext context) {
assertThat(map.get(key), is(value));
}
assertThat(map.size(), is(context.original().size()));
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.REPLACED));

if (context.isGuava() || context.isAsync()) {
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.REPLACED));
} else {
assertThat(context.consumedNotifications(), hasSize(0));
}
}

@CheckNoWriter
Expand Down Expand Up @@ -1341,7 +1366,11 @@ public void merge_sameValue(Map<Integer, Integer> map, CacheContext context) {
assertThat(map.get(key), is(value));
}
assertThat(map.size(), is(context.original().size()));
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.REPLACED));
if (context.isGuava() || context.isAsync()) {
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.REPLACED));
} else {
assertThat(context.consumedNotifications(), hasSize(0));
}
}

@CheckNoWriter
Expand Down
26 changes: 16 additions & 10 deletions caffeine/src/test/java/com/github/benmanes/caffeine/cache/CacheTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,10 @@ public void put_replace_sameValue(Cache<Integer, Integer> cache, CacheContext co
assertThat(cache.estimatedSize(), is(context.initialSize()));

int count = context.firstMiddleLastKeys().size();
assertThat(cache, hasRemovalNotifications(context, count, RemovalCause.REPLACED));

if (context.isGuava() || context.isAsync()) {
assertThat(cache, hasRemovalNotifications(context, count, RemovalCause.REPLACED));
}
}

@Test(dataProvider = "caches")
Expand Down Expand Up @@ -374,20 +377,23 @@ public void putAll_replace(Cache<Integer, Integer> cache, CacheContext context)
@CacheSpec(population = { Population.PARTIAL, Population.FULL },
removalListener = { Listener.DEFAULT, Listener.CONSUMING })
public void putAll_mixed(Cache<Integer, Integer> cache, CacheContext context) {
Map<Integer, Integer> expect = new HashMap<>(context.original());
Map<Integer, Integer> entries = new HashMap<>();
for (int i = 0; i < 2 * context.initialSize(); i++) {
int value = ((i % 2) == 0) ? i : (i + 1);
entries.put(i, value);
}
expect.putAll(entries);
Map<Integer, Integer> replaced = new HashMap<>();
context.original().forEach((key, value) -> {
if ((key % 2) == 0) {
value++;
replaced.put(key, value);
}
entries.put(key, value);
});

cache.putAll(entries);
assertThat(cache.asMap(), is(equalTo(expect)));
assertThat(cache, hasRemovalNotifications(context, entries.size() / 2, RemovalCause.REPLACED));
assertThat(cache.asMap(), is(equalTo(entries)));
Map<Integer, Integer> expect = (context.isGuava() || context.isAsync()) ? entries : replaced;
assertThat(cache, hasRemovalNotifications(context, expect.size(), RemovalCause.REPLACED));

verifyWriter(context, (verifier, writer) -> {
verifier.wroteAll(entries);
verifier.wroteAll(replaced);
});
}

Expand Down
3 changes: 2 additions & 1 deletion caffeine/src/test/java/com/github/benmanes/caffeine/cache/EvictionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,8 @@ public void maximumSize_decrease(Cache<Integer, Integer> cache,
}

@Test(dataProvider = "caches")
@CacheSpec(implementation = Implementation.Caffeine, maximumSize = Maximum.FULL,
@CacheSpec(implementation = Implementation.Caffeine,
maximumSize = Maximum.FULL, weigher = { CacheWeigher.DEFAULT, CacheWeigher.TEN },
removalListener = { Listener.DEFAULT, Listener.CONSUMING })
public void maximumSize_decrease_min(Cache<Integer, Integer> cache,
CacheContext context, Eviction<Integer, Integer> eviction) {
Expand Down
36 changes: 29 additions & 7 deletions caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpirationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@ public void put_replace(Cache<Integer, Integer> cache, CacheContext context) {
assertThat(cache.getIfPresent(context.middleKey()), is(nullValue()));
assertThat(cache.estimatedSize(), is(2L));

if (context.isGuava()) {
cache.cleanUp();
}

long count = context.initialSize() - 1;
assertThat(cache, hasRemovalNotifications(context, count, RemovalCause.EXPIRED));
verifyWriter(context, (verifier, writer) -> verifier.deletions(count, RemovalCause.EXPIRED));
Expand Down Expand Up @@ -205,6 +209,10 @@ public void putAll_replace(Cache<Integer, Integer> cache, CacheContext context)
assertThat(cache.getIfPresent(context.middleKey()), is(nullValue()));
assertThat(cache.estimatedSize(), is(2L));

if (context.isGuava()) {
cache.cleanUp();
}

long count = context.initialSize() - 1;
assertThat(cache, hasRemovalNotifications(context, count, RemovalCause.EXPIRED));
verifyWriter(context, (verifier, writer) -> verifier.deletions(count, RemovalCause.EXPIRED));
Expand Down Expand Up @@ -295,9 +303,12 @@ public void invalidateAll_full(Cache<Integer, Integer> cache, CacheContext conte
context.ticker().advance(1, TimeUnit.MINUTES);
cache.invalidateAll();

long count = context.initialSize();
assertThat(cache, hasRemovalNotifications(context, count, RemovalCause.EXPIRED));
verifyWriter(context, (verifier, writer) -> verifier.deletions(count, RemovalCause.EXPIRED));
if (!context.isGuava()) {
// https://github.com/google/guava/issues/2101
long count = context.initialSize();
assertThat(cache, hasRemovalNotifications(context, count, RemovalCause.EXPIRED));
verifyWriter(context, (verifier, writer) -> verifier.deletions(count, RemovalCause.EXPIRED));
}
}

@Test(dataProvider = "caches", expectedExceptions = DeleteException.class)
Expand Down Expand Up @@ -428,7 +439,7 @@ public void refresh_writerFails(LoadingCache<Integer, Integer> cache, CacheConte
expireAfterAccess = {Expire.DISABLED, Expire.ONE_MINUTE},
expireAfterWrite = {Expire.DISABLED, Expire.ONE_MINUTE})
public void get(AsyncLoadingCache<Integer, Integer> cache, CacheContext context) {
context.ticker().advance(1, TimeUnit.SECONDS);
context.ticker().advance(2, TimeUnit.MINUTES);

cache.get(context.firstKey());
cache.get(context.middleKey(), k -> context.absentValue());
Expand Down Expand Up @@ -560,9 +571,12 @@ public void clear(Map<Integer, Integer> map, CacheContext context) {
context.ticker().advance(1, TimeUnit.MINUTES);
map.clear();

long count = context.initialSize();
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.EXPIRED));
verifyWriter(context, (verifier, writer) -> verifier.deletions(count, RemovalCause.EXPIRED));
if (!context.isGuava()) {
// https://github.com/google/guava/issues/2101
long count = context.initialSize();
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.EXPIRED));
verifyWriter(context, (verifier, writer) -> verifier.deletions(count, RemovalCause.EXPIRED));
}
}

@Test(dataProvider = "caches", expectedExceptions = DeleteException.class)
Expand Down Expand Up @@ -630,6 +644,10 @@ public void put_replace(Map<Integer, Integer> map, CacheContext context) {
assertThat(map.get(context.middleKey()), is(nullValue()));
assertThat(map.size(), is(2));

if (context.isGuava()) {
context.cleanUp();
}

long count = context.initialSize() - 1;
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.EXPIRED));
verifyWriter(context, (verifier, writer) -> verifier.deletions(count, RemovalCause.EXPIRED));
Expand Down Expand Up @@ -828,6 +846,10 @@ public void computeIfPresent(Map<Integer, Integer> map, CacheContext context) {
assertThat(map.computeIfPresent(key, (k, v) -> value), is(nullValue()));

assertThat(map.size(), is(0));
if (context.isGuava()) {
context.cleanUp();
}

long count = context.initialSize();
assertThat(map, hasRemovalNotifications(context, count, RemovalCause.EXPIRED));
verifyWriter(context, (verifier, writer) -> verifier.deletions(count, RemovalCause.EXPIRED));
Expand Down

0 comments on commit d26d0c9

Please sign in to comment.