Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
YutaHiguchi-bsn opened this issue Oct 19, 2023 · 2 comments
Labels
P4 package=concurrent type=enhancement Make an existing feature better

Comments

@YutaHiguchi-bsn
Copy link

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
Copy link
Member

cpovirk commented Oct 20, 2023

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: #2134. Another possibility is static state (#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.

@cpovirk cpovirk added type=enhancement Make an existing feature better package=concurrent P4 labels Oct 20, 2023
@YutaHiguchi-bsn
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 package=concurrent type=enhancement Make an existing feature better
Projects
None yet
Development

No branches or pull requests

2 participants