Use the QuarkusForkJoinWorkerThreadFactory for fork-join pools #20178

Sanne · 2021-09-15T18:01:25Z

An experiment to see if we can better control the Classloaders in the Fork/Join initiated threads.

@stuartwdouglas WDYT of the approach? For dev-mode we'd likely want a slightly different implementation, so to be able to capture all threads when they are started, and then have the ability to swap the CL.

radcortez · 2021-09-16T09:56:31Z

Gave it a try with #20067 and it seems to fix the issue. I think it makes sensed to move to this approach.

...p/runner/src/main/java/io/quarkus/bootstrap/forkjoin/QuarkusForkJoinWorkerThreadFactory.java

radcortez · 2021-09-23T09:39:27Z

@Sanne @stuartwdouglas are we ok to move forward with this?

Sanne · 2021-09-23T10:54:39Z

Hi @radcortez - yes sorry I resumed yesterday with this.

I was trying to add some tests, but as I worked on it I realized my new integration tests were pointless, as it affects production mode only so @QuarkusTest isn't affected by the issue.

Do we have any existing pattern / utility to test such things? Any suggestion?

radcortez · 2021-09-23T11:15:23Z

Have a look here into how the maven integration tests work: https://github.com/quarkusio/quarkus/blob/main/test-framework/maven/src/main/java/io/quarkus/maven/it/RunAndCheckMojoTestBase.java#L21.

Not exactly what we need, because it runs quarkus:dev, but maybe it can be adjusted (or copied) to run in prod mode. I'm not sure if we have anything else.

I guess that it should be also possible to run the prod jar in pre-integrations-tests and then run tests with failsafe.

Sanne · 2021-09-23T12:09:36Z

thanks for the pointers!

Sanne · 2021-10-06T16:09:09Z

@radcortez apologies I abandoned this during some emergencies, I'm back on it now. I'll try to finish it soon :)

Sanne · 2021-10-06T17:33:02Z

I've cleaned it up a bit and added integration tests. Should be ready.

I tried to track all places in which we need to set the system property; I followed the example of where java.util.logging.manager is being set to org.jboss.logmanager.LogManager, but this also seems to be set in many tests where it should not really be necessary.

In particular we only need to set java.util.concurrent.ForkJoinPool.common.threadFactory in the Quarkus entrypoints for production mode as that's the only mode affected.

quarkus-bot · 2021-10-06T22:30:48Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `9b8f861`

Status	Name	Step	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11 Windows	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 17	`Build`	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/grpc/deployment 
! Skipped: docs integration-tests/devmode integration-tests/grpc-health and 9 more

📦 extensions/grpc/deployment

✖ io.quarkus.grpc.server.devmode.GrpcDevModeTest.testProtoFileChangeReload line 127 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual:

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/grpc/deployment 
! Skipped: docs integration-tests/devmode integration-tests/grpc-health and 9 more

📦 extensions/grpc/deployment

✖ io.quarkus.grpc.server.devmode.GrpcDevModeTest.testProtoFileChangeReload line 127 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual:

⚙️ JVM Tests - JDK 17 #

- Failing: extensions/grpc/deployment 
! Skipped: docs integration-tests/devmode integration-tests/grpc-health and 9 more

📦 extensions/grpc/deployment

✖ io.quarkus.grpc.server.devmode.GrpcDevModeTest.testProtoFileChangeReload line 127 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual:

radcortez · 2021-10-06T23:11:17Z

@radcortez apologies I abandoned this during some emergencies, I'm back on it now. I'll try to finish it soon :)

No worries. Thanks!

radcortez · 2021-10-06T23:14:07Z

Wondering if the test fails are related, but at first glance it doesn't seem like it.

stuartwdouglas · 2021-10-07T01:24:08Z

Failures are unrelated, fixed by #20577

stuartwdouglas · 2021-10-07T01:31:18Z

integration-tests/production-mode/pom.xml

+    <modelVersion>4.0.0</modelVersion>
+
+    <artifactId>production-mode</artifactId>
+    <name>Quarkus - Integration Tests - Quarkus Production Mode</name>


We have heaps of different testing utilities around this already. In particular you can either use:

@QuarkusIntegrationTest: This will test whatever artifact was built (jar, docker image, native etc), so you can just use this and make sure the output is always a jar (i.e. disable native)

QuarkusProdModeTest: This will build and run a jar using Shrinkwrap in a similar manner to QuarkusUnitTest, but actually testing the production jar.

I think we should use one of our existing mechanisms rather than introducing a new one.

I have to admit - I thought I tried to use QuarkusProdModeTest earlier and it seemed like some of its tricks would prevent the issue from manifesting.

But your suggestion got me to look better and I probably was confused - preparing now a cleanup which switches to use QuarkusProdModeTest.

I was also attempting to have this test in the main integration tests, but I'm hitting a series of accidental problems having to coexist with all other tests, so I'd prefer to keep it in this module for now.

core/deployment/src/main/java/io/quarkus/deployment/dev/QuarkusDevModeLauncher.java

Sanne · 2021-10-07T16:50:59Z

Pushed a cleaned up version following @stuartwdouglas 's suggestions.

Thanks!

…oader in the Fork/Join common pool

Sanne · 2021-10-07T21:13:59Z

pushed again, CI highlighted a problem.

This is where core functionality is usually tested, and removes the need for another maven module.

stuartwdouglas · 2021-10-07T23:35:07Z

@Sanne I have pushed a commit on top of this that moves the test to core/test-extension, which is where we usually test core functionality. As much as possible I want to avoid adding new modules unless we really need them. Let me know if this looks ok to you.

things changed

quarkus-bot · 2021-10-08T04:30:41Z

Failing Jobs - Building `1ade67f`

Status	Name	Step	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11 Windows	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 17	`Build`	⚠️ Check →	Logs	Raw logs

Full information is available in the Build summary check run.

⚠️ Errors occurred while downloading the build reports. This report is incomplete.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/oidc

📦 integration-tests/oidc

✖ io.quarkus.it.keycloak.WebsocketOidcTestCase.websocketTest line 49 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <hello [email protected]> but was: <null>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)

⚙️ JVM Tests - JDK 11 Windows #

- Failing: devtools/cli

📦 devtools/cli

✖ io.quarkus.cli.CliProjectMavenTest.testCreateAppDefaults line 61 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 
Expected OK return code. Result:
result: {

✖ io.quarkus.cli.CliProjectMavenTest.testCreateCliDefaults line 38 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <false> but was: <true>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:40)

Sanne · 2021-10-08T07:32:21Z

@Sanne I have pushed a commit on top of this that moves the test to core/test-extension, which is where we usually test core functionality. As much as possible I want to avoid adding new modules unless we really need them. Let me know if this looks ok to you.

Sure, great thanks! I didn't know where to put it, and thought that core/test-extension was something different.

And nice to see the CLI based test :)

Sanne · 2021-10-08T08:29:09Z

@radcortez looks like we're ready to merge; is there any open issue this should be linked to?

radcortez · 2021-10-08T08:45:59Z

@radcortez looks like we're ready to merge; is there any open issue this should be linked to?

I'm not aware of any other options issues. Please go ahead. Thanks!

quarkus-bot bot added area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/jbang Issues related to when using jbang.dev with Quarkus labels Sep 15, 2021

radcortez reviewed Sep 16, 2021

View reviewed changes

...p/runner/src/main/java/io/quarkus/bootstrap/forkjoin/QuarkusForkJoinWorkerThreadFactory.java Show resolved Hide resolved

radcortez linked an issue Sep 16, 2021 that may be closed by this pull request

Config property not found when in ForkJoin common Pool thread #20067

Closed

Sanne force-pushed the FJClassloader branch from b274f6c to b4ce70b Compare October 6, 2021 17:27

quarkus-bot bot added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Oct 6, 2021

Sanne marked this pull request as ready for review October 6, 2021 17:30

Sanne force-pushed the FJClassloader branch from b4ce70b to 9b8f861 Compare October 6, 2021 17:36

radcortez approved these changes Oct 6, 2021

View reviewed changes

stuartwdouglas previously requested changes Oct 7, 2021

View reviewed changes

Sanne force-pushed the FJClassloader branch from 9b8f861 to 997c3ce Compare October 7, 2021 16:49

Sanne requested a review from stuartwdouglas October 7, 2021 16:50

Sanne added 2 commits October 7, 2021 22:12

Introduce a custom QuarkusForkJoinWorkerThread to controll the classl…

98cfec7

…oader in the Fork/Join common pool

Introduce integration tests to cover the production classloader

5c97d4f

Sanne force-pushed the FJClassloader branch from 997c3ce to 5c97d4f Compare October 7, 2021 21:12

Move test to test-extension

1ade67f

This is where core functionality is usually tested, and removes the need for another maven module.

Sanne merged commit 11d725c into quarkusio:main Oct 8, 2021

quarkus-bot bot added this to the 2.4 - main milestone Oct 8, 2021

Sanne deleted the FJClassloader branch October 8, 2021 08:47

radcortez mentioned this pull request Apr 19, 2022

Getting configMapping with ConfigProvider or Arc in a forkjoinpool causes QuarkusConfigFactory not a subtype exception #24896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the QuarkusForkJoinWorkerThreadFactory for fork-join pools #20178

Use the QuarkusForkJoinWorkerThreadFactory for fork-join pools #20178

Sanne commented Sep 15, 2021

radcortez commented Sep 16, 2021

radcortez commented Sep 23, 2021

Sanne commented Sep 23, 2021

radcortez commented Sep 23, 2021

Sanne commented Sep 23, 2021

Sanne commented Oct 6, 2021

Sanne commented Oct 6, 2021

quarkus-bot bot commented Oct 6, 2021 •

edited

Loading

Failing Jobs - Building `9b8f861`

Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/grpc/deployment

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/grpc/deployment

⚙️ JVM Tests - JDK 17 #

📦 extensions/grpc/deployment

radcortez commented Oct 6, 2021

radcortez commented Oct 6, 2021

stuartwdouglas commented Oct 7, 2021

stuartwdouglas Oct 7, 2021

Sanne Oct 7, 2021

Sanne commented Oct 7, 2021

Sanne commented Oct 7, 2021

stuartwdouglas commented Oct 7, 2021

quarkus-bot bot commented Oct 8, 2021

Sanne commented Oct 8, 2021

Sanne commented Oct 8, 2021

radcortez commented Oct 8, 2021

Use the QuarkusForkJoinWorkerThreadFactory for fork-join pools #20178

Use the QuarkusForkJoinWorkerThreadFactory for fork-join pools #20178

Conversation

Sanne commented Sep 15, 2021

radcortez commented Sep 16, 2021

radcortez commented Sep 23, 2021

Sanne commented Sep 23, 2021

radcortez commented Sep 23, 2021

Sanne commented Sep 23, 2021

Sanne commented Oct 6, 2021

Sanne commented Oct 6, 2021

quarkus-bot bot commented Oct 6, 2021 • edited Loading

Failing Jobs - Building 9b8f861

Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/grpc/deployment

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/grpc/deployment

⚙️ JVM Tests - JDK 17 #

📦 extensions/grpc/deployment

radcortez commented Oct 6, 2021

radcortez commented Oct 6, 2021

stuartwdouglas commented Oct 7, 2021

stuartwdouglas Oct 7, 2021

Choose a reason for hiding this comment

Sanne Oct 7, 2021

Choose a reason for hiding this comment

Sanne commented Oct 7, 2021

Sanne commented Oct 7, 2021

stuartwdouglas commented Oct 7, 2021

quarkus-bot bot commented Oct 8, 2021

Failing Jobs - Building 1ade67f

Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/oidc

⚙️ JVM Tests - JDK 11 Windows #

📦 devtools/cli

Sanne commented Oct 8, 2021

Sanne commented Oct 8, 2021

radcortez commented Oct 8, 2021

quarkus-bot bot commented Oct 6, 2021 •

edited

Loading

Failing Jobs - Building `9b8f861`

Failing Jobs - Building `1ade67f`