error-prone
error-prone copied to clipboard
False positive FutureReturnValueIgnored in java Standard Library
From the javadocs of ScheduledExecutorService#scheduleAtFixedRate:
Returns:
a ScheduledFuture representing pending completion of the series of repeated tasks. The future's get() method will never return normally, and will throw an exception upon task cancellation or abnormal termination of a task execution.
So perhaps an exception for ScheduledFuture
Futures would be nice?
This technically is a false positive. But, it still pointed out something of concern in my code.
Since the returned Future
is not intended to be get()
-ed, any uncaught exceptions raised in the callable submitted via scheduleAtFixedRate
and scheduleWithFixedDelay
are automatically swallowed by default.
Background
A rather naive way to solve this would be by using a custom ThreadFactory
passed to the service, along with a custom UncaughtExceptionHandler
:
ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(r -> {
Thread thread = new Thread(r);
thread.setUncaughtExceptionHandler((t, e) -> LOG.warn("Uncaught exception observed", e));
return thread;
});
However, this approach doesn't work in this case, because the ScheduledThreadPoolExecutor
automatically wraps the runnable in a Future, so it never bubbles up to the UncaughtExceptionHandler
.
Thus only 2 solutions are available AFAIK:
- Call the
get()
method on the returned Future with another thread. Which is obviously nonsense - both not guaranteed to work and a waste of threads. - Wrap the submitted code with a try-catch block. Which is cumbersome at best. This makes it impossible to use method references when submitting a runnable and makes one-liner lambdas gain additional lines of wrapper code.
Frankly, all of this sounds like a major design flaw made by Doug Lea, or whomever was designing the java.util.concurrent
package. But here we are.
EP
So to solve this conundrum with ErrorProne, I propose EP will consider scheduleAtFixedRate
and scheduleWithFixedDelay
as special cases and skip the FutureReturnValueIgnored
check for those.
But at the same time, this should come with another checker for a bug pattern, which should check whether every line of code submitted via those methods is wrapped with a try-catch block.
So to solve this conundrum with ErrorProne, I propose EP will consider scheduleAtFixedRate and scheduleWithFixedDelay as special cases and skip the FutureReturnValueIgnored check for those. But at the same time, this should come with another checker for a bug pattern, which should check whether every line of code submitted via those methods is wrapped with a try-catch block.
Yeah that would be nice. Especially if you already have code that uses a Runnable
with code wrapped in a try-catch block that warning of EP is even more useless.
IMHO that amount of false positives generated by the FutureReturnValueIgnored
check for ScheduledExecutor
could also be seen a violation of the rules for EP warnings defined in https://errorprone.info/docs/criteria