-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Remove Quarkus thread dumper. Support native mode thread dumps and jcmd #48956
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
Thanks for your pull request! Your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
JCMD, | ||
JVMSTAT, | ||
JFR, | ||
JMXSERVER, | ||
JMXCLIENT, | ||
NMT, | ||
THREADDUMP, |
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.
Those two new options need documenting in docs/src/main/asciidoc/building-native-image.adoc
as well. Preferably with the minimal GraalVM version that supports it.
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.
Yes thank you for catching this. I've updated the documentation now.
I'd argue the |
I think that enabling |
This comment has been minimized.
This comment has been minimized.
🎊 PR Preview 820c935 has been successfully built and deployed to https://quarkus-pr-main-48956-preview.surge.sh/version/main/guides/
|
Thanks for the concern for my extreme minimalism :) but yes it's ok, let's not remove existing features, we might consider that one day but it would need separate debate. |
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.
It sounds like we cannot remove the thread dump option just yet (unless we'd like to accept the missing feature). At least not until we switch to Mandrel for JDK 25 as default for quarkus. Thoughts?
@@ -933,6 +933,10 @@ include at build time. | |||
|=== | |||
|Monitoring Option |Description |Availability As Of | |||
|
|||
|jcmd | |||
|Include JCMD support | |||
|GraalVM CE for JDK 24 Mandrel 24.2 |
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.
Hmm, the default GraalVM is 23.1 (JDK 21), so we need to do more to check that we only add that option to GraalVM for JDK 24+ native-image providers.
With Mandrel 23.1 I get:
$ native-image --enable-monitoring=threaddump Hello
[...]
Error: The '--enable-monitoring' option contains invalid value(s): threaddump. It can only contain 'heapdump', 'jfr', 'jvmstat', 'jmxserver' (experimental), 'jmxclient' (experimental), or 'all' (deprecated behavior: defaults to 'all' if no argument is provided).
[...]
|threaddump | ||
|Adds support for dumping thread stacks on SIGQUIT/SIGBREAK. | ||
|GraalVM for JDK 24 Mandrel 24.2 |
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.
Same here. We need to check support for this option. Especially when we add the option by default (which is a no for 23.1, so not sure how to proceed without accepting the fact that there won't be thread dumps in some version of quarkus that might still use 23.1).
I see that this has been considered for #44000 as well, but since nmt
isn't common it's a different can of worms.
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.
threaddump
has actually been available since GraalVM 24.0 (I just pushed a commit to correct the docs). However, that still leaves us with the same problem unfortunately :(
Yes, it sounds like it cannot be removed it yet. So now our options are: (A) What we could do instead is add the ability to enable/disable the Quarkus thread dump handler based on the GraalVM version used to build. However, according to Foivos' comment there may be no reliable way to do that (B) Do nothing yet and wait until Mandrel 25 becomes the default. (C) We can choose the next best option from the list. Possibly 1 or 4? |
Status for workflow
|
Status for workflow
|
Status | Name | Step | Failures | Logs | Raw logs | Build scan |
---|---|---|---|---|---|---|
✖ | Native Tests - Windows support | Build |
Failures | Logs | Raw logs | 🔍 |
You can consult the Develocity build scans.
Failures
⚙️ Native Tests - Windows support #
- Failing: integration-tests/liquibase
📦 integration-tests/liquibase
✖ Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-liquibase: Failed to build quarkus application
Summary
This PR removes the Quarkus thread dumping on SIGQUIT/SIGBREAK code. It is no longer needed because GraalVM supports it's own built-in thread dumps. This change is necessary because the Quarkus SIGQUIT/SIGBREAK handler overwrites the GraalVM handler that is required for the AttachAPI to work, which causes
jcmd
support to fail.The
JCMD
andTHREADDUMP
monitoring options have also been added to NativeConfig.java.Please see the related issue for more details: #48871