Skip to content

Commit

Permalink
Ensure the correct removal notification is published.
Browse files Browse the repository at this point in the history 
Fixes #2101.

Pull request #2122.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=109009756
  • Loading branch information
@ben-manes @cpovirk
ben-manes authored and cpovirk committed Nov 30, 2015
1 parent 69a42c5 commit 0a686a6
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 34 deletions.
18 changes: 18 additions & 0 deletions guava-tests/test/com/google/common/cache/CacheExpirationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import com.google.common.cache.TestingCacheLoaders.IdentityLoader;
import com.google.common.cache.TestingRemovalListeners.CountingRemovalListener;
import com.google.common.cache.TestingRemovalListeners.QueuingRemovalListener;
import com.google.common.collect.Iterators;
import com.google.common.testing.FakeTicker;
import com.google.common.util.concurrent.Callables;
Expand All @@ -31,6 +32,7 @@
import java.util.List;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;

/**
Expand Down Expand Up @@ -394,6 +396,22 @@ public void testExpirationOrder_writeAccess() throws ExecutionException {
assertThat(keySet).containsExactly(3, 6);
}

public void testExpiration_invalidateAll() {
FakeTicker ticker = new FakeTicker();
QueuingRemovalListener<Integer, Integer> listener =
TestingRemovalListeners.queuingRemovalListener();
Cache<Integer, Integer> cache = CacheBuilder.newBuilder()
.expireAfterAccess(1, TimeUnit.MINUTES)
.removalListener(listener)
.ticker(ticker)
.build();
cache.put(1, 1);
ticker.advance(10, TimeUnit.MINUTES);
cache.invalidateAll();

assertThat(listener.poll().getCause()).isEqualTo(RemovalCause.EXPIRED);
}

private void runRemovalScheduler(LoadingCache<String, Integer> cache,
CountingRemovalListener<String, Integer> removalListener,
WatchedCreatorLoader loader,
Expand Down
77 changes: 43 additions & 34 deletions guava/src/com/google/common/cache/LocalCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@
import java.util.Collection;
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.NoSuchElementException;
import java.util.Queue;
import java.util.Set;
Expand Down Expand Up @@ -2240,11 +2239,13 @@ V lockedGetOrLoad(K key, int hash, CacheLoader<? super K, V> loader)
} else {
V value = valueReference.get();
if (value == null) {
enqueueNotification(entryKey, hash, valueReference, RemovalCause.COLLECTED);
enqueueNotification(entryKey, hash, value,
valueReference.getWeight(), RemovalCause.COLLECTED);
} else if (map.isExpired(e, now)) {
// This is a duplicate check, as preWriteCleanup already purged expired
// entries, but let's accomodate an incorrect expiration queue.
enqueueNotification(entryKey, hash, valueReference, RemovalCause.EXPIRED);
enqueueNotification(entryKey, hash, value,
valueReference.getWeight(), RemovalCause.EXPIRED);
} else {
recordLockedRead(e, now);
statsCounter.recordHits(1);
Expand Down Expand Up @@ -2332,7 +2333,7 @@ ListenableFuture<V> loadAsync(final K key, final int hash,
@Override
public void run() {
try {
V newValue = getAndRecordStats(key, hash, loadingValueReference, loadingFuture);
getAndRecordStats(key, hash, loadingValueReference, loadingFuture);
} catch (Throwable t) {
logger.log(Level.WARNING, "Exception thrown during refresh", t);
loadingValueReference.setException(t);
Expand Down Expand Up @@ -2639,19 +2640,13 @@ void expireEntries(long now) {
// eviction

@GuardedBy("this")
void enqueueNotification(ReferenceEntry<K, V> entry, RemovalCause cause) {
enqueueNotification(entry.getKey(), entry.getHash(), entry.getValueReference(), cause);
}

@GuardedBy("this")
void enqueueNotification(@Nullable K key, int hash, ValueReference<K, V> valueReference,
void enqueueNotification(@Nullable K key, int hash, @Nullable V value, int weight,
RemovalCause cause) {
totalWeight -= valueReference.getWeight();
totalWeight -= weight;
if (cause.wasEvicted()) {
statsCounter.recordEviction();
}
if (map.removalNotificationQueue != DISCARDING_QUEUE) {
V value = valueReference.get();
RemovalNotification<K, V> notification = RemovalNotification.create(key, value, cause);
map.removalNotificationQueue.offer(notification);
}
Expand Down Expand Up @@ -2865,7 +2860,8 @@ V put(K key, int hash, V value, boolean onlyIfAbsent) {
if (entryValue == null) {
++modCount;
if (valueReference.isActive()) {
enqueueNotification(key, hash, valueReference, RemovalCause.COLLECTED);
enqueueNotification(key, hash, entryValue,
valueReference.getWeight(), RemovalCause.COLLECTED);
setValue(e, key, value, now);
newCount = this.count; // count remains unchanged
} else {
Expand All @@ -2884,7 +2880,8 @@ V put(K key, int hash, V value, boolean onlyIfAbsent) {
} else {
// clobber existing entry, count remains unchanged
++modCount;
enqueueNotification(key, hash, valueReference, RemovalCause.REPLACED);
enqueueNotification(key, hash, entryValue,
valueReference.getWeight(), RemovalCause.REPLACED);
setValue(e, key, value, now);
evictEntries(e);
return entryValue;
Expand Down Expand Up @@ -3001,7 +2998,7 @@ boolean replace(K key, int hash, V oldValue, V newValue) {
int newCount = this.count - 1;
++modCount;
ReferenceEntry<K, V> newFirst = removeValueFromChain(
first, e, entryKey, hash, valueReference, RemovalCause.COLLECTED);
first, e, entryKey, hash, entryValue, valueReference, RemovalCause.COLLECTED);
newCount = this.count - 1;
table.set(index, newFirst);
this.count = newCount; // write-volatile
Expand All @@ -3011,7 +3008,8 @@ boolean replace(K key, int hash, V oldValue, V newValue) {

if (map.valueEquivalence.equivalent(oldValue, entryValue)) {
++modCount;
enqueueNotification(key, hash, valueReference, RemovalCause.REPLACED);
enqueueNotification(key, hash, entryValue,
valueReference.getWeight(), RemovalCause.REPLACED);
setValue(e, key, newValue, now);
evictEntries(e);
return true;
Expand Down Expand Up @@ -3054,7 +3052,7 @@ V replace(K key, int hash, V newValue) {
int newCount = this.count - 1;
++modCount;
ReferenceEntry<K, V> newFirst = removeValueFromChain(
first, e, entryKey, hash, valueReference, RemovalCause.COLLECTED);
first, e, entryKey, hash, entryValue, valueReference, RemovalCause.COLLECTED);
newCount = this.count - 1;
table.set(index, newFirst);
this.count = newCount; // write-volatile
Expand All @@ -3063,7 +3061,8 @@ V replace(K key, int hash, V newValue) {
}

++modCount;
enqueueNotification(key, hash, valueReference, RemovalCause.REPLACED);
enqueueNotification(key, hash, entryValue,
valueReference.getWeight(), RemovalCause.REPLACED);
setValue(e, key, newValue, now);
evictEntries(e);
return entryValue;
Expand Down Expand Up @@ -3108,7 +3107,7 @@ V remove(Object key, int hash) {

++modCount;
ReferenceEntry<K, V> newFirst = removeValueFromChain(
first, e, entryKey, hash, valueReference, cause);
first, e, entryKey, hash, entryValue, valueReference, cause);
newCount = this.count - 1;
table.set(index, newFirst);
this.count = newCount; // write-volatile
Expand Down Expand Up @@ -3154,7 +3153,7 @@ boolean storeLoadedValue(K key, int hash, LoadingValueReference<K, V> oldValueRe
if (oldValueReference.isActive()) {
RemovalCause cause =
(entryValue == null) ? RemovalCause.COLLECTED : RemovalCause.REPLACED;
enqueueNotification(key, hash, oldValueReference, cause);
enqueueNotification(key, hash, entryValue, oldValueReference.getWeight(), cause);
newCount--;
}
setValue(e, key, newValue, now);
Expand All @@ -3164,8 +3163,7 @@ boolean storeLoadedValue(K key, int hash, LoadingValueReference<K, V> oldValueRe
}

// the loaded value was already clobbered
valueReference = new WeightedStrongValueReference<K, V>(newValue, 0);
enqueueNotification(key, hash, valueReference, RemovalCause.REPLACED);
enqueueNotification(key, hash, newValue, 0, RemovalCause.REPLACED);
return false;
}
}
Expand Down Expand Up @@ -3213,7 +3211,7 @@ boolean remove(Object key, int hash, Object value) {

++modCount;
ReferenceEntry<K, V> newFirst = removeValueFromChain(
first, e, entryKey, hash, valueReference, cause);
first, e, entryKey, hash, entryValue, valueReference, cause);
newCount = this.count - 1;
table.set(index, newFirst);
this.count = newCount; // write-volatile
Expand All @@ -3232,12 +3230,21 @@ void clear() {
if (count != 0) { // read-volatile
lock();
try {
long now = map.ticker.read();
preWriteCleanup(now);

AtomicReferenceArray<ReferenceEntry<K, V>> table = this.table;
for (int i = 0; i < table.length(); ++i) {
for (ReferenceEntry<K, V> e = table.get(i); e != null; e = e.getNext()) {
// Loading references aren't actually in the map yet.
if (e.getValueReference().isActive()) {
enqueueNotification(e, RemovalCause.EXPLICIT);
K key = e.getKey();
V value = e.getValueReference().get();
RemovalCause cause = (key == null || value == null)
? RemovalCause.COLLECTED
: RemovalCause.EXPLICIT;
enqueueNotification(key, e.getHash(), value,
e.getValueReference().getWeight(), cause);
}
}
}
Expand All @@ -3261,9 +3268,9 @@ void clear() {
@GuardedBy("this")
@Nullable
ReferenceEntry<K, V> removeValueFromChain(ReferenceEntry<K, V> first,
ReferenceEntry<K, V> entry, @Nullable K key, int hash, ValueReference<K, V> valueReference,
RemovalCause cause) {
enqueueNotification(key, hash, valueReference, cause);
ReferenceEntry<K, V> entry, @Nullable K key, int hash, V value,
ValueReference<K, V> valueReference, RemovalCause cause) {
enqueueNotification(key, hash, value, valueReference.getWeight(), cause);
writeQueue.remove(entry);
accessQueue.remove(entry);

Expand Down Expand Up @@ -3296,7 +3303,8 @@ ReferenceEntry<K, V> removeEntryFromChain(ReferenceEntry<K, V> first,

@GuardedBy("this")
void removeCollectedEntry(ReferenceEntry<K, V> entry) {
enqueueNotification(entry, RemovalCause.COLLECTED);
enqueueNotification(entry.getKey(), entry.getHash(), entry.getValueReference().get(),
entry.getValueReference().getWeight(), RemovalCause.COLLECTED);
writeQueue.remove(entry);
accessQueue.remove(entry);
}
Expand All @@ -3315,8 +3323,8 @@ boolean reclaimKey(ReferenceEntry<K, V> entry, int hash) {
for (ReferenceEntry<K, V> e = first; e != null; e = e.getNext()) {
if (e == entry) {
++modCount;
ReferenceEntry<K, V> newFirst = removeValueFromChain(
first, e, e.getKey(), hash, e.getValueReference(), RemovalCause.COLLECTED);
ReferenceEntry<K, V> newFirst = removeValueFromChain(first, e, e.getKey(), hash,
e.getValueReference().get(), e.getValueReference(), RemovalCause.COLLECTED);
newCount = this.count - 1;
table.set(index, newFirst);
this.count = newCount; // write-volatile
Expand Down Expand Up @@ -3349,8 +3357,8 @@ boolean reclaimValue(K key, int hash, ValueReference<K, V> valueReference) {
ValueReference<K, V> v = e.getValueReference();
if (v == valueReference) {
++modCount;
ReferenceEntry<K, V> newFirst = removeValueFromChain(
first, e, entryKey, hash, valueReference, RemovalCause.COLLECTED);
ReferenceEntry<K, V> newFirst = removeValueFromChain(first, e, entryKey, hash,
valueReference.get(), valueReference, RemovalCause.COLLECTED);
newCount = this.count - 1;
table.set(index, newFirst);
this.count = newCount; // write-volatile
Expand Down Expand Up @@ -3401,6 +3409,7 @@ boolean removeLoadingValue(K key, int hash, LoadingValueReference<K, V> valueRef
}
}

@VisibleForTesting
@GuardedBy("this")
boolean removeEntry(ReferenceEntry<K, V> entry, int hash, RemovalCause cause) {
int newCount = this.count - 1;
Expand All @@ -3411,8 +3420,8 @@ boolean removeEntry(ReferenceEntry<K, V> entry, int hash, RemovalCause cause) {
for (ReferenceEntry<K, V> e = first; e != null; e = e.getNext()) {
if (e == entry) {
++modCount;
ReferenceEntry<K, V> newFirst = removeValueFromChain(
first, e, e.getKey(), hash, e.getValueReference(), cause);
ReferenceEntry<K, V> newFirst = removeValueFromChain(first, e, e.getKey(), hash,
e.getValueReference().get(), e.getValueReference(), cause);
newCount = this.count - 1;
table.set(index, newFirst);
this.count = newCount; // write-volatile
Expand Down

0 comments on commit 0a686a6

Please sign in to comment.