Skip to content

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

roberttoyonaga
Copy link
Contributor

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 and THREADDUMP monitoring options have also been added to NativeConfig.java.


Please see the related issue for more details: #48871

Copy link

quarkus-bot bot commented Jul 16, 2025

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • title should not end up with dot

This message is automatically generated by a bot.

@roberttoyonaga roberttoyonaga changed the title Remove Quarkus thread dumper. Support native mode thread dumps and jcmd. Remove Quarkus thread dumper. Support native mode thread dumps and jcmd Jul 16, 2025
Comment on lines +590 to +596
JCMD,
JVMSTAT,
JFR,
JMXSERVER,
JMXCLIENT,
NMT,
THREADDUMP,
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@jerboaa
Copy link
Contributor

jerboaa commented Jul 16, 2025

I'd argue the threaddump monitoring option would then need to be enabled by default, like the heapdump option currently is so that native builds have thread dumps by default on SIGQUIT like they have now?

@roberttoyonaga
Copy link
Contributor Author

I'd argue the threaddump monitoring option would then need to be enabled by default, like the heapdump option currently is so that native builds have thread dumps by default on SIGQUIT like they have now?

I think that enabling threaddump by default would be okay since this feature basically only installs a signal handler. Based on the comments from the related issue, there does seem to be a preference for keeping the binary as minimal as possible though.

This comment has been minimized.

Copy link

github-actions bot commented Jul 16, 2025

🎊 PR Preview 820c935 has been successfully built and deployed to https://quarkus-pr-main-48956-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

@Sanne
Copy link
Member

Sanne commented Jul 16, 2025

I'd argue the threaddump monitoring option would then need to be enabled by default, like the heapdump option currently is so that native builds have thread dumps by default on SIGQUIT like they have now?

I think that enabling threaddump by default would be okay since this feature basically only installs a signal handler. Based on the comments from the related issue, there does seem to be a preference for keeping the binary as minimal as possible though.

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.

Copy link
Contributor

@jerboaa jerboaa left a 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
Copy link
Contributor

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).
[...]

Comment on lines 964 to 966
|threaddump
|Adds support for dumping thread stacks on SIGQUIT/SIGBREAK.
|GraalVM for JDK 24 Mandrel 24.2
Copy link
Contributor

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.

Copy link
Contributor Author

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 :(

@roberttoyonaga
Copy link
Contributor Author

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?

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?

Copy link

quarkus-bot bot commented Jul 17, 2025

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit cdb913c.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Jul 17, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit cdb913c.

Failing Jobs

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants