Skip to content

Commit

Permalink
restores and tests the write-through behavior on entrySet's toArray
Browse files Browse the repository at this point in the history
The previous commit delegated an unbounded cache's toArray to the
underlying map. This works for keys and values, but an entry may be
modified. This wrote through to the map but bypassed being intercepted
for the removal listener. Additional unit tests now assert this to
avoid regressions, as the optimization seemed oddly missing and that
nagging feeling was a reminder of why.
  • Loading branch information
ben-manes committed Jan 4, 2025
1 parent f381814 commit a9846ae
Show file tree
Hide file tree
Showing 10 changed files with 167 additions and 92 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -959,16 +959,6 @@ public Iterator<Entry<K, V>> iterator() {
public Spliterator<Entry<K, V>> spliterator() {
return new EntrySpliterator<>(cache);
}

@Override
public Object[] toArray() {
return cache.data.entrySet().toArray();
}

@Override
public <T> T[] toArray(T[] array) {
return cache.data.entrySet().toArray(array);
}
}

/** An adapter to safely externalize the entry iterator. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import java.util.List;
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Objects;
import java.util.Set;
import java.util.Spliterators;
import java.util.concurrent.CompletableFuture;
Expand Down Expand Up @@ -2584,6 +2585,42 @@ public void entrySet_toArray(Map<Int, Int> map, CacheContext context) {

var func = map.entrySet().toArray(Map.Entry<?, ?>[]::new);
assertThat(func).asList().containsExactlyElementsIn(context.original().entrySet());

if (context.isCaffeine()) {
for (var entry : array) {
assertThat(entry).isInstanceOf(WriteThroughEntry.class);
}
for (var entry : ints) {
assertThat(entry).isInstanceOf(WriteThroughEntry.class);
}
for (var entry : func) {
assertThat(entry).isInstanceOf(WriteThroughEntry.class);
}
}
}

@CheckNoStats
@Test(dataProvider = "caches")
@CacheSpec(population = Population.FULL,
removalListener = { Listener.DISABLED, Listener.CONSUMING })
public void entrySet_toArray_writeThrough(Map<Int, Int> map, CacheContext context) {
var expected = new HashMap<>(context.original());
expected.putAll(Maps.toMap(context.firstMiddleLastKeys(), key -> context.absentValue()));

@SuppressWarnings("unchecked")
var array = (Map.Entry<Int, Int>[]) map.entrySet().toArray(new Map.Entry<?, ?>[0]);
for (var entry : array) {
var value = expected.get(entry.getKey());
if (!Objects.equals(entry.getValue(), value)) {
entry.setValue(value);
assertThat(entry.getValue()).isEqualTo(value);
}
}
assertThat(map).isEqualTo(expected);
assertThat(context).removalNotifications().withCause(REPLACED)
.contains(Maps.toMap(context.firstMiddleLastKeys(),
key -> requireNonNull(context.original().get(key))))
.exclusively();
}

@CheckNoStats
Expand Down Expand Up @@ -2939,6 +2976,9 @@ public void entrySet(Map<Int, Int> map, CacheContext context) {
assertThat(entries.remove(entry)).isTrue();
assertThat(entries.remove(entry)).isFalse();
assertThat(entries).doesNotContain(entry);
if (context.isCaffeine()) {
assertThat(entry).isInstanceOf(WriteThroughEntry.class);
}
});
assertThat(map).isExhaustivelyEmpty();
assertThat(context).removalNotifications().withCause(EXPLICIT)
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ public void getBiFunc_absent_cancelled(AsyncCache<Int, Int> cache, CacheContext
@Test(dataProvider = "caches")
public void getBiFunc_absent(AsyncCache<Int, Int> cache, CacheContext context) {
Int key = context.absentKey();
var value = cache.get(key, (k, executor) -> context.absentValue().asFuture());
var value = cache.get(key, (k, executor) -> context.absentValue().toFuture());
assertThat(value).succeedsWith(context.absentValue());
assertThat(context).stats().hits(0).misses(1).success(1).failures(0);
}
Expand Down Expand Up @@ -1071,7 +1071,7 @@ public void getAllBifunction_early_failure(AsyncCache<Int, Int> cache, CacheCont
@Test(dataProvider = "caches")
@CacheSpec(removalListener = { Listener.DISABLED, Listener.REJECTING })
public void put_nullKey(AsyncCache<Int, Int> cache, CacheContext context) {
var value = context.absentValue().asFuture();
var value = context.absentValue().toFuture();
assertThrows(NullPointerException.class, () -> cache.put(null, value));
}

Expand Down Expand Up @@ -1121,7 +1121,7 @@ public void put_insert_failure_after(AsyncCache<Int, Int> cache, CacheContext co
@Test(dataProvider = "caches")
@CacheSpec(removalListener = { Listener.DISABLED, Listener.REJECTING })
public void put_insert(AsyncCache<Int, Int> cache, CacheContext context) {
var value = context.absentValue().asFuture();
var value = context.absentValue().toFuture();
cache.put(context.absentKey(), value);
assertThat(cache).hasSize(context.initialSize() + 1);
assertThat(context).stats().hits(0).misses(0).success(1).failures(0);
Expand Down Expand Up @@ -1173,7 +1173,7 @@ public void put_replace_nullValue(AsyncCache<Int, Int> cache, CacheContext conte
public void put_replace_differentValue(AsyncCache<Int, Int> cache, CacheContext context) {
var replaced = new HashMap<Int, Int>();
for (Int key : context.firstMiddleLastKeys()) {
var newValue = context.absentValue().asFuture();
var newValue = context.absentValue().toFuture();
cache.put(key, newValue);
assertThat(cache).containsEntry(key, newValue);
replaced.put(key, context.original().get(key));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,7 @@ private static final class LoadAllException extends RuntimeException {
@CacheSpec(population = { Population.SINGLETON, Population.PARTIAL, Population.FULL })
public void put_replace(AsyncLoadingCache<Int, Int> cache, CacheContext context) {
var replaced = new HashMap<Int, Int>();
var value = context.absentValue().asFuture();
var value = context.absentValue().toFuture();
for (Int key : context.firstMiddleLastKeys()) {
cache.put(key, value);
assertThat(cache.get(key)).succeedsWith(context.absentValue());
Expand Down Expand Up @@ -629,7 +629,7 @@ public void refresh_current_inFlight(AsyncLoadingCache<Int, Int> cache, CacheCon
@Test(dataProvider = "caches")
@CacheSpec(compute = Compute.ASYNC, removalListener = Listener.CONSUMING)
public void refresh_current_sameInstance(CacheContext context) {
var future = context.absentValue().asFuture();
var future = context.absentValue().toFuture();
var cache = context.buildAsync((key, executor) -> future);

cache.put(context.absentKey(), future);
Expand All @@ -640,7 +640,7 @@ public void refresh_current_sameInstance(CacheContext context) {
@CacheSpec
@Test(dataProvider = "caches")
public void refresh_current_failed(AsyncLoadingCache<Int, Int> cache, CacheContext context) {
var future = context.absentValue().asFuture();
var future = context.absentValue().toFuture();
cache.put(context.absentKey(), future);

future.obtrudeException(new Exception());
Expand All @@ -662,7 +662,7 @@ public void refresh_current_removed(CacheContext context) {
return key;
});

cache.put(context.absentKey(), context.absentValue().asFuture());
cache.put(context.absentKey(), context.absentValue().toFuture());
cache.synchronous().refresh(context.absentKey());
await().untilTrue(started);

Expand All @@ -679,14 +679,14 @@ public void refresh_current_removed(CacheContext context) {

@Test
public void asyncLoadAll() {
AsyncCacheLoader<Int, Int> loader = (key, executor) -> key.negate().asFuture();
AsyncCacheLoader<Int, Int> loader = (key, executor) -> key.negate().toFuture();
assertThrows(UnsupportedOperationException.class, () ->
loader.asyncLoadAll(Set.of(), Runnable::run));
}

@Test
public void asyncReload() throws Exception {
AsyncCacheLoader<Int, Int> loader = (key, executor) -> key.negate().asFuture();
AsyncCacheLoader<Int, Int> loader = (key, executor) -> key.negate().toFuture();
var future = loader.asyncReload(Int.valueOf(1), Int.valueOf(2), Runnable::run);
assertThat(future).succeedsWith(-1);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ public void get(AsyncCache<Int, Int> cache, CacheContext context) {

cache.get(context.firstKey(), k -> k).join();
cache.get(context.middleKey(), k -> context.absentValue()).join();
cache.get(context.lastKey(), (k, executor) -> context.absentValue().asFuture()).join();
cache.get(context.lastKey(), (k, executor) -> context.absentValue().toFuture()).join();

assertThat(context).notifications().withCause(EXPIRED)
.contains(context.original()).exclusively();
Expand Down Expand Up @@ -573,7 +573,7 @@ public void getAll(AsyncCache<Int, Int> cache, CacheContext context) {
expireAfterWrite = {Expire.DISABLED, Expire.ONE_MINUTE})
public void put_insert(AsyncCache<Int, Int> cache, CacheContext context) {
context.ticker().advance(Duration.ofMinutes(1));
cache.put(context.firstKey(), intern(context.absentValue().asFuture()));
cache.put(context.firstKey(), intern(context.absentValue().toFuture()));

runVariableExpiration(context);
assertThat(cache).hasSize(1);
Expand Down Expand Up @@ -613,7 +613,7 @@ public void put_insert_async(AsyncCache<Int, Int> cache, CacheContext context) {
expireAfterAccess = {Expire.DISABLED, Expire.ONE_MINUTE},
expireAfterWrite = {Expire.DISABLED, Expire.ONE_MINUTE})
public void put_replace(AsyncCache<Int, Int> cache, CacheContext context) {
var future = context.absentValue().asFuture();
var future = context.absentValue().toFuture();
context.ticker().advance(Duration.ofSeconds(30));

cache.put(context.firstKey(), future);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ public void put_replace(Cache<Int, Int> cache, CacheContext context) {
@CacheSpec(population = Population.FULL,
expiryTime = Expire.ONE_MINUTE, expiry = CacheExpiry.CREATE)
public void put_replace(AsyncCache<Int, Int> cache, CacheContext context) {
var future = context.absentValue().asFuture();
var future = context.absentValue().toFuture();
context.ticker().advance(Duration.ofSeconds(30));

cache.put(context.firstKey(), future);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ public void put_async(AsyncCache<Int, Int> cache, CacheContext context) {
Int key = context.absentKey();
context.clear();
GcFinalization.awaitFullGc();
cache.put(key, context.absentValue().asFuture());
cache.put(key, context.absentValue().toFuture());

assertThat(cache).hasSize(1);
assertThat(context).notifications().withCause(COLLECTED)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public void refreshIfNeeded_nonblocking(CacheContext context) {
public CompletableFuture<Int> asyncReload(Int key, Int oldValue, Executor executor) {
reloads.incrementAndGet();
await().untilTrue(refresh);
return oldValue.add(1).asFuture();
return oldValue.add(1).toFuture();
}
});
cache.put(key, original);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ public Int add(Int i) {
return add(i.value);
}

/** Returns a completed future of this value. */
public CompletableFuture<Int> asFuture() {
/** Returns a new completed future for this value. */
public CompletableFuture<Int> toFuture() {
return CompletableFuture.completedFuture(this);
}

Expand Down Expand Up @@ -155,9 +155,9 @@ public static Map<Int, Int> mapOf(int... mappings) {
return map;
}

/** Returns a completed future of the value, possibly cached. */
/** Returns a new completed future of the (possibly cached) value. */
public static CompletableFuture<Int> futureOf(int i) {
return valueOf(i).asFuture();
return valueOf(i).toFuture();
}

/** Returns a preallocated range of {@code Int} instances */
Expand Down

0 comments on commit a9846ae

Please sign in to comment.