google / guava

Google core libraries for Java
Apache License 2.0
50.19k stars 10.91k forks source link

ListenableFuture: Exception thrown when a future was cancelled seems to be lost #6794

Open YutaHiguchi-bsn opened 1 year ago

YutaHiguchi-bsn commented 1 year ago

When a ListenableFuture is canceled and the interrupted task threw an exception, exception thrown from the task does not seem to appear in the CancellationException as cause or as suppressed exception.

CountDownLatch latch = new CountDownLatch(1);
ListenableFuture<Void> future = Futures.submit(() -> {
    try {
        latch.await();
    } catch (InterruptedException e) {
        Thread.currentThread().interrupt();
        throw new IllegalStateException("Cancellation failed", e);
    }
}, ForkJoinPool.commonPool());
Thread.sleep(1000);
future.cancel(true);
CancellationException e = assertThrows(CancellationException.class, future::get);
// expected either e.getCause() or e.getSuppressed() will carry IllegalStateException thrown from Interrupt handler

Do I have a wrong expectation and is there another way to propagate failure during cancellation to the future?

cpovirk commented 1 year ago

Our behavior follows the JDK's Future implementations—at least as far as I'm aware, but I haven't followed as much in the days of ForkJoinPool and CompletableFuture, so do correct me if you're aware of counterexamples. As you point out, that behavior has some notable drawbacks, given that it could swallow even an Error.

Come to think of it, Guava actually used to rethrow an Error (but not any kind of Exception) aggressively when setting it as the result of a Future. That had its own problems, but I think it was getting at a similar concern to the one you raise here.

It is interesting to think about attaching execution exceptions as suppressed exceptions of the CancellationException. On the one hand, it could clearly be useful for debugging and monitoring. On the other hand, apps might come to depend on it, and I find it a little scary to imagine that a change between the a JDK Future implementation and a Guava Future implementation could change behavior in a way that they care about.

The obvious way to serve only the debugging and monitoring use cases is to log, but we know that that's not always desirable: https://github.com/google/guava/issues/2134. Another possibility is static state (https://github.com/google/guava/issues/1336), which is probably less likely to be used by apps for non-debugging/monitoring use cases than a suppressed exception would be. But no one loves static state.

Actually, if we're talking about Futures.submit specifically (rather than Guava's executors/futures in general), then we could choose to add an optional parameter that receive information about exceptions. But that would require some design work and some plumbing, so it would need more thought.

(It's possible that some people who want a "cancellable task" would be interested in https://guava.dev/Service. Others might want to make their task catch and store any exception that occurs. Of course, that latter approach helps only after they realize that their exception is being swallowed in the first place. But a project could define a common library for it.)

My guess is that, at the end of the day, we won't end up doing anything further about this in Guava. But it's worth keeping in my back of our minds.

YutaHiguchi-bsn commented 1 year ago

Thank you for the context. I've experimented with CompletableFuture and observed the same behavior, cancelling the future while some stage is in-flight also lost the exception from that stage.

The use case I had was specific to Futures.submit, so I will take uncaught exception handler-like approach outside of guava. Wrap around that task lambda to intercept uncaught exception, and compose a future which will add the intercepted exception as suppressed to the CancellationException.