Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide more options to configure ForkJoinPoolHierarchicalTestExecutorService's ForkJoinPool #2545

Closed
tmirzoev opened this issue Feb 5, 2021 · 9 comments · Fixed by #3044

Comments

@tmirzoev
Copy link

tmirzoev commented Feb 5, 2021

I've recently encountered a problem similar to #1675 , but a bit worse: provided that all tests use Future.get() at some point and there's enough of them, a user can run into situation where tests start failing with RejectedExecutionException thrown by ForkJoinPool's tryCompensate() method.

This can be avoided by tuning the parameters of ForkJoinPool used in JUnit's ForkJoinPoolHierarchicalTestExecutorService - there are several ways,and I'm using custom ForkJoinWorkerThreadFactory implementation that keeps track of running workers.

The problem is, ForkJoinPoolHierarchicalTestExecutorService and other classes are not very open for such modifications.
I have no way to inject custon thread factory via config. I can not inject already created FJP at construction time, or override a constructor, due to how FJP creation is made in private method called in a constructor.
This goes even further: ForkJoinPoolHierarchicalTestExecutorService parses the configuration with DefaultParallelExecutionConfigurationStrategy.getStrategy() method which is package-private, and JupiterTestEngine has its own share of private methods and is final, so no overriding there.

This results in a situation, where in order to change one argument sent to FJP's constructor, I have to either:
a) provide my own implementations of HierarchicalTestExecutorService, TestEngine and some other classes, which is a bit ridiculous because of how I want to change literally one parameter.
b) copy JUnit's default implementations to my code and change that argument there, which is also not so good due to excessive code and the need to license the copied code under EPL-2.0
c) use Reflection API to modify ForkJoinPoolHierarchicalTestExecutorService at runtime, replacing its FJP, or changing FJP's i
internal variables. This is the most effective solution, but it is rather brittle due to all that reflection stuff.

The point is, is there any possibility for JUnit to provide more options to configure ForkJoinPoolHierarchicalTestExecutorService and its pool, or, at least, to make the API more open to subclassing and modification? I totally understand that these APIs can and will be unstable and experimental, but it could potentially improve using JUnit with any tests that rely on code using Future - like Java's new HTTP API that runs all http requests this way.

@marcphilipp
Copy link
Member

Thanks for raising the issue! Would your custom ForkJoinWorkerThreadFactory implementation be a good default?

@tmirzoev
Copy link
Author

tmirzoev commented Feb 5, 2021

Thank you for quick response!

Basically, my custom implementation checks if (pool.getPoolSize() <= targetParallelism) every time a new ForkJoinWorkerThread is requested, and returns a new thread only if its true, otherwise it returns null.
This limits the number of tests running concurrently strictly to requested parallelism level.

I've yet to see, if it causes any un-obvious problems in our tests, but for now it looks good enough a solution.
I can make a PR if this idea sits well with JUnit team.

@marcphilipp
Copy link
Member

IIRC there's a reason a ForkJoinPool can use more threads than its configured parallelism, e.g. see managedBlock which we use for @ResourceLock. Thus, I don't think your solution would be applicable in general.

@tmirzoev
Copy link
Author

tmirzoev commented Mar 10, 2021

Well, there is a certain possibility that this solution is not for everyone.

I've considered this problem a bit more, and looks like providing more options to configure the ForkJoinPool used by ForkJoinPoolHierarchicalTestExecutorService would be enough.

The ForkJoinPool constructor has parameters maxPoolSize and saturate, which could be configured to achieve the required effect.
These parameters, along with custom ThreadFactory could be injected at FJP constructor call using the customised ParallelExecutionConfiguration.
And if I'm correct, there is a mechanic in place to allow us to provide a custom config class here .

The problem is, at the moment the ParallelExecutionConfiguration interface does not support any way to provide the saturate lambda or Thread factory instance.

So, what I'm proposing is this:

  1. extend the existing configuration with additional getters that return the current default values, like
 Predicate<? super ForkJoinPool> getSaturate(){return null;}

 ForkJoinWorkerThreadFactory getThreadFactory(){return new WorkerThreadFactory();}
  1. Use these additional methods for obtaining the configuration values at ForkJoinPoolHierarchicalTestExecutorService's FJP creation.

This way, the behavior will remain mostly the same for existing users, and in the same time the FJP will become completely configurable.
The main problem I see here is in how the ParallelExecutionConfiguration and DefaultParallelExecutionConfiguration can become coupled with ForkJoinPoolHierarchicalTestExecutorService in, well, providing ForkJoinPool-specific values.
This is totally open to discussion.
A possible solution here could be creating ForkJoinPoolExecutionConfiguration interface/concrete implementation, that would extend/implement ParallelExecutionConfiguration and support the getters for additional values. This new configuration will then be used in ForkJoinPoolHierarchicalTestExecutorService.

As I've stated before, I can make a PR with a solution we'll agree upon.

@marcphilipp
Copy link
Member

Tentatively slated for 5.8 M2 solely for the purpose of team discussion.

@marcphilipp
Copy link
Member

Team decision: Add ForkJoinPoolExecutionConfiguration with the additional getters as described above and check for it in ForkJoinPoolHierarchicalTestExecutorService.

@tmirzoev Would you be interested in submitting a PR?

@sbrannen sbrannen changed the title Providing more options to configure ForkJoinPoolHierarchicalTestExecutorService's ForkJoinPool Provide more options to configure ForkJoinPoolHierarchicalTestExecutorService's ForkJoinPool Jun 7, 2021
@marcphilipp marcphilipp removed this from the 5.8 M2 milestone Aug 13, 2021
@christophs78
Copy link

getSaturate gets covered by #2792

mpkorstanje added a commit to mpkorstanje/junit5 that referenced this issue Sep 9, 2022
The JUnit Platform now supports saturating the ForkJoinPool via a property.
Enabling this property will limit the number of concurrently executing tests
will to the configured parallelism even when some are blocked.

For JUnit Jupiter this can be enabled by through the
`junit.jupiter.execution.parallel.config.dynamic.saturate` and
`junit.jupiter.execution.parallel.config.fixed.saturate` configuration
properties.

Fixes: junit-team#2545
Fixes: junit-team#1858
Fixes: junit-team#3026
mpkorstanje added a commit to mpkorstanje/junit5 that referenced this issue Sep 9, 2022
The JUnit Platform now supports saturating the ForkJoinPool via a property.
Enabling this property will limit the number of concurrently executing tests
will to the configured parallelism even when some are blocked.

For JUnit Jupiter this can be enabled by through the
`junit.jupiter.execution.parallel.config.dynamic.saturate` and
`junit.jupiter.execution.parallel.config.fixed.saturate` configuration
properties.

Fixes: junit-team#2545
Fixes: junit-team#1858
Fixes: junit-team#3026
mpkorstanje added a commit to mpkorstanje/junit5 that referenced this issue Sep 9, 2022
The JUnit Platform now supports saturating the ForkJoinPool via a property.
Enabling this property will limit the number of concurrently executing tests
will to the configured parallelism even when some are blocked.

For JUnit Jupiter this can be enabled by through the
`junit.jupiter.execution.parallel.config.dynamic.saturate` and
`junit.jupiter.execution.parallel.config.fixed.saturate` configuration
properties.

Fixes: junit-team#2545
Fixes: junit-team#1858
Fixes: junit-team#3026
mpkorstanje added a commit to mpkorstanje/junit5 that referenced this issue Sep 24, 2022
The JUnit Platform now supports saturating the ForkJoinPool via a property.
Enabling this property will limit the number of concurrently executing tests
will to the configured parallelism even when some are blocked.

For JUnit Jupiter this can be enabled by through the
`junit.jupiter.execution.parallel.config.dynamic.saturate` and
`junit.jupiter.execution.parallel.config.fixed.saturate` configuration
properties.

Fixes: junit-team#2545
Fixes: junit-team#1858
Fixes: junit-team#3026
mpkorstanje added a commit to mpkorstanje/junit5 that referenced this issue Sep 24, 2022
The JUnit Platform now supports saturating the ForkJoinPool via a property.
Enabling this property will limit the number of concurrently executing tests
will to the configured parallelism even when some are blocked.

For JUnit Jupiter this can be enabled by through the
`junit.jupiter.execution.parallel.config.dynamic.saturate` and
`junit.jupiter.execution.parallel.config.fixed.saturate` configuration
properties.

Fixes: junit-team#2545
Fixes: junit-team#1858
Fixes: junit-team#3026
mpkorstanje added a commit to mpkorstanje/junit5 that referenced this issue Sep 24, 2022
JUnit Jupiter (and The JUnit Platform) now support limiting the
maximum number of concurrently executing tests via the
`junit.jupiter.execution.parallel.config.fixed.max-pool-size` property.

With Java 9+ the `ForkJoinPool` used by JUnit can be configured with a maximum
pool size. While the number of concurrently executing tests may exceed the
configured parallelism when tests become blocked, it will not exceed the
maximum pool size.

With the following configuration:

```properties
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.

```java
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.

```properties
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 behaviour.

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: junit-team#2545
Fixes: junit-team#1858
Fixes: junit-team#3026
mpkorstanje added a commit to mpkorstanje/junit5 that referenced this issue Sep 24, 2022
JUnit Jupiter (and The JUnit Platform) now support limiting the
maximum number of concurrently executing tests via the
`junit.jupiter.execution.parallel.config.fixed.max-pool-size` property.

With Java 9+ the `ForkJoinPool` used by JUnit can be configured with a maximum
pool size. While the number of concurrently executing tests may exceed the
configured parallelism when tests become blocked, it will not exceed the
maximum pool size.

With the following configuration:

```properties
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.

```java
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.

```properties
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 behaviour.

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: junit-team#2545
Fixes: junit-team#1858
Fixes: junit-team#3026
mpkorstanje added a commit to mpkorstanje/junit5 that referenced this issue Sep 24, 2022
JUnit Jupiter (and The JUnit Platform) now support limiting the
maximum number of concurrently executing tests via the
`junit.jupiter.execution.parallel.config.fixed.max-pool-size` property.

With Java 9+ the `ForkJoinPool` used by JUnit can be configured with a maximum
pool size. While the number of concurrently executing tests may exceed the
configured parallelism when tests become blocked, it will not exceed the
maximum pool size.

With the following configuration:

```properties
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.

```java
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.

```properties
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 behaviour.

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: junit-team#3026
Fixes: junit-team#2545
Fixes: junit-team#1858
mpkorstanje added a commit to mpkorstanje/junit5 that referenced this issue Sep 24, 2022
JUnit Jupiter (and The JUnit Platform) now support limiting the
maximum number of concurrently executing tests via the
`junit.jupiter.execution.parallel.config.fixed.max-pool-size` property.

With Java 9+ the `ForkJoinPool` used by JUnit can be configured with a maximum
pool size. While the number of concurrently executing tests may exceed the
configured parallelism when tests become blocked, it will not exceed the
maximum pool size.

With the following configuration:

```properties
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.

```java
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.

```properties
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 behaviour.

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: junit-team#3026
Fixes: junit-team#2545
Fixes: junit-team#1858
mpkorstanje added a commit to mpkorstanje/junit5 that referenced this issue Sep 24, 2022
JUnit Jupiter (and The JUnit Platform) now support limiting the
maximum number of concurrently executing tests via the
`junit.jupiter.execution.parallel.config.fixed.max-pool-size` property.

With Java 9+ the `ForkJoinPool` used by JUnit can be configured with a maximum
pool size. While the number of concurrently executing tests may exceed the
configured parallelism when tests become blocked, it will not exceed the
maximum pool size.

With the following configuration:

```properties
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.

```java
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.

```properties
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 behaviour.

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: junit-team#3026
Fixes: junit-team#2545
Fixes: junit-team#1858
mpkorstanje added a commit to mpkorstanje/junit5 that referenced this issue Oct 3, 2022
JUnit Jupiter (and The JUnit Platform) now support limiting the
maximum number of concurrently executing tests via the
`junit.jupiter.execution.parallel.config.fixed.max-pool-size` property.

With Java 9+ the `ForkJoinPool` used by JUnit can be configured with a maximum
pool size. While the number of concurrently executing tests may exceed the
configured parallelism when tests become blocked, it will not exceed the
maximum pool size.

With the following configuration:

```properties
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.

```java
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.

```properties
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 behaviour.

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: junit-team#3026
Fixes: junit-team#2545
Fixes: junit-team#1858
@marcphilipp marcphilipp added this to the 5.10 M1 milestone Oct 21, 2022
@marcphilipp
Copy link
Member

Reopening to discuss backporting to 5.9.2.

@marcphilipp marcphilipp reopened this Dec 30, 2022
@marcphilipp marcphilipp modified the milestones: 5.10.0-M1, 5.9.2 Dec 30, 2022
@marcphilipp marcphilipp self-assigned this Dec 30, 2022
@marcphilipp
Copy link
Member

Team decision: Include in 5.9.2

marcphilipp pushed a commit that referenced this issue Jan 6, 2023
JUnit Jupiter (and The JUnit Platform) now support limiting the maximum
number of concurrently executing tests via the
`junit.jupiter.execution.parallel.config.fixed.max-pool-size`
configuration parameter.

With Java 9+ the `ForkJoinPool` used by JUnit can be configured with a
maximum pool size. While the number of concurrently executing tests may
exceed the configured parallelism when tests become blocked, it will
not exceed the maximum pool size.

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 behavior users of the `fixed` strategy might expect, I can not say
the same about the `dynamic` strategy.

Fixes #3026.
Fixes #2545.
Fixes #1858.
marcphilipp added a commit that referenced this issue Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment