Vert.x TCCL fixes #19487

stuartwdouglas · 2021-08-18T20:15:39Z

No description provided.

Sanne · 2021-08-18T20:39:28Z

...nsions/vertx-core/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxCoreRecorder.java

+                if (launchMode == LaunchMode.DEVELOPMENT) {
+                    devModeThreads.add(thread);
+                }
+                return VertxThreadFactory.super.newVertxThread(target, name, worker, maxExecTime, maxExecTimeUnit);


I suppose you meant to return the existing thread instance?

I mean, rather than invoking the factory once more.

Sanne · 2021-08-18T20:40:42Z

...nsions/vertx-core/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxCoreRecorder.java

-        EnhancedQueueExecutor executor = (EnhancedQueueExecutor) result;
-        final Thread[] runningThreads = executor.getRunningThreads();
-        for (Thread t : runningThreads) {
+        for (var t : devModeThreads) {


Awesome 👍

Sanne · 2021-08-18T20:41:38Z

...loyment/src/main/java/io/quarkus/deployment/configuration/RunTimeConfigurationGenerator.java

@@ -383,7 +383,7 @@ private RunTimeConfigurationGenerator() {
            cc.getFieldCreator(C_BUILD_TIME_CONFIG_SOURCE)
                    .setModifiers(Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC | Opcodes.ACC_FINAL);
            final ResultHandle buildTimeConfigSource = clinit.newInstance(PCS_NEW, buildTimeValues,
-                    clinit.load("Build time config"), clinit.load(100));
+                    clinit.load("Build time config"), clinit.load(Integer.MAX_VALUE));


How is this related?

...loyment/src/main/java/io/quarkus/deployment/configuration/RunTimeConfigurationGenerator.java

famod · 2021-08-18T21:11:08Z

FTR, I confirm that this is fixing that nasty problem we were fighting with the last few days: https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/main.3A.20NPE.20in.20SR.20Config.20ClassPathUtils/near/249911993

Thanks @stuartwdouglas, master of threads, contexts and loaders! 😁

Sanne · 2021-08-18T21:20:36Z

there's a potential race condition between expansion of the threadpool and the context being swapped: not a blocker for the current excellent PR, but we should follow up on that.

Also considering some machines might want to start lots of threads, I'd replace the "copy on write set" with a regular set , as we get rid of the race with a lock.

stuartwdouglas · 2021-08-18T21:51:35Z

@Sanne race should be gone now, and it is also a lot more careful not to capture the original TCCL and cause a leak in dev mode.

quarkus-bot · 2021-08-19T00:53:04Z

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

Failing Jobs - Building `8ed92ed`

Status	Name	Step	Test failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✖	JVM Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs
✔️	JVM Tests - JDK 16

jaikiran · 2021-08-19T04:27:38Z

...nsions/vertx-core/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxCoreRecorder.java

@@ -230,6 +176,10 @@ public void handle(AsyncResult<Void> event) {
            } catch (InterruptedException e) {
                throw new RuntimeException(e);
            }
+            synchronized (devModeThreads) {


Hello Stuart, is there a chance that the threads and classloader will leak if the previous try block throws an exception? In other words, should we use a finally block here to clear the collection and the classloader?

In theory this could happen, I will change it.

quarkus-bot · 2021-08-19T08:01:19Z

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

Failing Jobs - Building `e761880`

Status	Name	Step	Test failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✖	JVM Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs
✔️	JVM Tests - JDK 16
✖	MicroProfile TCKs Tests	`Verify`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ MicroProfile TCKs Tests #

📦 tcks/microprofile-fault-tolerance

✖ org.eclipse.microprofile.fault.tolerance.tck.TimeoutUninterruptableTest.testTimeoutAsyncBulkhead line 190 - More details - Source on GitHub

java.lang.AssertionError: Unexpected exception thrown from Future
	at org.testng.Assert.fail(Assert.java:85)
	at org.eclipse.microprofile.fault.tolerance.tck.util.Exceptions.expect(Exceptions.java:98)
	at org.eclipse.microprofile.fault.tolerance.tck.TimeoutUninterruptableTest.testTimeoutAsyncBulkhead(TimeoutUninterruptableTest.java:190)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at io.quarkus.arquillian.QuarkusProtocol$QuarkusMethodExecutor$1.invoke(QuarkusProtocol.java:87)
	at org.jboss.arquillian.container.test.impl.execution.LocalTestExecuter.execute(LocalTestExecuter.java:57)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base...

famod · 2021-08-19T11:13:55Z

...nsions/vertx-core/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxCoreRecorder.java

@@ -253,9 +204,34 @@ public static Vertx initialize(VertxConfiguration conf, VertxOptionsCustomizer c

        Vertx vertx;

+        Optional<ClassLoader> nonDevModeTccl;


This whole block looks very similar to createThreadFactory(), no?

IMO, the complexity warrants a single method that can be used in both cases, but YMMV!

...nsions/vertx-core/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxCoreRecorder.java

quarkus-bot · 2021-08-20T06:00:27Z

Failing Jobs - Building `8d4a650`

Status	Name	Step	Test failures	Logs	Raw logs
✖	Gradle Tests - JDK 11 Windows	`Build`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.QuarkusDevDependencyDevModeTest.main line 14 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:166)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:939)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:908)
	at io.quarkus.test.devmode.util.DevModeTestUtils.getHttpResponse(DevModeTestUtils.java:130)
	at io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpResponse(QuarkusDevGradleTestBase.java:126)
	at io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpRes...

famod

LGTM and it still fixes the problem I was seeing. 🤓

famod · 2021-08-20T09:58:16Z

I briefly tested a few other things with my current app (e.g. dev mode reload) and it's all looking good so far.

quarkus-bot bot added area/config area/core area/vertx labels Aug 18, 2021

stuartwdouglas force-pushed the reset-tccl branch 2 times, most recently from d03e549 to 1d68df5 Compare August 18, 2021 20:25

stuartwdouglas requested a review from Sanne August 18, 2021 20:26

stuartwdouglas added the triage/backport? label Aug 18, 2021

Sanne reviewed Aug 18, 2021

View reviewed changes

...loyment/src/main/java/io/quarkus/deployment/configuration/RunTimeConfigurationGenerator.java Outdated Show resolved Hide resolved

stuartwdouglas force-pushed the reset-tccl branch 2 times, most recently from d3052a3 to 4a87bb2 Compare August 18, 2021 20:51

Sanne approved these changes Aug 18, 2021

View reviewed changes

stuartwdouglas force-pushed the reset-tccl branch 2 times, most recently from 14a2def to e381f0f Compare August 18, 2021 21:50

stuartwdouglas force-pushed the reset-tccl branch from e381f0f to 8ed92ed Compare August 18, 2021 21:53

stuartwdouglas mentioned this pull request Aug 18, 2021

Bump SmallRye Reactive Messaging version to 3.9.0 #19461

Merged

jaikiran reviewed Aug 19, 2021

View reviewed changes

stuartwdouglas force-pushed the reset-tccl branch from 8ed92ed to e761880 Compare August 19, 2021 04:34

famod reviewed Aug 19, 2021

View reviewed changes

...nsions/vertx-core/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxCoreRecorder.java Outdated Show resolved Hide resolved

Vert.x TCCL fixes

8d4a650

stuartwdouglas force-pushed the reset-tccl branch from e761880 to 8d4a650 Compare August 19, 2021 23:27

famod approved these changes Aug 20, 2021

View reviewed changes

famod merged commit 0cd5c9f into quarkusio:main Aug 20, 2021

quarkus-bot bot added this to the 2.3 - main milestone Aug 20, 2021

gsmet modified the milestones: 2.3 - main, 2.2.0.Final Aug 23, 2021

gsmet removed the triage/backport? label Aug 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vert.x TCCL fixes #19487

Vert.x TCCL fixes #19487

stuartwdouglas commented Aug 18, 2021

Sanne Aug 18, 2021

Sanne Aug 18, 2021

Sanne Aug 18, 2021

Sanne Aug 18, 2021

famod commented Aug 18, 2021

Sanne commented Aug 18, 2021

stuartwdouglas commented Aug 18, 2021

quarkus-bot bot commented Aug 19, 2021 •

edited

Loading

Failing Jobs - Building `8ed92ed`

jaikiran Aug 19, 2021

stuartwdouglas Aug 19, 2021

quarkus-bot bot commented Aug 19, 2021 •

edited

Loading

Failing Jobs - Building `e761880`

Test Failures

⚙️ MicroProfile TCKs Tests #

📦 tcks/microprofile-fault-tolerance

famod Aug 19, 2021

quarkus-bot bot commented Aug 20, 2021

famod left a comment

famod commented Aug 20, 2021

		@@ -253,9 +204,34 @@ public static Vertx initialize(VertxConfiguration conf, VertxOptionsCustomizer c

		Vertx vertx;

		Optional<ClassLoader> nonDevModeTccl;

Vert.x TCCL fixes #19487

Vert.x TCCL fixes #19487

Conversation

stuartwdouglas commented Aug 18, 2021

Sanne Aug 18, 2021

Choose a reason for hiding this comment

Sanne Aug 18, 2021

Choose a reason for hiding this comment

Sanne Aug 18, 2021

Choose a reason for hiding this comment

Sanne Aug 18, 2021

Choose a reason for hiding this comment

famod commented Aug 18, 2021

Sanne commented Aug 18, 2021

stuartwdouglas commented Aug 18, 2021

quarkus-bot bot commented Aug 19, 2021 • edited Loading

Failing Jobs - Building 8ed92ed

jaikiran Aug 19, 2021

Choose a reason for hiding this comment

stuartwdouglas Aug 19, 2021

Choose a reason for hiding this comment

quarkus-bot bot commented Aug 19, 2021 • edited Loading

Failing Jobs - Building e761880

Test Failures

⚙️ MicroProfile TCKs Tests #

📦 tcks/microprofile-fault-tolerance

famod Aug 19, 2021

Choose a reason for hiding this comment

quarkus-bot bot commented Aug 20, 2021

Failing Jobs - Building 8d4a650

Test Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

famod left a comment

Choose a reason for hiding this comment

famod commented Aug 20, 2021

quarkus-bot bot commented Aug 19, 2021 •

edited

Loading

Failing Jobs - Building `8ed92ed`

quarkus-bot bot commented Aug 19, 2021 •

edited

Loading

Failing Jobs - Building `e761880`

Failing Jobs - Building `8d4a650`