-
Notifications
You must be signed in to change notification settings - Fork 3k
Fix native applications built with GraalVM 25 --future-defaults options
#50846
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
base: main
Are you sure you want to change the base?
Conversation
|
/cc @Karm (awt) |
This comment has been minimized.
This comment has been minimized.
|
🎊 PR Preview b64659e has been successfully built and deployed to https://quarkus-pr-main-50846-preview.surge.sh/version/main/guides/
|
| } | ||
|
|
||
| @BuildStep(onlyIf = NativeImageFutureDefaults.CompleteReflectionTypes.class) | ||
| void registerReflectionForCompleteReflectionTypes(BuildProducer<ReflectiveClassBuildItem> producer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @galderz , I don't know all details here so just being curious - how did you figure out that this was needed, are the new defaults stricter, or do they come with better diagnostic messages?
Also - is the conditional execution of the buildStep really necessary? I'd generally let the build system decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @galderz , I don't know all details here so just being curious - how did you figure out that this was needed, are the new defaults stricter, or do they come with better diagnostic messages?
The new defaults are stricter. They come with no additional diagnostic messages. The initial future default values are 5, but really it's only 3 since the other 2 are composites. So, I started testing them individually and saw what broke and needed changing. Then I started to add conditionals for each of them. So, in this particular case, this change was only needed when the complete-reflection-types future default was enabled.
Also - is the conditional execution of the buildStep really necessary? I'd generally let the build system decide.
I wanted to minimise the effects of adding support for these future defaults, so I went for the most conservative option of only making changes if any of these future defaults are enabled. I don't expect this option to be something users would typically enable. What seemed important to me is that if they ever enable any of them (or all of them) then things work as expected..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks for the context!
Regarding the conditional though - I'd say let's test such things in all cases? The less conditionals, the better. And we'd get better coverage in preparation for such changes as well.
I'm still a bit puzzled on why this would have worked without it, if it was necessary it surely would have been neccessary even before?
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note that the commits will need some cleanup. I'll have a closer look later.
* Added boolean suppliers to customize build steps only when particular future defaults are enabled. * Build time changes help integration tests pass for suites such as `main`, `smallrye-config`, `opentelemetry`, `spring-data-jpa`, `awt`, and multiple security suites.
168a3b1 to
c927a3c
Compare
Status for workflow
|
|
@gsmet squashed commits |
Status for workflow
|
| Status | Name | Step | Failures | Logs | Raw logs | Build scan |
|---|---|---|---|---|---|---|
| ✔️ | JVM Tests - JDK 17 | Logs | Raw logs | 🚧 | ||
| ✔️ | JVM Tests - JDK 21 | Logs | Raw logs | 🚧 | ||
| ❌ | JVM Tests - JDK 25 | Build |
Failures | Logs | Raw logs | 🚧 |
| ✔️ | JVM Integration Tests - JDK 17 | Logs | Raw logs | 🚧 | ||
| ✔️ | JVM Integration Tests - JDK 17 Windows | Logs | Raw logs | 🚧 | ||
| ❌ | JVM Integration Tests - JDK 21 | Build |
Failures | Logs | Raw logs | 🚧 |
| ✔️ | JVM Integration Tests - JDK 25 | Logs | Raw logs | 🚧 |
Full information is available in the Build summary check run.
You can consult the Develocity build scans.
Failures
⚙️ JVM Tests - JDK 25 #
- Failing: devtools/cli
📦 devtools/cli
❌ io.quarkus.cli.CliProjectJBangTest.testCreateAppDefaults line 64 - History - More details - Source on GitHub
org.opentest4j.AssertionFailedError:
Expected OK return code. Result:
result: {
exitCode: {1},
system_err: {Downloading JBang...
Installing JBang...
[jbang] Downloading JDK 21. Be patient, this can take several minutes...
[jbang] Installing JDK 21...
⚙️ JVM Integration Tests - JDK 21 #
- Failing: integration-tests/virtual-threads/grpc-virtual-threads
📦 integration-tests/virtual-threads/grpc-virtual-threads
❌ io.quarkus.grpc.example.streaming.VertxVirtualThreadTest. - History - More details - Source on GitHub
org.junit.jupiter.engine.execution.ConditionEvaluationException: Failed to evaluate condition [io.quarkus.test.junit.QuarkusTestExtension]: Internal error: Test class was loaded with an unexpected classloader (QuarkusClassLoader:Quarkus Base Runtime ClassLoader: TEST for JUnitQuarkusTest-io.quarkus.grpc.test.utils.VertxGRPCTestProfile (QuarkusTest)@4a891c97) or the thread context classloader (jdk.internal.loader.ClassLoaders$AppClassLoader@639fee48) was incorrect.
at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179)
at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179)
at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
at java.base/java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1685)
...
❌ io.quarkus.grpc.example.streaming.VertxVirtualThreadTest.testStreamingOutputCall - History - More details - Source on GitHub
io.grpc.StatusRuntimeException: INTERNAL: Half-closed without a request
at io.grpc.Status.asRuntimeException(Status.java:532)
at io.grpc.stub.ClientCalls$BlockingResponseStream.hasNext(ClientCalls.java:731)
at java.base/java.util.Iterator.forEachRemaining(Iterator.java:132)
at io.quarkus.grpc.example.streaming.VirtualThreadTestBase.testStreamingOutputCall(VirtualThreadTestBase.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at io.quarkus.test.junit.QuarkusTestExtension.runExtensionMethod(QuarkusTestExtension.java:998)
at io.quarkus.test.junit.QuarkusTestExtension.interceptTestMethod(QuarkusTestExtension.java:846)
Flaky tests - Develocity
⚙️ MicroProfile TCKs Tests
📦 tcks/microprofile-lra
❌ org.eclipse.microprofile.lra.tck.TckRecoveryTests.testCancelWhenParticipantIsUnavailable - History
Expecting the metric Compensated callback was called Expected: a value equal to or greater than <1> but: <0> was less than <1>-java.lang.AssertionError
java.lang.AssertionError:
Expecting the metric Compensated callback was called
Expected: a value equal to or greater than <1>
but: <0> was less than <1>
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.eclipse.microprofile.lra.tck.TckRecoveryTests.assertMetricCallbackCalled(TckRecoveryTests.java:210)
at org.eclipse.microprofile.lra.tck.TckRecoveryTests.testCancelWhenParticipantIsUnavailable(TckRecoveryTests.java:195)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

Closes #46092
The changes in this PR adapt Quarkus to be able to run natives built with
--future-defaultsvalues available in GraalVM 25.0.The changes do not affect Quarkus unless the additional native build time argument via the given parameter is passed in. The possible values can be found in the GraalVM 25.0 release notes. Each of the necessary changes is protected by some build time check that indicates which future defaults are responsible for which changes.
I have tested the flag in this custom Mandrel CI run, which includes Quarkus testsuite (the failures seen in the run are strictly limited to the Mandrel testsuite which has no yet been adjusted for JDK 25 /cc @Karm). To test the flag, I set the additional native build time argument env variable to relevant values to make sure the test passes. The Mandrel team will create a periodic CI that verifies that the testsuite still passes with these arguments.
I run peak and startup performance tests for the
getting-started-reactiveandsecurity-jpa-reactive quickstarts. I didn't see any signinificant changes in peak throughtput. Startup times are unchanged but I've noticed a ~2MB additional startup RSS and this decreases to around ~1MB at peak performance. There is a 1-2 MB increase in binary file size increase too.