error-prone icon indicating copy to clipboard operation
error-prone copied to clipboard

False positive FutureReturnValueIgnored in java Standard Library

Open RoiEXLab opened this issue 7 years ago • 2 comments

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?

RoiEXLab avatar Dec 26 '17 20:12 RoiEXLab

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:

  1. 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.
  2. 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.

mdindoffer avatar Sep 26 '21 13:09 mdindoffer

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

jnehlmeier avatar Jun 24 '22 09:06 jnehlmeier