refresh() appears to not use lock

[eddified]

Hello. Thanks for the great software. It is quite useful.

I have found an issue (tested in 2.7.0), which is not necessarily a bug. I'd like to get your thoughts on what to do about it. When using AsyncLoadingCache, the value for a given key is correctly loaded only once when tested under concurrency (many threads asking for the same value at the same time, results in only a single load call to the given CacheLoader). But when using cache.synchronous().refresh(), the behavior deviates from its single-load guarantee -- that is, multiple loads become possible at the same time. I would expect multiple calls to refresh() at the same time to only trigger a single refresh.

Here is the code:

  @Test
  public void testRefresh() throws Exception {
    int numThreads = 50;
    Integer testKey = 2;
    AtomicInteger numLoaded = new AtomicInteger(); // this is the critical counter
    AtomicInteger numDoneGet = new AtomicInteger();
    AtomicInteger numDone = new AtomicInteger();

    AsyncLoadingCache<Integer, Integer> cache = Caffeine.newBuilder()
        .refreshAfterWrite(1, TimeUnit.DAYS)
        .expireAfterWrite(2, TimeUnit.DAYS)
        .buildAsync((key) -> {
          logger.info("loading");
          numLoaded.getAndIncrement(); // this is the critical counter
          try {
            Thread.sleep(2000);
          } catch (InterruptedException e) {
            e.printStackTrace();
            fail("the test did not run according to plan, please re-try");
          }
          logger.info("loaded");
          return key;
        });

    // This gate is to make all of the threads wait at the same point, just before
    // calling cache.get(). Then, once they are all ready to go, we let the race begin.
    // The goal is to attempt to cause multiple loads to occur due to possible race condition.
    // This test is to ensure there is *not* a race condition.
    // The "+ 1" for number of parties is to account for the main thread (this one).
    final CyclicBarrier gate = new CyclicBarrier(numThreads + 1);

    for (int i = 0; i < numThreads; i++) {
      Thread thread = new Thread(() -> {
        try {
          gate.await();
        } catch (InterruptedException | BrokenBarrierException e) {
          e.printStackTrace();
          fail("the test did not run according to plan, please re-try");
        }

        @NonNull CompletableFuture<Integer> future = cache.get(testKey);
        numDoneGet.getAndIncrement();
        if (future.isDone()) {
          numDone.getAndIncrement();
        }

        future.thenAccept((key) -> {
          logger.warn("refreshing");
          cache.synchronous().refresh(testKey);
        });
      });
      thread.start();
    }
    gate.await();

    Thread.sleep(2500);

    logger.info("numDone " + numDone.get());
    logger.info("numLoaded " + numLoaded.get());
    logger.info("numDoneGet " + numDoneGet.get());

    // ensure the test is running as expected
    assertEquals(numThreads, numDoneGet.get());
    // ensure load only happened once
    assertEquals(1, numLoaded.get());
  }

This test on my machine shows an average of 8 loads happening after the initial successful load. I'd expect that to be 1 or 2 loads in the test above, but never any running simultaneously. (The log output shows they are running simultaneously).

A) is this expected? is it a bug?
B) is there a simple workaround? I've written a solution that uses an additional ConcurrentHashMap to keep track of which keys we are currently loading values for, which does work for this test case -- but is there a better solution using the library itself?
C) tangentially, I've noticed that for AsyncLoadingCache, .synchronous().refresh() is actually done asynchronously, which may be a tiny gotcha for some... But it's not the focus of this github issue.. I'm merely noting that for the purposes of this issue, this behavior is treated as known but expected.

If I've said anything incorrect here, I'd like correction.

[ben-manes]

Sorry, this has been in my backlog and I never got to it, e.g. #236 (comment).

a) LoadingCache#refresh() doesn't make a single load guarantee, though it is desirable behavior.
b) Guava does it using low-level access to its hashtable. That's not an option for us, so I had planned on using an auxiliary map for this as you suggested.
c) refresh() is spec'd as asynchronous for a synchronous cache. In Guava this is async but on the calling thread by default, whereas in Caffeine we delegate to the executor. In both cases, it's not synchronous as they allow other threads to do the work. That's why there is no AsyncLoadingCache#refresh as it wouldn't change the contract, so it makes sense to drill into the other interface. I agree as a line of code it could look confusing when scanning a large chunk.

[TuomasKiviaho]

This is outside of the ticket but are there plans to provide also a synchronous refresh or at least a variant with CompletionStage as a return value instead of only logging (and swallowing). The async behavior came to me as a surprise as I had't read the javadocs and therefore I first noticed this when I got a stacktrace from an unexpected thread.

[ben-manes]

Yes, see #143.

[ben-manes]

Released in 3.0