Future's default executor prevents JVM from terminating

[smillies]

There even is a comment in the Javadocs: "Please note that it may prevent the VM from shutdown." This is a pretty serious shortcoming, because it always happens when doing something asynchronously with the default ExecutorService.

What is there against using an ExecutorService that creates daemon threads? The requirement that there should be one non-daemon thread that does something useful with the async result seems harmless, not a real restriction.

I will try to attach a patch for java.io.concurrent.Future for your consideration.

-- Sebastian

[smillies]

Attached patch:
future-patch.zip

[danieldietrich]

I love this change :)

Scala's default execution context behaves the same, it starts daemon threads.

Does it still make sense to use a CachedThreadPool? I mean, is the thread still reusable/cacheable when it is a daemon?

[danieldietrich]

I think Scala's default execution context is based on a ForkJoinPool (see this).

We had some problems using it. What do you think about it? Has the CachedThreadPool any drawbacks? Or would have the ForkJoinPool any advantages?

Just asking because it is hard to find someone who has an opinion regarding concurrency questions. It does not seem to be a 'mainstream topic' :)

[danieldietrich]

@smillies btw - your blog is lovely!

[nfekete]

@danieldietrich what were the problems using ForkJoinPool?

[danieldietrich]

I think I had a deadlock in the unit tests... I should switch back to investigate... I think the pool might has been depleted.

[nfekete]

I could maybe take a look into it if it's still an issue.

[danieldietrich]

That would be nice! The Future.DEFAULT_EXECUTOR_SERVICE needs to be changed and unit tests to be re-run...

[danieldietrich]

I think ForkJoinPool.commonPool() does not work because we need that Daemon thingy...

[smillies]

ForkJoinPool threads are all daemon threads. FJP is also work-stealing, meaning less contention on the queue, so it is a good choice under heavy load or when recursive tasks keep creating more tasks. However, loading everything off to the common FJP pool that is shared across the entire VM is likely to cause unpredictable behavior. (At my company, for example, we do not use Stream.parallel() for exactly that reason.)

Currently Vavr Future uses a CachedThreadPool. That basically has a task queue size of 0 and spawns new threads for each incoming task, killing them after a while. The problem is, it doesn't know when to stop creating more threads, and the default configuration has no limit. That makes it a bit unsuitable for computation-intensive tasks, it's better for short-lived tasks. As you don't know what kinds of tasks to expect in general, maybe one should move away from it.

A fixed thread pool would probably not be dynamic enough, especially in the recursive cases.

In sum, I would tend to use a ForkJoinPool, but not the common one, but one created with Executors.newWorkStealingPool(). (So forget my patch that started this issue.)

[danieldietrich]

@smillies Thank you, that is enlightening!

(Thinking loud: We can take big parts of your explanation directly for the javadoc - or even for the documentation)

[nfekete]

@danieldietrich I ran the tests with Future.DEFAULT_EXECUTOR_SERVICE = ForkJoinPool.commonPool() and all passed (except two tests that are ignored because of #1530). What should I look for? Maybe it got fixed?

[danieldietrich]

@nfekete thank you! I think all runs fine now. I switched the DEFAULT_EXECUTOR_SERVICE 'in the early days' of Future.

As @smillies said, we should not switch to Executors.newWorkStealingPool(). For now, it should be sufficient. Towards 1.0 we will add some features, like adding an uncaught exception handler (see #1979), maybe reading system properties for min and max threads etc.

[danieldietrich]

@nfekete If you want, you can create a PR that fixes this issue. It is the last one - then we are able to release 0.9.1 :-) I will wait, take your time.

[smillies]

@danieldietrich: Well, I did originally recommend to switch to Executors.newWorkStealingPool(), because I didn't like the idea of a VM-wide globally shared pool. You never know what it's going to be used for, and if someone makes the mistake of submitting blocking tasks, threads in the common FJP can be made inactive. This is more of a feeling, however, not really an argument.

On the other hand, using ForkJoinPool.commonPool() has the advantage of being both the simplest and the "expected" thing, in the sense that other async constructs in Java default to that as well (e. g. CompletableFuture).

So finally I agree that as a first step, ForkJoinPool.commonPool() is a reasonable choice. Experience will then show what features need to be added.

[danieldietrich]

@smillies

Experience will then show what features need to be added.

Yes, I agree. We should go the simplest way possible (that makes sense) and then refine.

@nfekete I will perform that change now in order to be able to release 0.9.1 this evening. I hope you are ok with that.

[danieldietrich]

@nfekete interesting, the Travis-CI build broke again after switching to ForkJoinPool.commonPool(): https://travis-ci.org/vavr-io/vavr/builds/276357097

The local build works fine.

Locally all is fine. I will switch the maven-surefire-plugin back to non-parallel mode. I think this will fix it (for now). Maybe it has something to do with an exit of test worker threads before the test threads finished. Could be solved by calling commonPool().awaitQuiescence(long, TimeUnit) in @Before. Will test that first...

[nfekete]

@nfekete I will perform that change now in order to be able to release 0.9.1 this evening. I hope you are ok with that.

Yes, absolutely.

[danieldietrich]

The impact of this change is too great for the upcoming patch release 0.9.1. I will defer the change and target 1.0.

[danieldietrich]

@smillies The ForkJoinPool resets the interrupt state of a worker thread. That has impact on cancelling Futures, also when working with the standard Java library:

    // -Djava.util.concurrent.ForkJoinPool.common.parallelism=1
    @Test
    public void test() {
        ForkJoinPool pool = ForkJoinPool.commonPool();
        Object monitor = new Object();
        java.util.concurrent.Future<?> future = pool.submit(() -> {
            try { synchronized(monitor) { monitor.wait(); } }
            catch(InterruptedException x) { x.printStackTrace(); }
        });
        try { Thread.sleep(1000); } catch(InterruptedException x) { x.printStackTrace(); }
        System.out.println("isCancelled: " + future.cancel(true));
        System.out.println("isDone: " + future.isDone());
        while (pool.getActiveThreadCount() > 0) {
            System.out.println("waiting...");
            try { Thread.sleep(1000); } catch(InterruptedException x) { x.printStackTrace(); }
        }
    }

The test runs forever because the wait() call is not interrupted:

isCancelled: true
isDone: true
waiting...
waiting...
waiting...
waiting...
waiting...
waiting...
...

Do you know how Scala does handle this?

However, it should not hinder us from switching to ForkJoinPool as default ExecutorService.