In some legacy code I have inherited, there is a snippet that looks similar to this.
List<Callable<String>> callableStrings = Arrays.asList("one", "two", "three").stream()
.map(s -> (Callable<String>) () -> s)
.collect(Collectors.toList());
List<String> results =
someExecutorService.invokeAll(callableStrings).stream()
.map(
future -> {
try {
return future.get();
} catch (ExecutionException | InterruptedException e) {
LOG.info("Unexpected Exception encountered while fetching result", e);
return null;
}
})
.filter(Objects::nonNull)
.collect(Collectors.toList());
Instead of Callable<String>
, I am really dealing with some heavier computation. Sonar highlights the catch (ExecutionException | InterruptedException e)
as a problem, suggesting that you should always rethrow InterruptedException
. https://rules.sonarsource.com/java/tag/multi-threading/RSPEC-2142
The intent of this code seems to be to filter out any of the heavy computations which had a problem, returning only those which succeeded. For context, this code is called as a result of an HTTP request my application is handling. If the InterruptedException
is allowed to propagate all the way to the top then the client is going to get an error response rather than seeing the list of successful results.
There is a lot of different kinds of information to be found on dealing with InterruptedException
from Future#get
. Most of the ones that seem like very clear and solid advice are those that are dealing with a situation where you have implemented something like Runnable
and are calling Future#get
from within that class.
This answer, https://stackoverflow.com/a/4268994, suggests that if I see InterruptedException
thrown in the code above, that indicates that the thread handling the HTTP request has been interrupted, not the one running the Future. Is that correct?
My questions are:
- Should I be concerned with the Sonar warning and, if so, what bad things will happen as a result of keeping the code as it is?
- How should the code be refactored to be improved?