From cb9bf093efb500c9768cae2eb10ae0b0890566ac Mon Sep 17 00:00:00 2001 From: cpovirk Date: Thu, 6 Oct 2022 08:14:17 -0700 Subject: [PATCH] Avoid creating an entry whose key is `null` at construction time. In the case of an entry whose key is stored in a `WeakReference`, the resulting entry will never be enqueued in the `ReferenceQueue`, so I'm not sure that it will necessarily ever be removed from the cache. Compare a similar change to `MapMakerInternalMap` that was part of cl/479157599. I get the impression that there may be a similar situation with weak _values_, but I'm not biting that off right now. As always, we [recommend](https://guava.dev/CacheBuilder) that you use [Caffeine](https://github.com/ben-manes/caffeine/wiki) instead of Guava's `cache` library. RELNOTES=n/a PiperOrigin-RevId: 479318112 --- .../com/google/common/cache/LocalCache.java | 59 +++++++++++++------ .../com/google/common/cache/LocalCache.java | 59 +++++++++++++------ 2 files changed, 82 insertions(+), 36 deletions(-) diff --git a/android/guava/src/com/google/common/cache/LocalCache.java b/android/guava/src/com/google/common/cache/LocalCache.java index 02c36671f1c3..6d9ec0533780 100644 --- a/android/guava/src/com/google/common/cache/LocalCache.java +++ b/android/guava/src/com/google/common/cache/LocalCache.java @@ -70,7 +70,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; @@ -456,8 +455,11 @@ ReferenceEntry newEntry( @Override ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - ReferenceEntry newEntry = super.copyEntry(segment, original, newNext); + Segment segment, + ReferenceEntry original, + ReferenceEntry newNext, + K key) { + ReferenceEntry newEntry = super.copyEntry(segment, original, newNext, key); copyAccessEntry(original, newEntry); return newEntry; } @@ -471,8 +473,11 @@ ReferenceEntry newEntry( @Override ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - ReferenceEntry newEntry = super.copyEntry(segment, original, newNext); + Segment segment, + ReferenceEntry original, + ReferenceEntry newNext, + K key) { + ReferenceEntry newEntry = super.copyEntry(segment, original, newNext, key); copyWriteEntry(original, newEntry); return newEntry; } @@ -486,8 +491,11 @@ ReferenceEntry newEntry( @Override ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - ReferenceEntry newEntry = super.copyEntry(segment, original, newNext); + Segment segment, + ReferenceEntry original, + ReferenceEntry newNext, + K key) { + ReferenceEntry newEntry = super.copyEntry(segment, original, newNext, key); copyAccessEntry(original, newEntry); copyWriteEntry(original, newEntry); return newEntry; @@ -509,8 +517,11 @@ ReferenceEntry newEntry( @Override ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - ReferenceEntry newEntry = super.copyEntry(segment, original, newNext); + Segment segment, + ReferenceEntry original, + ReferenceEntry newNext, + K key) { + ReferenceEntry newEntry = super.copyEntry(segment, original, newNext, key); copyAccessEntry(original, newEntry); return newEntry; } @@ -524,8 +535,11 @@ ReferenceEntry newEntry( @Override ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - ReferenceEntry newEntry = super.copyEntry(segment, original, newNext); + Segment segment, + ReferenceEntry original, + ReferenceEntry newNext, + K key) { + ReferenceEntry newEntry = super.copyEntry(segment, original, newNext, key); copyWriteEntry(original, newEntry); return newEntry; } @@ -539,8 +553,11 @@ ReferenceEntry newEntry( @Override ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - ReferenceEntry newEntry = super.copyEntry(segment, original, newNext); + Segment segment, + ReferenceEntry original, + ReferenceEntry newNext, + K key) { + ReferenceEntry newEntry = super.copyEntry(segment, original, newNext, key); copyAccessEntry(original, newEntry); copyWriteEntry(original, newEntry); return newEntry; @@ -588,13 +605,18 @@ abstract ReferenceEntry newEntry( /** * Copies an entry, assigning it a new {@code next} entry. * - * @param original the entry to copy + * @param original the entry to copy. But avoid calling {@code getKey} on it: Instead, use the + * {@code key} parameter. That way, we prevent the key from being garbage collected in the + * case of weak keys. If we create a new entry with a key that is null at construction time, + * we're not sure if entry will necessarily ever be garbage collected. * @param newNext entry in the same bucket + * @param key the key to copy from the original entry to the new one. Use this in preference to + * {@code original.getKey()}. */ // Guarded By Segment.this ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - return newEntry(segment, original.getKey(), original.getHash(), newNext); + Segment segment, ReferenceEntry original, ReferenceEntry newNext, K key) { + return newEntry(segment, key, original.getHash(), newNext); } // Guarded By Segment.this @@ -1983,7 +2005,8 @@ ReferenceEntry newEntry(K key, int hash, @CheckForNull ReferenceEntry copyEntry(ReferenceEntry original, ReferenceEntry newNext) { - if (original.getKey() == null) { + K key = original.getKey(); + if (key == null) { // key collected return null; } @@ -1995,7 +2018,7 @@ ReferenceEntry copyEntry(ReferenceEntry original, ReferenceEntry newEntry = map.entryFactory.copyEntry(this, original, newNext); + ReferenceEntry newEntry = map.entryFactory.copyEntry(this, original, newNext, key); newEntry.setValueReference(valueReference.copyFor(this.valueReferenceQueue, value, newEntry)); return newEntry; } diff --git a/guava/src/com/google/common/cache/LocalCache.java b/guava/src/com/google/common/cache/LocalCache.java index c95a48f4d8f4..e4f769be87ff 100644 --- a/guava/src/com/google/common/cache/LocalCache.java +++ b/guava/src/com/google/common/cache/LocalCache.java @@ -69,7 +69,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; @@ -459,8 +458,11 @@ ReferenceEntry newEntry( @Override ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - ReferenceEntry newEntry = super.copyEntry(segment, original, newNext); + Segment segment, + ReferenceEntry original, + ReferenceEntry newNext, + K key) { + ReferenceEntry newEntry = super.copyEntry(segment, original, newNext, key); copyAccessEntry(original, newEntry); return newEntry; } @@ -474,8 +476,11 @@ ReferenceEntry newEntry( @Override ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - ReferenceEntry newEntry = super.copyEntry(segment, original, newNext); + Segment segment, + ReferenceEntry original, + ReferenceEntry newNext, + K key) { + ReferenceEntry newEntry = super.copyEntry(segment, original, newNext, key); copyWriteEntry(original, newEntry); return newEntry; } @@ -489,8 +494,11 @@ ReferenceEntry newEntry( @Override ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - ReferenceEntry newEntry = super.copyEntry(segment, original, newNext); + Segment segment, + ReferenceEntry original, + ReferenceEntry newNext, + K key) { + ReferenceEntry newEntry = super.copyEntry(segment, original, newNext, key); copyAccessEntry(original, newEntry); copyWriteEntry(original, newEntry); return newEntry; @@ -512,8 +520,11 @@ ReferenceEntry newEntry( @Override ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - ReferenceEntry newEntry = super.copyEntry(segment, original, newNext); + Segment segment, + ReferenceEntry original, + ReferenceEntry newNext, + K key) { + ReferenceEntry newEntry = super.copyEntry(segment, original, newNext, key); copyAccessEntry(original, newEntry); return newEntry; } @@ -527,8 +538,11 @@ ReferenceEntry newEntry( @Override ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - ReferenceEntry newEntry = super.copyEntry(segment, original, newNext); + Segment segment, + ReferenceEntry original, + ReferenceEntry newNext, + K key) { + ReferenceEntry newEntry = super.copyEntry(segment, original, newNext, key); copyWriteEntry(original, newEntry); return newEntry; } @@ -542,8 +556,11 @@ ReferenceEntry newEntry( @Override ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - ReferenceEntry newEntry = super.copyEntry(segment, original, newNext); + Segment segment, + ReferenceEntry original, + ReferenceEntry newNext, + K key) { + ReferenceEntry newEntry = super.copyEntry(segment, original, newNext, key); copyAccessEntry(original, newEntry); copyWriteEntry(original, newEntry); return newEntry; @@ -591,13 +608,18 @@ abstract ReferenceEntry newEntry( /** * Copies an entry, assigning it a new {@code next} entry. * - * @param original the entry to copy + * @param original the entry to copy. But avoid calling {@code getKey} on it: Instead, use the + * {@code key} parameter. That way, we prevent the key from being garbage collected in the + * case of weak keys. If we create a new entry with a key that is null at construction time, + * we're not sure if entry will necessarily ever be garbage collected. * @param newNext entry in the same bucket + * @param key the key to copy from the original entry to the new one. Use this in preference to + * {@code original.getKey()}. */ // Guarded By Segment.this ReferenceEntry copyEntry( - Segment segment, ReferenceEntry original, ReferenceEntry newNext) { - return newEntry(segment, original.getKey(), original.getHash(), newNext); + Segment segment, ReferenceEntry original, ReferenceEntry newNext, K key) { + return newEntry(segment, key, original.getHash(), newNext); } // Guarded By Segment.this @@ -1983,7 +2005,8 @@ ReferenceEntry newEntry(K key, int hash, @Nullable ReferenceEntry ne */ @GuardedBy("this") ReferenceEntry copyEntry(ReferenceEntry original, ReferenceEntry newNext) { - if (original.getKey() == null) { + K key = original.getKey(); + if (key == null) { // key collected return null; } @@ -1995,7 +2018,7 @@ ReferenceEntry copyEntry(ReferenceEntry original, ReferenceEntry newEntry = map.entryFactory.copyEntry(this, original, newNext); + ReferenceEntry newEntry = map.entryFactory.copyEntry(this, original, newNext, key); newEntry.setValueReference(valueReference.copyFor(this.valueReferenceQueue, value, newEntry)); return newEntry; }