Skip to content

Commit

Permalink
Roll back [the change that made LocalCache avoid synchronized](15…
Browse files Browse the repository at this point in the history 
…12730).

The `LocalCache` change led to [false reports of recursion during refresh](#6851 (comment)).

This CL keeps the new tests from the `LocalCache` change, which already would have passed before the CL. Ideally we would additionally include [tests that demonstrate the refresh problem](#6851 (comment)), but we'll need to see if the contributor can sign a CLA.

This rollback un-fixes #6845. Users of `common.cache` and virtual threads will likely have to wait until `synchronized` no longer pins threads. (And at that point, if not earlier, they should migrate to [Caffeine](https://github.com/ben-manes/caffeine) :))

RELNOTES=`cache`: Fixed a bug that could cause [false "recursive load" reports during refresh](#6851 (comment)).
PiperOrigin-RevId: 605069776
  • Loading branch information
@cpovirk
cpovirk authored and Google Java Core Libraries committed Feb 7, 2024
1 parent 476faf9 commit 0e1aebf
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 48 deletions.
31 changes: 7 additions & 24 deletions android/guava/src/com/google/common/cache/LocalCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2180,7 +2180,12 @@ V lockedGetOrLoad(K key, int hash, CacheLoader<? super K, V> loader) throws Exec

if (createNewEntry) {
try {
return loadSync(key, hash, loadingValueReference, loader);
// Synchronizes on the entry to allow failing fast when a recursive load is
// detected. This may be circumvented when an entry is copied, but will fail fast most
// of the time.
synchronized (e) {
return loadSync(key, hash, loadingValueReference, loader);
}
} finally {
statsCounter.recordMisses(1);
}
Expand All @@ -2196,22 +2201,7 @@ V waitForLoadingValue(ReferenceEntry<K, V> e, K key, ValueReference<K, V> valueR
throw new AssertionError();
}

// As of this writing, the only prod ValueReference implementation for which isLoading() is
// true is LoadingValueReference. (Note, however, that not all LoadingValueReference instances
// have isLoading()==true: LoadingValueReference has a subclass, ComputingValueReference, for
// which isLoading() is false!) However, that might change, and we already have a *test*
// implementation for which it doesn't hold. So we check instanceof to be safe.
if (valueReference instanceof LoadingValueReference) {
// We check whether the thread that is loading the entry is our current thread, which would
// mean that we are both loading and waiting for the entry. In this case, we fail fast
// instead of deadlocking.
checkState(
((LoadingValueReference<K, V>) valueReference).getLoadingThread()
!= Thread.currentThread(),
"Recursive load of: %s",
key);
}

checkState(!Thread.holdsLock(e), "Recursive load of: %s", key);
// don't consider expiration as we're concurrent with loading
try {
V value = valueReference.waitForValue();
Expand Down Expand Up @@ -3437,8 +3427,6 @@ static class LoadingValueReference<K, V> implements ValueReference<K, V> {
final SettableFuture<V> futureValue = SettableFuture.create();
final Stopwatch stopwatch = Stopwatch.createUnstarted();

final Thread loadingThread;

public LoadingValueReference() {
this(LocalCache.<K, V>unset());
}
Expand All @@ -3450,7 +3438,6 @@ public LoadingValueReference() {
*/
public LoadingValueReference(ValueReference<K, V> oldValue) {
this.oldValue = oldValue;
this.loadingThread = Thread.currentThread();
}

@Override
Expand Down Expand Up @@ -3554,10 +3541,6 @@ public ValueReference<K, V> copyFor(
ReferenceQueue<V> queue, @CheckForNull V value, ReferenceEntry<K, V> entry) {
return this;
}

Thread getLoadingThread() {
return this.loadingThread;
}
}

// Queues
Expand Down
31 changes: 7 additions & 24 deletions guava/src/com/google/common/cache/LocalCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2184,7 +2184,12 @@ V lockedGetOrLoad(K key, int hash, CacheLoader<? super K, V> loader) throws Exec

if (createNewEntry) {
try {
return loadSync(key, hash, loadingValueReference, loader);
// Synchronizes on the entry to allow failing fast when a recursive load is
// detected. This may be circumvented when an entry is copied, but will fail fast most
// of the time.
synchronized (e) {
return loadSync(key, hash, loadingValueReference, loader);
}
} finally {
statsCounter.recordMisses(1);
}
Expand All @@ -2200,22 +2205,7 @@ V waitForLoadingValue(ReferenceEntry<K, V> e, K key, ValueReference<K, V> valueR
throw new AssertionError();
}

// As of this writing, the only prod ValueReference implementation for which isLoading() is
// true is LoadingValueReference. (Note, however, that not all LoadingValueReference instances
// have isLoading()==true: LoadingValueReference has a subclass, ComputingValueReference, for
// which isLoading() is false!) However, that might change, and we already have a *test*
// implementation for which it doesn't hold. So we check instanceof to be safe.
if (valueReference instanceof LoadingValueReference) {
// We check whether the thread that is loading the entry is our current thread, which would
// mean that we are both loading and waiting for the entry. In this case, we fail fast
// instead of deadlocking.
checkState(
((LoadingValueReference<K, V>) valueReference).getLoadingThread()
!= Thread.currentThread(),
"Recursive load of: %s",
key);
}

checkState(!Thread.holdsLock(e), "Recursive load of: %s", key);
// don't consider expiration as we're concurrent with loading
try {
V value = valueReference.waitForValue();
Expand Down Expand Up @@ -3527,15 +3517,12 @@ static class LoadingValueReference<K, V> implements ValueReference<K, V> {
final SettableFuture<V> futureValue = SettableFuture.create();
final Stopwatch stopwatch = Stopwatch.createUnstarted();

final Thread loadingThread;

public LoadingValueReference() {
this(null);
}

public LoadingValueReference(@CheckForNull ValueReference<K, V> oldValue) {
this.oldValue = (oldValue == null) ? LocalCache.unset() : oldValue;
this.loadingThread = Thread.currentThread();
}

@Override
Expand Down Expand Up @@ -3660,10 +3647,6 @@ public ValueReference<K, V> copyFor(
ReferenceQueue<V> queue, @CheckForNull V value, ReferenceEntry<K, V> entry) {
return this;
}

Thread getLoadingThread() {
return this.loadingThread;
}
}

static class ComputingValueReference<K, V> extends LoadingValueReference<K, V> {
Expand Down

0 comments on commit 0e1aebf

Please sign in to comment.