Exception from weigher is ignored in an AsyncCache

[chaoren]

Here's a test case:

  @Test
  public void weigher() {
    AsyncCache<String, String> cache =
        Caffeine.newBuilder()
            .maximumWeight(0)
            .weigher(
                ((key, value) -> {
                  throw new RuntimeException();
                }))
            .buildAsync();

    // case 1
    assertThrows(
        RuntimeException.class,
        () -> cache.get("key", (key, executor) -> completedFuture("value"))); // pass
    assertThat(cache.asMap()).isEmpty(); // pass

    // case 2
    var future = cache.get("key", (key, executor) -> new CompletableFuture<>());
    assertThrows(RuntimeException.class, () -> future.complete("value")); // fail
    assertThat(cache.asMap()).isEmpty(); // fail
  }

The first case:

cache.get("key", (key, executor) -> completedFuture("value"))

throws as expected. The cache also remains empty after the exception.

The second case:

future.complete("value")

however, does not throw at all. Probably impossible to get it to throw due to how CompletableFutures are structured, but the exception doesn't seem to be logged anywhere either. It seems to be silently ignored. Also, the entry remains in the cache after the exception.

[ben-manes]

It must be these lines below that need to handle if the replacement fails exceptionally. The future completed and the dependent action tries to update the metadata. In a synchronous cache the operation would fail untouched, as one would expect, but here we already stored it and need to unpin it (has a weight of zero while in-flight). It would need to log the exception try to conditionally remove it in that scenario, which would bubble up to the removal listener as the explicit cause (which I think guava/caffeine does elsewhere, like on a discarded refresh).

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

Lines 217 to 219 in a03e95d

	// update the weight and expiration timestamps
	cache().statsCounter().recordLoadSuccess(loadTime);
	cache().replace(key, castedFuture, castedFuture, /* shouldDiscardRefresh */ false);

[ben-manes]

I have most of this ready in the v3.dev branch. I'll try to wrap up the unit tests this weekend, as I need to also cover when the Expiry fails which would have a similar impact. In addition to the singular key-value case you found, the unit tests also uncovered the same mistake during refresh and bulk loads.