3

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?
Quincy Bowers
  • 390
  • 2
  • 15
  • This is code I've inherited. I am sure Sonar is telling me to do the right thing. What I am trying to get from the question is to understand more deeply what is going on when that InterruptedException is thrown and how I should respond to it, if at all. Is it the HTTP request handling thread which was interrupted? If so, it seems clear that I should just let the exception propagate. – Quincy Bowers Mar 04 '20 at 15:14
  • 3
    Since all futures are supposed to be completed when `invokeAll` returns, `InterruptedException` should never occur. Hence, the correct way of handling an exception that should never occur is `catch(InterruptedException ex) { throw new AssertionError(ex); }` – Holger Mar 05 '20 at 09:36
  • Thank you @Holger. That is obviously true. – Quincy Bowers Mar 05 '20 at 22:19

0 Answers0