Skip to content

Commit

Permalink
Fix compatibility between the cache compute methods and a load.
Browse files Browse the repository at this point in the history
Fixes #5348
Fixes #5342
Fixes #2827
Resolves underlying cause of #2108

RELNOTES=Fix compatibility between the cache compute methods and a load.
PiperOrigin-RevId: 357192480
  • Loading branch information
ben-manes authored and Google Java Core Libraries committed Feb 12, 2021
1 parent 7c566b5 commit 42bf4f4
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@

import static com.google.common.truth.Truth.assertThat;

import com.google.common.util.concurrent.UncheckedExecutionException;
import java.util.ArrayList;
import java.util.List;
import java.util.Queue;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.function.IntConsumer;
import java.util.stream.IntStream;
Expand Down Expand Up @@ -91,6 +97,31 @@ public void testComputeIfPresent() {
assertThat(cache.getIfPresent(key).split(delimiter)).hasLength(count + 1);
}

public void testComputeIfPresentRemove() {
List<RemovalNotification<Integer, Integer>> notifications = new ArrayList<>();
Cache<Integer, Integer> cache =
CacheBuilder.newBuilder()
.removalListener(
new RemovalListener<Integer, Integer>() {
@Override
public void onRemoval(RemovalNotification<Integer, Integer> notification) {
notifications.add(notification);
}
})
.build();
cache.put(1, 2);

// explicitly remove the existing value
cache.asMap().computeIfPresent(1, (key, value) -> null);
assertThat(notifications).hasSize(1);
CacheTesting.checkEmpty(cache);

// ensure no zombie entry remains
cache.asMap().computeIfPresent(1, (key, value) -> null);
assertThat(notifications).hasSize(1);
CacheTesting.checkEmpty(cache);
}

public void testUpdates() {
cache.put(key, "1");
// simultaneous update for same key, some null, some non-null
Expand All @@ -113,6 +144,41 @@ public void testCompute() {
assertEquals(0, cache.size());
}

public void testComputeWithLoad() {
Queue<RemovalNotification<String, String>> notifications = new ConcurrentLinkedQueue<>();
cache =
CacheBuilder.newBuilder()
.removalListener(
new RemovalListener<String, String>() {
@Override
public void onRemoval(RemovalNotification<String, String> notification) {
notifications.add(notification);
}
})
.expireAfterAccess(500000, TimeUnit.MILLISECONDS)
.maximumSize(count)
.build();

cache.put(key, "1");
// simultaneous load and deletion
doParallelCacheOp(
count,
n -> {
try {
cache.get(key, () -> key);
cache.asMap().compute(key, (k, v) -> null);
} catch (ExecutionException e) {
throw new UncheckedExecutionException(e);
}
});

CacheTesting.checkEmpty(cache);
for (RemovalNotification<String, String> entry : notifications) {
assertThat(entry.getKey()).isNotNull();
assertThat(entry.getValue()).isNotNull();
}
}

public void testComputeExceptionally() {
try {
doParallelCacheOp(
Expand Down
31 changes: 21 additions & 10 deletions guava/src/com/google/common/cache/LocalCache.java
Original file line number Diff line number Diff line change
Expand Up @@ -2188,7 +2188,7 @@ V waitForLoadingValue(ReferenceEntry<K, V> e, K key, ValueReference<K, V> valueR
V compute(K key, int hash, BiFunction<? super K, ? super V, ? extends V> function) {
ReferenceEntry<K, V> e;
ValueReference<K, V> valueReference = null;
LoadingValueReference<K, V> loadingValueReference = null;
ComputingValueReference<K, V> computingValueReference = null;
boolean createNewEntry = true;
V newValue;

Expand Down Expand Up @@ -2229,33 +2229,33 @@ V compute(K key, int hash, BiFunction<? super K, ? super V, ? extends V> functio

// note valueReference can be an existing value or even itself another loading value if
// the value for the key is already being computed.
loadingValueReference = new LoadingValueReference<>(valueReference);
computingValueReference = new ComputingValueReference<>(valueReference);

if (e == null) {
createNewEntry = true;
e = newEntry(key, hash, first);
e.setValueReference(loadingValueReference);
e.setValueReference(computingValueReference);
table.set(index, e);
} else {
e.setValueReference(loadingValueReference);
e.setValueReference(computingValueReference);
}

newValue = loadingValueReference.compute(key, function);
newValue = computingValueReference.compute(key, function);
if (newValue != null) {
if (valueReference != null && newValue == valueReference.get()) {
loadingValueReference.set(newValue);
computingValueReference.set(newValue);
e.setValueReference(valueReference);
recordWrite(e, 0, now); // no change in weight
return newValue;
}
try {
return getAndRecordStats(
key, hash, loadingValueReference, Futures.immediateFuture(newValue));
key, hash, computingValueReference, Futures.immediateFuture(newValue));
} catch (ExecutionException exception) {
throw new AssertionError("impossible; Futures.immediateFuture can't throw");
}
} else if (createNewEntry) {
removeLoadingValue(key, hash, loadingValueReference);
} else if (createNewEntry || valueReference.isLoading()) {
removeLoadingValue(key, hash, computingValueReference);
return null;
} else {
removeEntry(e, hash, RemovalCause.EXPLICIT);
Expand Down Expand Up @@ -3603,6 +3603,17 @@ public ValueReference<K, V> copyFor(
}
}

static class ComputingValueReference<K, V> extends LoadingValueReference<K, V> {
ComputingValueReference(ValueReference<K, V> oldValue) {
super(oldValue);
}

@Override
public boolean isLoading() {
return false;
}
}

// Queues

/**
Expand Down Expand Up @@ -3927,7 +3938,7 @@ long longSize() {
Segment<K, V>[] segments = this.segments;
long sum = 0;
for (int i = 0; i < segments.length; ++i) {
sum += Math.max(0, segments[i].count); // see https://github.com/google/guava/issues/2108
sum += segments[i].count;
}
return sum;
}
Expand Down

0 comments on commit 42bf4f4

Please sign in to comment.