LocalAsyncCache#handleCompletion does not properly handle cancellation in mapping function and logs the warning

[ujohnny]

Async cache implementation schedules futures with CompletableFuture.supplyAsync and in case of throwable CompletableFuture performs encodeThrowable that wraps Throwable into CompletionException.

On the other side the implementation for #597 does not perform unwrap operation (was it done for non async cache?) and just checks whether the error is a CancellationException. https://github.com/ben-manes/caffeine/blob/98895ffbb5e06b345d69fa969fc116c625b70217/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java#L208

However for async cache it's actually wrapped and requires unwrap or otherwise the CancellationException is logged which might not make any sense.

[ben-manes]

I'm not sure what you mean by "On the other side..." as cancelation is only a concept for async caches to participate in in CompletableFuture's lifecycle. A synchronous cache is not future-based.

Throwing a CancellationException as the result of an async call does not cause isCancelled to be true. I don't think we should peek into the underlying cause to guess what the user's intent is since java.util.concurrent does not, and there are usability benefits by being consistent with their behavior.

If you want to customize the suppression consider using a log filter.

@Test
public void cancelled() {
  var explicit = new CompletableFuture<Void>();
  explicit.whenComplete((r, e) -> {
    System.out.println("explicit exception: " + e);
  });
  explicit.completeExceptionally(new CancellationException());
  System.out.println("explicit: " + explicit.isCancelled());

  var implicit = CompletableFuture.supplyAsync(() -> {
    throw new CancellationException();
  });
  var f = implicit.whenComplete((r, e) -> {
    System.out.println("implicit exception: " + e);
  });
  try {
    f.join();
  } catch (Exception e) { /* expected */ }
  System.out.println("implicit: " + implicit.isCancelled());
}

explicit exception: java.util.concurrent.CancellationException
explicit: true
implicit exception: java.util.concurrent.CompletionException: java.util.concurrent.CancellationException
implicit: false

[ben-manes]

Another approach is to bubble up the exception so that it cancels the future in the cache, instead of wraps it. Then the explicit cancelation will be suppressed and the future will indicate that for any upstream consumers who might query isCancelled().

buildAsync((key, executor) -> {
  var future = new CompletableFuture<V>();
  performWork(key).whenComplete((r, e) -> {
    if (e instanceof CompletionException) {
      future.completeExceptionally(requireNonNullElse(e.getCause(), e));
    } else if (e != null) {
      future.completeExceptionally(e);
    } else {
      future.complete(r);
    }
  });
  return future;
});

[ujohnny]

@ben-manes sorry for the delay here. The issue is about AsyncCache. IMO it's fine to have java.util.concurrent.CancellationException thrown as a part of computation (that is performed using CompletableFuture.async). And it's not an error to have it there, however it's logged by default and not unwrapped from CompletionException.

[ben-manes]

I don’t think that’s a strong argument because the api for CompletableFuture says it’s fine to throw any exception as part of the computation. It doesn’t say that it will interpret it as a cancellation and infer the user’s intent. There are easy solutions if you want to avoid the logging, which so far seems like a more appropriate solution by keeping our interpretation consistent with CompletableFuture‘s.