7

I have to run multiple external call operations and then obtain the result in a form of list. I decided to use the CompletableFuture api, and the code I prepared is pretty disgusting:

The example:

public class Main {
    public static void main(String[] args) {
        String prefix = "collection_";

        List<CompletableFuture<User>> usersResult = IntStream.range(1, 10)
                .boxed()
                .map(num -> prefix.concat("" + num))
                .map(name -> CompletableFuture.supplyAsync(
                        () -> callApi(name)))
                .collect(Collectors.toList());

        try {
            CompletableFuture.allOf(usersResult.toArray(new CompletableFuture[usersResult.size()])).get();
        } catch (InterruptedException | ExecutionException e) {
            e.printStackTrace();
        }

        List<User> users = usersResult //the result I need
                .stream()
                .map(userCompletableFuture -> {
                    try {
                        return userCompletableFuture.get();
                    } catch (InterruptedException | ExecutionException e) {
                        e.printStackTrace();
                    }
                    return null;
                })
                .filter(Objects::nonNull)
                .collect(Collectors.toList());
    }

    private static User callApi(String collection) {
        return new User(); //potentially time-consuming operation
    }
}

I have the following questions:

  1. Can I somehow avoid duplicating the try-catch block in the stream, where I'm mapping CompletableFuture to User?
  2. Can this code be less sequential (how can I avoid waiting for all the futures to finish?)
  3. Is it ok, to do it this way (will all the futures be resolved in the stream?):

    public class Main {
        public static void main(String[] args) {
            String prefix = "collection_";
    
            List<User> usersResult = IntStream.range(1, 10)
                    .boxed()
                    .map(num -> prefix.concat("" + num))
                    .map(name -> CompletableFuture.supplyAsync(
                            () -> callApi(name)))
                    .filter(Objects::nonNull)
                    .map(userCompletableFuture -> {
                        try {
                            return userCompletableFuture.get();
                        } catch (InterruptedException | ExecutionException e) {
                            e.printStackTrace();
                        }
                        return null;
                    })
                    .collect(Collectors.toList());
        }
    
        private static User callApi(String collection) {
            return new User(); //potentially time-consuming operation
        }
    }
    
Didier L
  • 18,905
  • 10
  • 61
  • 103
agienka
  • 396
  • 1
  • 2
  • 11
  • You don't need to explicitly wait for them to finish at all, `get ` does that when invoked. You can extract the `try /catch ` lambda to its own method to make it more readable. – daniu Oct 30 '17 at 22:55
  • For 1- you can implement & call an utility method which catch exceptions and throw a runtime so instead of userCompletableFuture.get(); RuntimeUtils.getCompletable(userCompletableFuture); – HRgiger Oct 31 '17 at 09:17

2 Answers2

9

For 1., you can entirely skip the allOf().get() calls since you are anyway waiting on all futures one by one.¹

For 2., you can simplify the try-catch by doing the following:

  • use exceptionally() to handle exceptions directly in the future;
  • use join() instead of get() to avoid checked exceptions (and you know no exceptions are possible).

For 3., you cannot really make it less sequential since you need at least to steps: create all futures and then process their results.

If you do everything in a single stream, it will create each future, then immediately wait on it before creating the next one – so you would lose the parallelism. You could use a parallel stream instead but then there wouldn't be much benefit of using CompletableFutures.

So the final code is:

List<CompletableFuture<User>> usersResult = IntStream.range(1, 10)
        .boxed()
        .map(num -> prefix.concat("" + num))
        .map(name -> CompletableFuture.supplyAsync(() -> callApi(name))
            .exceptionally(e -> {
                e.printStackTrace();
                return null;
            }))
        .collect(Collectors.toList());

List<User> users = usersResult
        .stream()
        .map(CompletableFuture::join)
        .filter(Objects::nonNull)
        .collect(Collectors.toList());

¹ Note that an allOf() call remains needed if you want your result to be a CompletableFuture<List<User>> as well, e.g.

final CompletableFuture<List<User>> result =
        CompletableFuture.allOf(usersResult.stream().toArray(CompletableFuture[]::new))
                .thenApply(__ -> usersResult
                        .stream()
                        .map(CompletableFuture::join)
                        .filter(Objects::nonNull)
                        .collect(Collectors.toList()));
Didier L
  • 18,905
  • 10
  • 61
  • 103
  • Hm well, if there is no need to call `CompletableFuture.allOf`, maybe there is no need to split it to two separate streams either? (see my edit). However I have some doubts about the completion of all completables within the stream.. Does the stream have to be closed before collecting results? – agienka Oct 31 '17 at 09:36
  • @nibsa This was already addressed in the answer but I tried to clarify it a bit. – Didier L Oct 31 '17 at 10:19
  • Making the stream parallel is not entirely pointless: as nibsa pointed out, this results in using a common thread pool. Combining this with `CompletableFuture`s, the common pool can create the working threads and collect them later, while the worker pool is under the implementor's control. – daniu Nov 01 '17 at 10:53
  • @daniu each thread in the pool would still create 1 CF and immediately wait for its result. For simple background tasks (like in the question here) you don't get any benefit compared to just running the task itself inside the stream pipeline. – Didier L Nov 01 '17 at 11:05
0

Alternatively, you can just drop the CompletableFuture and use parallelStream() as Didier mentioned:

Optional<User> wrapApiCall(String name) {
    try { return Optional.of(callApi(name)); }
    catch (Exception e) { 
        e.printStackTrace();
        return Optional.empty(); 
    }
}

List<User> usersResult = IntStream.range(1, 10)
    .boxed()
    .parallelStream()
    .map(num -> String.format("%s%d", prefix, num))
    .map(this::wrapApiCall)
    .filter(Optional::isPresent)
    .map(Optional::get)
    .collect(Collectors.toList());
daniu
  • 14,137
  • 4
  • 32
  • 53
  • Parallel stream was my first approach to solve this issue, but unfortunately parallel stream uses common thread pool, and cannot be supplied with custom, monitored one :/ – agienka Nov 01 '17 at 10:25
  • @nibsa That's a good point of course. Actually, that makes the use of `CompletableFuture` and `parallelStream` not as pointless as Didier states; the common pool only does the creation and joining of the `Future`s, the pool of which does the work. – daniu Nov 01 '17 at 10:32
  • It is actually possible to run a parallel stream in another pool, look for it on SO. – Didier L Nov 01 '17 at 11:13