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

New bad-practice rule for missing exception handling on CompletableFuture #950

Open
Vampire opened this issue May 17, 2019 · 1 comment
Open

Comments

@Vampire
Copy link
Contributor

Vampire commented May 17, 2019

When you are using CompletableFutures (or CompletionStages) and do not do explicit exception handling, you are effectively ignoring exceptions as they are swallowed.
I'd say a spotbugs rule that forbids this would make sense.
But be aware that it is not a simple "call exceptionally on all CompletionStages that are returned" check. You can have chain multiple completion stages / steps together using then... kind of methods and only do the exception handling as last call in the chain in exceptionally or in whenComplete (if there the throwable parameter also is checked actually).
You could also have intermediary exceptionally blocks and some additional chained stages afterwards that should again be checked with exception handling after that.
When returning the completable future from a method it is probably not necessary to enforce this as the calling code should probably handle exceptions.

To do this right can get a bit tricky, but it would be very helpful, as not doing exception handling means it is just swallowed and you do not get any notification so you might heavily wonder about what is going wrong when not even an exception is displayed.

@A248
Copy link

A248 commented Apr 1, 2021

I significantly agree with adding a spotbugs rule for this. Allow me some background.

This problem of CompletableFutures is very annoying, and the more you use them, the more it shows. If you have a CompletableFuture, and you do not await it, return it, or arrange a dependent action, it must have an exceptionally block or whenComplete attached, otherwise you will swallow exceptions. Forgetting to handle exceptions is equivalent to a try { } catch (Throwable ignored) {}, which is undesirable for obvious reasons.

Details of such a spotbugs rule

Spotbugs needs to catch the situation where a future is not awaited or returned and has no dependent futures. I call this issue the problem of "dangling futures," because it arises whenever a CompletableFuture is ignored.

This rule is not as simple as "don't ignore a method returning a CompletableFuture."

All Possible Situations

The first 4 situations are fine. The last one, No. 5, should be flagged by this spotbugs rule.

  1. Future awaited. The exception will be re-thrown in the awaiting thread.
future.join();
  1. Dependent future arranged. The exception will be propagated to the dependent future. thenRun could also be thenAccept, thenApply, thenCompose etc.
future.thenRun(
  // Callback here
)

Note well: there are 2 futures involved here. The first future, future, does not need exception handling. The second future, returned from thenRun, needs to be analyzed independently as to whether its exceptions are properly handled.

  1. Returned future. The caller is responsible for handling exceptions.
return future;
  1. Exception handled explicitly.
future.exceptionally((ex) -> {
  logger.error("Exception encountered", ex);
  return null;
});

Note well: once again, there are 2 futures involved here. The methods exceptionally and whenComplete both return CompletableFutures, yet it is these methods which are themselves designed to handle exceptions.

  1. Exception NOT handled. This is a problem and should be flagged
previousFuture.thenRun(() -> {
  // Something
});

Note well: there are yet again 2 futures involved here. previousFuture does not need exception handling. However, thenRun returns another future, a future which does indeed need exception handling.

In case someone does not think this rule is needed

I would say this rule is crucial for any project using CompletableFutures. I personally have never used spotbugs in my own projects before, but this rule would make me instantly use spotbugs and save so much trouble.

The following code looks harmless, but actually it swallows all exceptions:

public void outerMethod() {
  myMethodReturningACompletableFuture().thenApply((x) -> {
    // Do some computations
    return y;
  }).thenAcceptAsync((y) -> {
    // Do more operations
  });
}

In one project, for instance, I solved some exceptions which were swallowed in this way. A few months later in the same project,I found more instances of ignored exceptions and so fixed those. Yet again, I've recently found another case. At that point I thought of creating some kind of CompletableFuture-checker or using an existing tool, which is how I found my way here.

A248 added a commit to A248/spotbugs that referenced this issue Apr 1, 2021
Does not add the DM_COMPLETABLE_FUTURE bug type. Merely creates
  a test to demonstrate how such a rule would function.
A248 added a commit to A248/spotbugs that referenced this issue Apr 1, 2021
Does not add the DE_COMPLETABLE_FUTURE bug type. Merely creates
  a test to demonstrate how such a rule would function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants