15

I am currently using CompletableFuture supplyAsync() method for submitting some tasks to common thread pool. Here is what code snippet looks like:

final List<CompletableFuture<List<Test>>> completableFutures = resolvers.stream()
        .map(resolver -> supplyAsync(() -> task.doWork()))
        .collect(toList());

CompletableFuture.allOf(completableFutures.toArray(new CompletableFuture[completableFutures.size()])).join();

final List<Test> tests = new ArrayList<>();
completableFutures.stream()
        .map(completableFuture -> completableFuture.getNow())
        .forEach(tests::addAll);

I would like to know how below differs from above code. I removed the parent completableFuture from below code, and added join() for each of the completableFuture instead of getNow():

final List<CompletableFuture<List<Test>>> completableFutures = resolvers.stream()
        .map(resolver -> supplyAsync(() -> task.doWork()))
        .collect(toList());

final List<Test> tests = new ArrayList<>();
completableFutures.stream()
        .map(completableFuture -> completableFuture.join())
        .forEach(tests::addAll);

I am using this in the spring service and there are issues with thread pool exhaustion. Any pointers is deeply appreciated.

Ganesh Shenoy
  • 619
  • 1
  • 10
  • 28
  • Why would you use `join` in the second example? You can just do `List tests = futures.stream().map(CompletableFuture::get).collect(toList())`. – daniu Sep 17 '18 at 15:26
  • 5
    @daniu On the contrary, you can't do that because `get` is declared as throwing a checked exception. – Sotirios Delimanolis Sep 17 '18 at 15:37
  • As far as I understand get vs join is about exception it throws. Checked vs unchecked. I am not sure any other issues are there using join. Thank you for pointer – Ganesh Shenoy Sep 17 '18 at 15:37
  • myThreadFactory.getExecutorService() - does it takes an existing thread pool or creates new? If creates, then it explains thread pool exhaustion. – Alexei Kaigorodov Sep 17 '18 at 15:54
  • @alexei Apologies it is the new code. Old code uses ForkJoinPool.commonpPool(). I have corrected the question. But could you please highlight what might go wrong if I use my own factory? ForkJoinPool was getting exhausted hence I was planning to use own ThreadPoolExecutor with core and max thread size, externalizing sizes in spring property file. And I don't have a use case of recursive sub tasks in my code hence was planning plain ThreadPoolExecutor. – Ganesh Shenoy Sep 17 '18 at 16:15
  • 2
    both variants are equivalent in regard of thread exhaustion. The reason is probably inside the code of task.doWork() - if it runs slowly and there are too many tasks. Just use fixed thread pool. – Alexei Kaigorodov Sep 17 '18 at 16:41
  • Yes thanks. That is what I am looking at. Fixed size ThreadPoolExecutor and load testing for figuring the thread count. – Ganesh Shenoy Sep 17 '18 at 17:48

1 Answers1

10

First of all, .getNow() does not work, as this method requires a fall-back value as argument for the case the future is not completed yet. Since you are assuming the future to be completed here, you should also use join().

Then, there is no difference regarding thread exhaustion as in either case, you are waiting for the completion of all jobs before proceeding, potentially blocking the current thread.

The only way to avoid that, is by refactoring the code to not expect a result synchronously, but rather schedule the subsequent processing action to do done, when all jobs have been completed. Then, using allOf becomes relevant:

final List<CompletableFuture<List<Test>>> completableFutures = resolvers.stream()
    .map(resolver -> supplyAsync(() -> task.doWork()))
    .collect(toList());

CompletableFuture.allOf(completableFutures.toArray(new CompletableFuture<?>[0]))
    .thenAccept(justVoid -> {
        // here, all jobs have been completed
        final List<Test> tests = completableFutures.stream()
            .flatMap(completableFuture -> completableFuture.join().stream())
            .collect(toList());
        // process the result here
    });

By the way, regarding the toArray method on collections, I recommended reading Arrays of Wisdom of the Ancients

Holger
  • 285,553
  • 42
  • 434
  • 765
  • To clarify the first para of yours. About allOf method I read it says it executes all threads parallely and will wait for all threads to complete. So I expected all threads to finish there and also I am using CompletableFuture.allOf(...).join() and hence was asking about allOf version. In your example with thenAccept it means current thread will continue its work after allOf method? If that the case synchronised result won't be possible, then I need to look at tuning pool size to make it synchronous, since this is the behaviour I need? Any better alternatives? ThreadPoolExecutor? – Ganesh Shenoy Sep 21 '18 at 17:47
  • 1
    `allOf` does not influence how the futures get completed. All it does, is to create a new future which gets completed when all futures in the array get completed and their completion is already in progress if they are synchronous. Calling `join` on the future returned by `allOf` is like calling `join` on each of the futures. No difference. `join` blocks, that's the fundamental, unavoidable point. If you call it in a F/J worker thread, the F/J pool will start a compensation thread, to ensure the configured target parallelism. But the result can be disastrous when calling `join` excessively. – Holger Sep 22 '18 at 09:42