refreshAfterWrite does not use loadAll

[cjohnson2]

If you have a CacheLoader with loadAll implemented in use with a LoadingCache that is configured to refreshAfterWrite, calls to getAll load each item individually rather than falling back to the bulk loadAll for all expired keys. This can cause some serious issues when your datasource has high latency. It would be nice if refreshAfterWrite functioned the same as expiredAfterWrite, in that loadAll gets called for all the expired keys.

Here is an example that shows the behavior:

public class TestCacheLoader extends CacheLoader<String, Integer> {

    @Override
    public Map<String, Integer> loadAll(Iterable<? extends String> keys) throws Exception {
        System.out.println("loadAll: " + Joiner.on(",").join(keys));

        Map<String, Integer> results = new HashMap<>();
        for (String key : keys) {
            results.put(key, 1);
        }
        return results;
    }

    @Override
    public Integer load(String key) throws Exception {
        System.out.println("load: " + key);
        return 1;
    }
}

@Test
public void testRefreshAfterWrite() throws Exception {

    com.google.common.cache.LoadingCache<String, Integer> cache
            = CacheBuilder.from("refreshAfterWrite=1s").build(new TestCacheLoader());

    for (int i = 1; i <= 4; i++) {
        System.out.println("getAll");
        Map<String, Integer> values = cache.getAll(Lists.newArrayList("A", "B"));
        Thread.sleep(1100);
    }
}

[ben-manes]

This may be too invasive of a change for the Guava team to resolve soon. For the short-term, I'd see if your datasource supports multi-get escalation to collapse a sequence of gets into a single operation (see memcached clients for an example).

---

For the Guava team, since few are familiar with the cache logic....

When getAll probes for the missing keys it effectively performs a getIfPresent which ignores expired entries and schedules a refresh if needed. It then performs the bulk load of the keys for the entries not found. This crosses the segment boundary, which makes choosing an optimal strategy a hard choice.

A simplistic approach of returning a tuple (value, shouldRefresh) results in garbage that may not be stack eliminated. If not, that garbage on read adds significant overhead under high usage by incurring more frequent GCs. This can already be seen by the read buffers causing GC thrashing, where the proposed fix has languished.

A naive but optimal approach would have to duplicate logic and be much more low-level in order to track both missing and refreshable keys. This might mean that the top-level is more aware of the segment's internals and creates a more cumbersome execution path.

Probably the best solution would be to pass a collecting parameter to the segment's get which captures if the key needs to be refreshed. This could be a duplicate method or unified by either a flag or using a queue's offer that rejects addition if the caller won't handle the refresh themselves (thereby allowing a static container instance). Regardless, then a refresh could be scheduled at the top-level in a cleaner fashion. There would still be a few quirks to work out when inserting the refreshed entries to ensure that the removal notification is correctly specified.

Note that this issue is currently present in Caffeine and I haven't decided if it will be addressed soon or backlogged. No matter how it is done, this change is invasive and slightly ugly but worthwhile as a valid expectation.

[ben-manes]

The issue also manifests in getAllPresent. It may be best to fix it there and then have getAll delegate to it instead of get.

[FS1360472174]

still had met the same issue

[ben-manes]

I think this should be closed and users directed to use their favorite Reactive Streams library. See this example using Resctor, which could trivially be adjusted to complete Guava’s ListenableFuture.