Limit max pool size for parallel execution via config param

[mpkorstanje]

Overview

With Java 9+ the ForkJoinPool used by JUnit can be configured with a maximum pool size via the junit.jupiter.execution.parallel.config.fixed.max-pool-size property.

While the number of concurrently executing tests may exceed the configured parallelism when tests become blocked, it will not exceed the maximum pool size in these circumstances.

With the following configuration:

junit.jupiter.execution.parallel.enabled=true
junit.jupiter.execution.parallel.config.strategy=fixed
junit.jupiter.execution.parallel.mode.default=concurrent
junit.jupiter.execution.parallel.config.fixed.parallelism=2

This example will report between 2-5 tests running concurrently.

class ExampleTest {

    private static final AtomicInteger running = new AtomicInteger();

    @BeforeEach
    void increment() {
        System.out.println("Running " + running.incrementAndGet());
    }

    @AfterEach
    void decrement() {
        running.decrementAndGet();
    }

    static IntStream numbers() {
        return IntStream.range(0, 1000);
    }

    @ParameterizedTest
    @MethodSource("numbers")
    void test(int i) throws ExecutionException, InterruptedException {
        Runnable sleep = () -> {
            try {
                Thread.sleep(600);
            } catch (InterruptedException e) {
                throw new RuntimeException(e);
            }
        };
        ForkJoinPool.commonPool().submit(sleep).get();
    }
}

By also configuring the max-pool-size we can ensure the concurrently executing test does not exceed the configured 2.

junit.jupiter.execution.parallel.config.fixed.max-pool-size=2

Additionally, because the ForkJoinPool will by default reject tasks that would exceed the maximum pool size the
junit.jupiter.execution.parallel.config.fixed.saturate property has been added and will default to true. There appears to be no reason to ever set this false but it is there should someone depend on the old behavior.

These changes were intentionally not made to the dynamic strategy to limit the scope of this pull request. While I can reasonably predict what behaviour users of the fixed strategy might expect, I can not say the same about the dynamic strategy.

Fixes: #2545
Fixes: #1858
Fixes: #3026

This PR replaces #3027.

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests including corner cases, errors, and exception handling
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes
• All continuous integration builds pass

[mpkorstanje]

@marcphilipp To update the changelog I'd need to know which milestone would go into.

[sbrannen]

To update the changelog I'd need to know which milestone would go into.

I just created 5.10 M1. So feel free to use that.

If it ends up landing in a different milestone, we'll move the change log entry accordingly.

[sbrannen]

Note, however, that there is currently no file for the 5.10 M1 release notes.

So, you'll need to follow the instructions in release-notes-TEMPLATE.adoc.

If that seems too cumbersome, a core committer can create the new file for you.

[see-quick]

Thanks for implementing this! :)

[mpkorstanje]

#3027 (comment)

Fixes: #2545
Fixes: #1858
Fixes: #3026

@mpkorstanje Could you please briefly explain why all three issues would be fixed by merging this PR?

@marcphilipp

For #2545:

While the proposed solution involves a ForkJoinWorkerThreadFactory the underlying root cause follows from the inability to saturate the fork join pool. This was fixed by #2792 and can be achieved through a custom parallel strategy. This PR simplifies the process and should for this specific use case remove the need for more complicated solutions.

If more complicated solutions are needed I expect people will open a new issue to describe their exact problem in more detail.

For #1858:

The root cause was explained in #1858 (comment). When a test puts its executing thread to sleep, JUnit will start another test to keep up the desired parallism. However as explained in #1858 (comment) when using Selenium threads are commonly put to sleep and so it is desirable to limit the maximum pool size to limit the maximum number of web drivers created. This wasn't possible without saturating the pool.

This too was fixed by #2792 and can be achieved through a custom parallel strategy. This PR simplifies the process and should for this specific use case remove the need for more complicated solutions.

For #3026:

Essentially this issues requests a configuration parameter to limit the maximum pool size and allow the pool to be saturated. Removing the need to create a custom class for this very common scenario,

[OPeyrusse]

⚠️ Even though I am not a member of this repository, I would like to warn you that what you did will not work with people also using ForkJoinPool. ⚠️
I explained in this comment on #1858 how a single thread can end up running 40+ tests "concurrently". By concurrently, I mean that 40+ tests can report as having start, before any can complete.
If this is a use case that you plan to support with this PR, I would gladly assist you in designing a test case that reproduce the issue. Don't hesitate to ping me if you want.

[mpkorstanje]

Thanks for the heads-up! I didn't see that specific problem explicitly mentioned in the issues before so I left it out of scope.

I reckon the current PR can still be salvaged by fixing the wording. Limiting the max poolsize via a property is still useful.

Replacing the Fork Join Pool might be a bit more involved. 😅

[OPeyrusse]

@mpkorstanje Do you want me to create a special test and open an issue for that particular problem?

[marcphilipp]

Looks good except for a few minor documentation things.

[mpkorstanje]

@OPeyrusse yes, please do. I can't promise or commit to solving it. But perhaps someone can.

[mpkorstanje]

I think this is what @OPeyrusse had in mind. The code below will report between 2-6 active tests as the .join() call will end up starting other tests. The number of active threads however does not exceed 2.

I've updated the documentation to say concurrent "threads" instead of "tests".

class ExampleTest {
    private static final Runnable sleep = () -> {
        try {
            Thread.sleep(600);
        } catch (InterruptedException e) {
            throw new RuntimeException(e);
        }
    };

    private static final AtomicInteger running = new AtomicInteger();

    @BeforeEach
    void increment() {
        System.out.println("Running " + running.incrementAndGet());
    }

    @AfterEach
    void decrement() {
        running.decrementAndGet();
    }

    static IntStream numbers() {
        return IntStream.range(0, 1000);
    }

    @ParameterizedTest
    @MethodSource("numbers")
    void test(int i) {
        ForkJoinPool.commonPool()
                .submit(sleep)
                .join();
    }

    @ParameterizedTest
    @MethodSource("numbers")
    void test2(int i) {
        ForkJoinPool.commonPool()
                .submit(sleep)
                .join();
    }
}

[mpkorstanje]

@marcphilipp good to go.

[mpkorstanje]

Changes applied!

For fixing small mistakes and typos please do feel free to push directly to my branch.