Cache returns already stale value when refreshing

[GoogleCodeExporter]

When using a LoadingCache with refreshAfterWrite, if the given 
CacheLoader.reload() is asynchronous and returns too fast, then the following 
scenario occurs indeterministically:
0. The value already exist in the cache but is eligible for refresh.
1. A call to get() starts a refresh, the asynchronous reload() returns a future 
that is already done when Cache checks this and therefore returns synchronously 
with the result.
1a. Meanwhile the future's callback starts to put the new value into the cache.
2. An immediate second call to get() occurs before 1a. would complete, 
therefore returns the value described in 0.

The following test case reproduces the problem after some iterations:

import java.util.concurrent.Callable;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;

import org.junit.Assert;
import org.junit.Test;

import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.common.util.concurrent.MoreExecutors;

public class TooFastRefresh {
    private ListeningExecutorService EXECUTOR = MoreExecutors.listeningDecorator(Executors.newCachedThreadPool());

    @Test
    public void test() throws Exception {
        final AtomicInteger counter = new AtomicInteger();

        LoadingCache<String, Integer> cache = CacheBuilder.newBuilder()
        .refreshAfterWrite(10, TimeUnit.MILLISECONDS)
        .build(new CacheLoader<String, Integer>() {
            @Override
            public Integer load(String key) {
                return counter.incrementAndGet();
            }

            @Override
            public ListenableFuture<Integer> reload(final String key, Integer prev) {
                return EXECUTOR.submit(new Callable<Integer>() {
                    @Override
                    public Integer call() throws Exception {
                        return load(key);
                    }
                });
            }
        });

        cache.get("");
        for(int i=0; i<50; i++) {
            Thread.sleep(20);
            Integer first = cache.get("");
            Integer second = cache.get("");
            Assert.assertEquals(first, second);
        }

        EXECUTOR.shutdown();
    }
}

Original issue reported on code.google.com by harmathdenes on 14 Mar 2013 at 5:34

[GoogleCodeExporter]

In version 14 r0202617129f4, the problem appears to be in LocalCache.refresh():

      ListenableFuture<V> result = loadAsync(key, hash, loadingValueReference, loader);
      if (result.isDone()) {
        try {
          return Uninterruptibles.getUninterruptibly(result);
        } catch (Throwable t) {
          // don't let refresh exceptions propagate; error was already logged
        }
      }

Maybe the solution would be if the method checked if the new value is already 
in the cache.

Original comment by harmathdenes on 14 Mar 2013 at 5:43

[GoogleCodeExporter]

I haven't tested it yet myself, but this looks legit.  Investigating fixes.

Original comment by wasserman.louis on 14 Mar 2013 at 10:04

• Changed state: Accepted
• Added labels: Type-Defect

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<issue id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:12

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:18

• Changed state: Migrated

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:08

• Added labels: Restrict-AddIssueComment-Commit