Async refreshAfterWrite - error handling

[nicopatche]

Hi,

I was having a look at the refreshAfterWrite behaviour and I'd like to suggest some improvement to have a better control over the error handling.

Unless I'm wrong at the moment any raised error while reloading is logged and swallowed.

I'd be great to tell the cache to return the stale value for a maximum of time while the reloading is failing otherwise the user might get stale data forever and unless we monitor the system logs there is no way to know.

Maybe the refreshAfterWrite could take another argument ie lambda such as

`T handleFailedReload(T staleValue, LocalDateTime writeTimestamp, Throwable error) throws Throwable {

// User can log error however he likes

// User can return null meaning the current stale value is returned by the cache and another attempt at reloading will occur asynchronously

// User can decide to rethrow the error (or another one) using writeTimestamp or not. Cache.get() will throw the exception from now on while another attempt at reloading will occur asynchronously

// User may return a different instance of T (or the stale one) which is equivalent to writing the value into the cache discarding the current error.

}`

In any case, unless it is the first call to get(), we should not block and keep reloading in the background and return the stale value or the last reloading error returned by the handler.

[ben-manes]

The intent of refreshAfterWrite was for it to be combined with an expiration policy. This is sometimes referred to as soft vs hard TTL, where the entry must be discarded when it reaches this expiration time as unusable and prior to that an optimistic reload can be performed when the entry ages beyond a threshold. The intent is to hide the latency for popular items, like a configuration value, where expiration would force a periodic delay as many threads wait for the new value. Instead, an optimistic reload allows hot items to stay fresh and not impact the P99 response times.

Unfortunately this feature can be enabled with expiration, so how stale an entry is can be unbounded. That's an oversight from Guava which was carried over for compatibility and now both may be stuck never disallowing it in case some users did find this adventitious. Typically those users misunderstood this feature where they wanted a replica, not a cache, with a scheduled task to reload the contents. That's better served by a ScheduledExecutorService.

When the reload fails the cache does swallow and log the exception. The next time the entry is requested a new reload is attempted. If the entry is evicted then any in-flight reload is abandoned. If the reload returns null then the entry is removed, and if a value then it is treated as an update to reset any policy constraints.

For more advanced error handling, like retries, a dedicated resilience library like failsafe can be combined to make smarter decisions. The asyncReload returns a CompletableFuture, so you can return a retrying future that encapsulates all of the backoff, attempts, and failure handling. If you exhaust retries then returning null would discard the entry, or simply never completing would allow expiration to eventually do so. For the cache the future is simply an indicator not to reattempt unless the mapping is otherwise changed.

new CacheLoader<K, V>() {
  
  public V load(K key) { ... }

  public CompletableFuture<V> asyncReload(K key, V oldValue, Executor executor) {
    var retryPolicy = RetryPolicy.builder()
        .onFailure(event -> log.error("Failed to refresh cache entry {}", key, event.getLastException())
        .withBackoff(1, 30, ChronoUnit.SECONDS)
        .withJitter(Duration.ofMillis(100))
        .withMaxAttempts(5)
        .build();
    var failsafe = Failsafe.with(retryPolicy, Fallback.of(null));
    return failsafe.getAsync(() -> load(key));
  }
}