6913: Add daemon thread information to the console threads view #110

jpstotz · 2020-09-17T11:15:57Z

I had an Java application which did not exit because of a non-daemon thread still running. Therefore I added a column to the treads view that displays if a thread is a daemon thread or not.

oca: has been signed on 2019-01-17 (got positive response from Dalibor Topic on 2019-01-23).

Progress

Commit message must refer to an issue
Change must be properly reviewed

Issue

JMC-6913: Add daemon thread information to the console threads view

Reviewers

Alex Macdonald (@aptmac - Committer)
Guru Hb (@guruhb - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jmc pull/110/head:pull/110
$ git checkout pull/110

bridgekeeper · 2020-09-17T11:19:31Z

Hi @jpstotz, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user jpstotz" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

jpstotz · 2020-09-17T11:34:41Z

/signed

bridgekeeper · 2020-09-17T11:35:37Z

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

aptmac · 2020-09-23T21:15:43Z

Hi @jpstotz !

The new daemon column looks okay and works well. Having said that, this won't work on jvms running on jdk8 [0]; the daemon information is added to java.lang.management.ThreadInfo as of jdk9 [1], and trying to use this column on a jdk8 jvm results in an error dialog (which when paired with the polling nature of the console pages makes this rather difficult to close jmc).

Let me know if you need a JIRA issue created for this PR and I can set one up.

[0] https://docs.oracle.com/javase/8/docs/api/java/lang/management/ThreadInfo.html
[1] https://docs.oracle.com/javase/9/docs/api/index.html?java/lang/management/ThreadInfo.html

jpstotz · 2020-09-24T08:27:59Z

@aptmac Thanks for reviewing my patch.
I had not noticed that there is a Java version restriction on the used ThreadInfo class. As (daemon) threads are nothing new in Java this is really a surprise to me that this information is not available on Java 8. But now it makes perfectly sense why jmc did not already provide such a column.

As mitigation I would assume that the best way would be to check the Java version number before adding the isDaemon in org.openjdk.jmc.console.ui.tabs.threads.ThreadTableSectionPart.
Hopefully the other definitions in ThreadInfoCompositeSupport don't cause any problems if not used.

I will check this out and update my patch.

Regarding the JIRA issue this would be nice of you.

aptmac · 2020-09-24T14:11:46Z

@jpstotz Great, I've created a JIRA issue for this enhancement, it can be found at: https://bugs.openjdk.java.net/browse/JMC-6913

jpstotz · 2020-09-24T16:06:29Z

@aptmac I just noticed that you were not talking about the JVM running JMC but about the JVM JMC is connected to. That makes a version check a bit more complicated, at least I don't see a way to do so with my limited knowledge on JMC.

My current approach uses a Getter in org.openjdk.jmc.console.ui.tabs.threads.ThreadInfoCompositeSupport that catches potential InvalidKeyException that occur when enabling the Daemon column on a Java 8 VM.
In such a case a defined error string like "Data not available" is returned and shown for each thread as value in the Daemon column.

Do you think this is a suitable approach?

mlbridge · 2020-09-28T12:02:00Z

Webrevs

aptmac · 2020-09-29T21:04:38Z

My current approach uses a Getter in org.openjdk.jmc.console.ui.tabs.threads.ThreadInfoCompositeSupport that catches potential InvalidKeyException that occur when enabling the Daemon column on a Java 8 VM.
In such a case a defined error string like "Data not available" is returned and shown for each thread as value in the Daemon column.

Do you think this is a suitable approach?

Yes, catching the exception in the Getter.getMember() could work. Alternatively, it might be cleaner to implement this like IS_DEADLOCKED or GET_ALLOCATED_BYTES, which also perform a check to see if the feature is enabled or not.

…mon() in java.lang.management.ThreadInfo

jpstotz · 2020-10-10T16:18:26Z

@aptmac Sorry for the delay, it took me some time to understand how the information extraction from other JVMs in Missioncontrol works.

In the end I rejected some approaches I first had in mind and ended up with an simple approach that checks if the daemon data has been extracted from the target JVM and if not returns the "data not available" message instead of the actual daemon data.

Hopefully my current approach is what you had in mind when speaking about a clean solution. The two other approaches you mentioned like IS_DEADLOCKED or GET_ALLOCATED_BYTES were not that helpful as they get the data from ThreadMXBean not ThreadInfo and thus require a different way to handle it.

aptmac

@jpstotz Thank you for the revision, this is what I had in mind. I think the code looks good, and I verified that the correct messages are showing on a jdk8 jvm vs. a jdk11 jvm.

One minor nit is that the message texts don't usually end in a period, so other than that needing being changed this looks okay to me.

...nsole.ui/src/main/resources/org/openjdk/jmc/console/ui/messages/internal/messages.properties

jpstotz · 2020-10-19T18:07:07Z

@aptmac I fixed the message text as you suggested.

aptmac

Thanks, the changes look okay to me.

The PR will require at least one more review before it's formally approved and ready to integrate.

openjdk · 2020-10-19T18:13:26Z

⚠️ @jpstotz the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout daemonthread
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

openjdk · 2020-10-19T18:13:28Z

@jpstotz This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

6913: Add daemon thread information to the console threads view

Reviewed-by: aptmac, ghb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 24 new commits pushed to the master branch:

e2e5171: 6944: build.sh does not correctly run UI tests due to not running p2 site
16b3685: 6542: Improve calls to render in the flame view
45449e9: 6911: Third Party Upgrade to HdrHistogram 2.1.12
ea1b02c: 6942: The README is showing the wrong syntax for the build script
8129edd: 6941: build.sh does not terminate jetty after executing '--packageJmc'
1988ad1: 6940: build.sh test arguments wrong
f778c8b: 6695: Build agent as part of checkin tests
fea3cc6: 6846: Parser needs to skip events with no metadata
7de07b4: 6900: Switch from IPredicate to Predicate
5280df6: 6929: Remove obsolete .hgignore file
... and 14 more: https://git.openjdk.java.net/jmc/compare/16513ca16b1421c443c22ce2146819fddb8258c7...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@aptmac, @guruhb) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

thegreystone · 2020-10-19T18:58:28Z

/reviewers 2

openjdk · 2020-10-20T07:07:24Z

@thegreystone
The number of required reviews for this PR is now set to 2 (with at least 1 of role committers).

openjdk · 2020-10-20T07:11:31Z

⚠️ @jpstotz the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout daemonthread
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

openjdk · 2020-10-20T07:11:37Z

@thegreystone
The number of required reviews for this PR is now set to 2 (with at least 1 of role committers).

openjdk · 2020-10-20T07:15:13Z

@thegreystone
The number of required reviews for this PR is now set to 2 (with at least 1 of role committers).

openjdk · 2020-10-20T07:21:36Z

⚠️ @jpstotz the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout daemonthread
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

openjdk · 2020-10-20T07:21:38Z

@jpstotz This change is no longer ready for integration - check the PR body for details.

openjdk · 2020-10-20T07:21:54Z

@thegreystone
The number of required reviews for this PR is now set to 2 (with at least 1 of role committers).

guruhb

Looks good to me

thegreystone · 2020-10-21T16:45:09Z

@jpstotz - you can now type /integrate in a new message to integrate the changes. Thanks for your contribution!

jpstotz · 2020-10-22T07:37:17Z

/integrate

openjdk · 2020-10-22T07:38:34Z

@jpstotz
Your change (at version cf4a0fc) is now ready to be sponsored by a Committer.

thegreystone · 2020-10-24T16:32:54Z

@guruhb - you can sponsor now.

guruhb · 2020-10-27T03:21:54Z

/sponsor

openjdk · 2020-10-27T03:23:19Z

@guruhb @jpstotz Since your change was applied there have been 26 commits pushed to the master branch:

69101e8: 5700: Improve file and socket IO rule result texts
94fd9ff: 6669: FlameGraphView tree calculation should (probably) be interruptable
e2e5171: 6944: build.sh does not correctly run UI tests due to not running p2 site
16b3685: 6542: Improve calls to render in the flame view
45449e9: 6911: Third Party Upgrade to HdrHistogram 2.1.12
ea1b02c: 6942: The README is showing the wrong syntax for the build script
8129edd: 6941: build.sh does not terminate jetty after executing '--packageJmc'
1988ad1: 6940: build.sh test arguments wrong
f778c8b: 6695: Build agent as part of checkin tests
fea3cc6: 6846: Parser needs to skip events with no metadata
... and 16 more: https://git.openjdk.java.net/jmc/compare/16513ca16b1421c443c22ce2146819fddb8258c7...master

Your commit was automatically rebased without conflicts.

Pushed as commit 47e197f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

…38ef4332f7744a-sap sap: Merge 23fb4f2

bridgekeeper bot added the oca label Sep 17, 2020

Threads view: "deamon" column added

398f559

jpstotz force-pushed the daemonthread branch from d6942f8 to 398f559 Compare September 17, 2020 11:34

bridgekeeper bot added the oca-verify label Sep 17, 2020

code formatting

a726248

bridgekeeper bot removed oca oca-verify labels Sep 18, 2020

thegreystone changed the title ~~Threads view: "deamon" column added~~ 6913: Add daemon thread information to the console threads view Sep 24, 2020

openjdk bot added the rfr label Sep 28, 2020

show error message for Java 8 VMs and older that do not provide isDae…

c864b4b

…mon() in java.lang.management.ThreadInfo

aptmac reviewed Oct 19, 2020

View reviewed changes

...nsole.ui/src/main/resources/org/openjdk/jmc/console/ui/messages/internal/messages.properties Outdated Show resolved Hide resolved

message text corrected as requested

cf4a0fc

aptmac approved these changes Oct 19, 2020

View reviewed changes

openjdk bot added the ready label Oct 19, 2020

thegreystone requested review from aptmac and guruhb October 19, 2020 18:58

openjdk bot added the ready label Oct 20, 2020

openjdk bot removed the ready label Oct 20, 2020

openjdk bot added the ready label Oct 20, 2020

openjdk bot removed the ready label Oct 20, 2020

thegreystone requested a review from Gunde October 20, 2020 20:17

thegreystone added the hacktoberfest-accepted label Oct 20, 2020

guruhb approved these changes Oct 21, 2020

View reviewed changes

openjdk bot added the ready label Oct 21, 2020

thegreystone removed the request for review from Gunde October 21, 2020 16:43

openjdk bot added the sponsor label Oct 22, 2020

openjdk bot closed this Oct 27, 2020

openjdk bot added integrated and removed sponsor ready rfr labels Oct 27, 2020

jpstotz deleted the daemonthread branch October 27, 2020 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6913: Add daemon thread information to the console threads view #110

6913: Add daemon thread information to the console threads view #110

jpstotz commented Sep 17, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 17, 2020

jpstotz commented Sep 17, 2020

bridgekeeper bot commented Sep 17, 2020

aptmac commented Sep 23, 2020

jpstotz commented Sep 24, 2020

aptmac commented Sep 24, 2020

jpstotz commented Sep 24, 2020

mlbridge bot commented Sep 28, 2020 •

edited by bridgekeeper bot

Loading

aptmac commented Sep 29, 2020

jpstotz commented Oct 10, 2020

aptmac left a comment

jpstotz commented Oct 19, 2020

aptmac left a comment

openjdk bot commented Oct 19, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 19, 2020 •

edited

Loading

thegreystone commented Oct 19, 2020

openjdk bot commented Oct 20, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 20, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 20, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 20, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 20, 2020

openjdk bot commented Oct 20, 2020 •

edited

Loading

openjdk bot commented Oct 20, 2020

guruhb left a comment

thegreystone commented Oct 21, 2020

jpstotz commented Oct 22, 2020

openjdk bot commented Oct 22, 2020

thegreystone commented Oct 24, 2020

guruhb commented Oct 27, 2020

openjdk bot commented Oct 27, 2020

6913: Add daemon thread information to the console threads view #110

6913: Add daemon thread information to the console threads view #110

Conversation

jpstotz commented Sep 17, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 17, 2020

jpstotz commented Sep 17, 2020

bridgekeeper bot commented Sep 17, 2020

aptmac commented Sep 23, 2020

jpstotz commented Sep 24, 2020

aptmac commented Sep 24, 2020

jpstotz commented Sep 24, 2020

mlbridge bot commented Sep 28, 2020 • edited by bridgekeeper bot Loading

Webrevs

aptmac commented Sep 29, 2020

jpstotz commented Oct 10, 2020

aptmac left a comment

Choose a reason for hiding this comment

jpstotz commented Oct 19, 2020

aptmac left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 19, 2020 • edited by bridgekeeper bot Loading

openjdk bot commented Oct 19, 2020 • edited Loading

thegreystone commented Oct 19, 2020

openjdk bot commented Oct 20, 2020 • edited by bridgekeeper bot Loading

openjdk bot commented Oct 20, 2020 • edited by bridgekeeper bot Loading

openjdk bot commented Oct 20, 2020 • edited by bridgekeeper bot Loading

openjdk bot commented Oct 20, 2020 • edited by bridgekeeper bot Loading

openjdk bot commented Oct 20, 2020

openjdk bot commented Oct 20, 2020 • edited Loading

openjdk bot commented Oct 20, 2020

guruhb left a comment

Choose a reason for hiding this comment

thegreystone commented Oct 21, 2020

jpstotz commented Oct 22, 2020

openjdk bot commented Oct 22, 2020

thegreystone commented Oct 24, 2020

guruhb commented Oct 27, 2020

openjdk bot commented Oct 27, 2020

jpstotz commented Sep 17, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 28, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 19, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 19, 2020 •

edited

Loading

openjdk bot commented Oct 20, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 20, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 20, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 20, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 20, 2020 •

edited

Loading