Vertx: use NoopShutdownExecutorService and DevModeExecutorService #38478
Conversation
mkouba
changed the title
| // The first one is used for the worker thread pool, the second one is used internally, | ||
| // and additional executors may be created on demand | ||
| // Unfortunately, there is no way to distinguish the particular executor types | ||
| // Therefore, we only consider the first one as the worker thread pool |
There was a problem hiding this comment.
This is IMO quite risky but the Vertx API does not provide a way to distinguish the particular executor types... 🤷
There was a problem hiding this comment.
The internal one is for file system operations only.
There was a problem hiding this comment.
I fear that this may actually break @Blocking specifying a worker pool name.
There was a problem hiding this comment.
ok, I think it was already broken...
We could change the Vert.x SPI to get the "type" (worker, internal, custom)
There was a problem hiding this comment.
My point is that the ordering can change any time. The working thread pool is here: https://github.com/eclipse-vertx/vert.x/blob/master/src/main/java/io/vertx/core/impl/VertxImpl.java#L190. And the internal pool is here: https://github.com/eclipse-vertx/vert.x/blob/master/src/main/java/io/vertx/core/impl/VertxImpl.java#L193. Just a few lines below. You see?
There was a problem hiding this comment.
We could change the Vert.x SPI to get the "type" (worker, internal, custom)
Yes, that would be helpful.
CC @jponge
This comment has been minimized.
This comment has been minimized.
...nsions/vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/DevModeExecutorService.java
Show resolved
Hide resolved
...ons/vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/ForwardingExecutorService.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
- in the prod mode we wrap the ExecutorService and the shutdown() and shutdownNow() are deliberately not delegated - this is a workaround to solve the problem described in quarkusio#16833 (comment) - the Vertx instance is closed before io.quarkus.runtime.ExecutorRecorder.createShutdownTask() is used - and when it's closed the underlying worker thread pool (which is in the prod mode backed by the ExecutorBuildItem) is closed as well - as a result the quarkus.thread-pool.shutdown-interrupt config property and logic defined in ExecutorRecorder.createShutdownTask() is completely ignored
- in dev mode we use a special executor for the worker pool where the underlying executor can be shut down and then replaced with a new re-initialized executor - this is a workaround to solve the problem described in quarkusio#16833 (comment) - the Vertx instance is reused between restarts but we must attempt to shut down this executor, for example to cancel/interrupt the scheduled methods
This comment has been minimized.
This comment has been minimized.
mkouba
added
the
triage/waiting-for-ci
Status for workflow
|
| Status | Name | Step | Failures | Logs | Raw logs | Build scan |
|---|---|---|---|---|---|---|
| ✔️ | JVM Tests - JDK 17 | Failures | Logs | Raw logs | 🔍 | |
| ✔️ | JVM Tests - JDK 21 | Logs | Raw logs | 🔍 | ||
| ✔️ | MicroProfile TCKs Tests | Failures | Logs | Raw logs | 🔍 | |
| ✔️ | Native Tests - Data1 | Failures | Logs | Raw logs | 🔍 | |
| ✔️ | Quickstarts Compilation - JDK 17 | Failures | Logs | Raw logs | 🚧 |
Full information is available in the Build summary check run.
You can consult the Develocity build scans.
Failures
⚙️ JVM Tests - JDK 17 #
- Failing: extensions/smallrye-health/deployment
! Skipped: extensions/agroal/deployment extensions/elytron-security-jdbc/deployment extensions/flyway/deployment and 189 more📦 extensions/smallrye-health/deployment
✖ io.quarkus.smallrye.health.test.DispatchedThreadTest.check line 33 - More details - Source on GitHub
java.lang.AssertionError:
1 expectation failed.
JSON path checks.data.request[0] doesn't match.
Expected: is <true>
Actual: <false>
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
⚙️ MicroProfile TCKs Tests #
- Failing: tcks/resteasy-reactive
📦 tcks/resteasy-reactive
✖ Failed to execute goal org.codehaus.mojo:exec-maven-plugin:3.0.0:exec (test) on project quarkus-tck-resteasy-reactive: Command execution failed.
⚙️ Native Tests - Data1 #
- Failing: integration-tests/hibernate-orm-tenancy/connection-resolver
📦 integration-tests/hibernate-orm-tenancy/connection-resolver
✖ io.quarkus.it.hibernate.multitenancy.inventory.HibernateNamedPersistenceUnitTestInGraalITCase.testGetPlanesDefaultTenant - More details - Source on GitHub
java.lang.AssertionError:
1 expectation failed.
Expected status code is <200> but was <500>.
at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480)
at org.codehaus.groovy.reflection.CachedConstructor.invoke(CachedConstructor.java:73)
at org.codehaus.groovy.runtime.callsite.ConstructorSite$ConstructorSiteNoUnwrapNoCoerce.callConstructor(ConstructorSite.java:108)
✖ io.quarkus.it.hibernate.multitenancy.inventory.HibernateNamedPersistenceUnitTestInGraalITCase.testGetPlanesTenantMycompany - More details - Source on GitHub
java.lang.AssertionError:
1 expectation failed.
Expected status code is <200> but was <500>.
at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480)
at org.codehaus.groovy.reflection.CachedConstructor.invoke(CachedConstructor.java:73)
at org.codehaus.groovy.runtime.callsite.ConstructorSite$ConstructorSiteNoUnwrapNoCoerce.callConstructor(ConstructorSite.java:108)
⚙️ Quickstarts Compilation - JDK 17 #
- Failing: reactive-messaging-http-quickstart reactive-messaging-websockets-quickstart
📦 reactive-messaging-http-quickstart
✖ Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:generate-code (default) on project reactive-messaging-http-quickstart: Quarkus code generation phase has failed
📦 reactive-messaging-websockets-quickstart
✖ Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:generate-code (default) on project reactive-messaging-websockets-quickstart: Quarkus code generation phase has failed
quarkus-bot
bot
added
kind/bugfix
and removed
triage/waiting-for-ci
This PR attempts to fix two problems described in #16833 (comment). Both are related to the Vertx worker thread pool implementation.
NoopShutdownExecutorService
In prod mode, we wrap the
ExecutorServiceand theshutdown()andshutdownNow()methods are deliberately not delegated. This is a workaround to solve the problem described in #16833 (comment). The problem: the Vertx instance is closed beforeio.quarkus.runtime.ExecutorRecorder.createShutdownTask()is used and when it's closed the underlying worker thread pool (which is in the prod mode backed by theExecutorBuildItem) is closed as well. As a result thequarkus.thread-pool.shutdown-interruptconfig property and logic defined inExecutorRecorder.createShutdownTask()is completely ignored.DevModeExecutorService
In dev mode we use a special executor for the worker pool where the underlying executor can be shut down and then replaced with a new re-initialized executor. This is a workaround to solve the problem described in #16833 (comment). The problem: the Vertx instance is reused between restarts but we must attempt to shut down this executor, for example to cancel/interrupt the scheduled methods.
Fixes #16833