Cache Disobeys Expiration if Value Is Stuck in Refreshing

[gm2211]

#Issue
If refreshAfterWrite is set and a value refresh is triggered, if the refreshing call takes "forever", the old value is never evicted.

#Ideal outcome
expireAfterWrite is respected regardless of whether there is a refresh in progress

#Acceptable outcomes [not mutually exclusive]

• Caffeine somehow makes it so that refresh async futures have a timeout of max(0, node.write + expireAfterNanos - now) or something like that.
• This behavior is documented in the builder method for refreshAfterWrite and for buildAsync so consumers can choose to either implement their own logic for refreshing or mitigate the issue by wrapping loading function calls in time limiters (which is a good idea anyway)
  (after further digging, I found the behavior to be described here https://github.com/ben-manes/caffeine/wiki/Refresh, but I think it would still be useful to add If an entry isn't queried after it comes eligible for refreshing, it is allowed to expire. to the javadoc of the builder methods (+ maybe change the language to A query won't be allowed to expire if it is queried after it has become eligible for refreshing)

#Some debugging details
I believe this is somewhat related to #193 and #251.

I think I've traced it to https://github.com/ben-manes/caffeine/blob/master/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Async.java#L35
which is used when setting the new write time on a node when refreshing is kicked off

caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java

Line 1183 in 37f6ad3

&& node.casWriteTime(oldWriteTime, refreshWriteTime)) {

.

I'm sure there are good reasons for why it was implemented that way, but it seems a little odd to modify the write time before the value has actually been refreshed.

Sample code illustrating the issue

        StubTicker ticker = new StubTicker();

        CyclicBarrier barrier = new CyclicBarrier(2);
        AtomicInteger count = new AtomicInteger(0);
        CacheLoader<Integer, Integer> loader = key -> {
            if (count.getAndIncrement() == 0) {
                return key * 2;
            }
            barrier.await();
            return key * count.get();
        };

        AsyncLoadingCache<Integer, Integer> cache = Caffeine.newBuilder()
                .initialCapacity(2)
                .maximumSize(2)
                .ticker(ticker)
                .executor(Executors.newSingleThreadExecutor())
                .expireAfterWrite(Duration.ofMinutes(10))
                .refreshAfterWrite(Duration.ofMinutes(1))
                .buildAsync(loader);

        assertThat(cache.get(1).get(1, TimeUnit.SECONDS)).isEqualTo(2);
        assertThat(count).hasValue(1);

        // Should still get cached value
        assertThat(cache.get(1)).isCompletedWithValue(2);
        assertThat(count).hasValue(1);

        ticker.tick(Duration.ofMinutes(2)); // > than refresh duration

        assertThat(cache.get(1)).isCompletedWithValue(2);

        Awaitility.await().untilAtomic(count, Matchers.equalTo(2)); // Make sure loader is invoked

        ticker.tick(Duration.ofDays(20)); // Insanely after expiration time
        assertThat(cache.get(1)).isNotCompleted();

[ben-manes]

I think this problem is also discussed in #282? If so, then I was planning on trying the approach that @Maaartinus proposed.

The current logic is an attempt to avoid extra fields just for refreshing. Guava avoids that problem by forking the hash-table, so it swaps the Entry class type with one that temporarily has refreshable attributes.

[gm2211]

whoops - I thought I went through previous issues, but somehow missed that one. Feel free to close as duplicate

[gm2211]

wonder if it's worth updating the javadocs as an interim mitigation though - can submit a PR for that if helpful

[ben-manes]

PR would be great. We just shouldn't write it in a way that locks ourselves into behavior if we want to change that. So loose words like "may" is better than a strong contractual word like "won't".

[ben-manes]

Released in 3.0