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