Guava: LocalCache computeIfAbsent updates write timestamp on existing entries, affecting expireAfterWrite caches

[an-cwi]

It seems that there is a bug in LocalCache's implementation of Map.computeIfAbsent(K, Function<? super K, ? extends V>), specifically affecting an expireAfterWrite cache.

Expected behaviour:
If there is an existing entry for key K, calling computeIfAbsent(K, ...) should not modify the existing entry for K (unless the entry is expired and cleaned up). _In particular, the write timestamp of entry K should not be changed._ Of course, if there is no existing entry for K, a new value is computed and a write timestamp set.

Actual behaviour:
If there is an existing entry for key K, calling computeIfAbsent(K, ...) will update the write timestamp (and write queue) for that existing entry. (As expected, if there is no existing entry for K, a new value is computed and a write timestamp set.)

Implication:
Because the write timestamp of the entry is reset, if an entry is accessed using computeIfAbsent() repeatedly in intervals less that the expireAfterWrite duration, it will never expire, acting more like an expireAfterAccess cache.

Related code in LocalCache:
where recordWrite() is called: LocalCache.java#L2248
computeIfAbsent() implementation: LocalCache.java#L4201

Proposed test case in LocalCacheTest, which currently will fail:

  public void testComputeIfAbsent_ExistingEntryWriteTimeNotChanged() {
    LocalCache<Object, Object> map =
          makeLocalCache(
                createCacheBuilder()
                      .expireAfterWrite(1, MINUTES)
                      .expireAfterAccess(1, MINUTES));

    String key = "a";
    map.put(key, "b");
    ReferenceEntry<Object, Object> entry = map.getEntry(key);
    Object originalValue = entry.getValueReference().get();
    long originalWriteTime = entry.getWriteTime();
    long originalAccessTime = entry.getAccessTime();

    map.computeIfAbsent(key, k -> "c");
    assertEquals(originalValue, map.get(key));
    assertEquals("Write time should not be changed, since the value was not changed",
          originalWriteTime, entry.getWriteTime());
    assertThat(entry.getAccessTime()).isGreaterThan(originalAccessTime);
  }

Versions:

• Discovered using guava:27.0-jre
• Introduced in Release Java 8 changes to Guava (73e382f)
• This issue still exists in master.

Specific use case:
A polling endpoint which checks an authentication cache (expire after write) that is accessed via Spring Boot's @Cacheable AOP abstraction. The implementation of this abstraction uses the cache's Map.computeIfAbsent() method when synchronized access is needed:
Spring caching abstraction, synchronized access: CacheAspectSupport.java#L381
Actual call to computeIfAbsent() in Spring: ConcurrentMapCache.java#L144

[cpovirk]

Thanks for the report. It does sound like we should avoid updating the write timestamp in this case.

True as that is, we rarely make changes to common.cache nowadays. We recommend that users use Caffeine: Its API is based on Guava's (and it has an adapter to the Guava cache interfaces), and it is more actively maintained.

I suspect that it does not have this bug, but I can't say for certain.

[ben-manes]

A direct translation of your test for Caffeine passes:

static <K, V> BoundedLocalCache<K, V> asBoundedLocalCache(Cache<K, V> cache) {
  return (BoundedLocalCache<K, V>) cache.asMap();
}

public void testComputeIfAbsent_ExistingEntryWriteTimeNotChanged() {
  FakeTicker ticker = new FakeTicker();
  BoundedLocalCache<Object, Object> cache = asBoundedLocalCache(Caffeine.newBuilder()
      .expireAfterWrite(1, TimeUnit.MINUTES)
      .expireAfterAccess(1, TimeUnit.MINUTES)
      .executor(Runnable::run)
      .ticker(ticker::read)
      .build());

  String key = "a";
  Object originalValue = "b";
  cache.put(key, originalValue);

  long originalWriteTime = cache.data.get(key).getWriteTime();
  long originalAccessTime = cache.data.get(key).getAccessTime();

  ticker.advance(30, TimeUnit.SECONDS);
  cache.computeIfAbsent(key, k -> "c");

  long currentWriteTime = cache.data.get(key).getWriteTime();
  long currentAccessTime = cache.data.get(key).getAccessTime();

  assertThat(cache.get(key), is(originalValue));
  assertThat(currentWriteTime, is(originalWriteTime));
  assertThat(currentAccessTime, is(greaterThan(originalAccessTime)));
}