Skip to content

8309397: com/sun/jdi/JdbXXX tests fail due to not being run with -Djdk.trackAllThreads #14293

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

Closed
wants to merge 1 commit into from

Conversation

plummercj
Copy link
Contributor

@plummercj plummercj commented Jun 2, 2023

The com/sun/jdi/JdbXXX tests rely on the jdb "threads" command output to find the main thread. If it is a virtual thread, it will not be included in the "threads" output unless the debuggee is run with -Djdk.trackAllThreads, so we need to make sure to include this option when launching the debuggee. The following tests are impacted.

com/sun/jdi/JdbMethodExitTest.java
com/sun/jdi/JdbStepTest.java
com/sun/jdi/JdbStopThreadTest.java
com/sun/jdi/JdbStopThreadidTest.java

Note that all these tests also fail due to JDK-8309334, which needs to be fixed first. Also JdbMethodExitTest.java will fail due to JDK-8309396, which should be fixed after this CR.

I've tested with mach5 tier5 in a workspace that has integrated the various CRs mentioned. Once JDK-8309334 is fixed, before integrating this PR I'll first merge and verify that the 3 tests being removed from the problem list by this PR also pass.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8309397: com/sun/jdi/JdbXXX tests fail due to not being run with -Djdk.trackAllThreads

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14293/head:pull/14293
$ git checkout pull/14293

Update a local copy of the PR:
$ git checkout pull/14293
$ git pull https://git.openjdk.org/jdk.git pull/14293/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14293

View PR using the GUI difftool:
$ git pr show -t 14293

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14293.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 2, 2023

👋 Welcome back cjplummer! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title 8309397 8309397: com/sun/jdi/JdbXXX tests fail due to not being run with -Djdk.trackAllThreads Jun 2, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 2, 2023
@openjdk
Copy link

openjdk bot commented Jun 2, 2023

@plummercj The following label will be automatically applied to this pull request:

  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the serviceability serviceability-dev@openjdk.org label Jun 2, 2023
@mlbridge
Copy link

mlbridge bot commented Jun 2, 2023

Webrevs

@AlanBateman
Copy link
Contributor

Would it be possible to expand a bit on why jdk.trackAllThreads is needed? I would have expected jdb "threads" command to be sensitive to whether the debuggee is launched with the JDWP includevirtualthreads option but jdk.trackAllThreads isn't used by the JDWP agent, it is instead used to determine if virtual threads created directly with the Thread API show up in thread dumps or not.

@plummercj
Copy link
Contributor Author

Would it be possible to expand a bit on why jdk.trackAllThreads is needed? I would have expected jdb "threads" command to be sensitive to whether the debuggee is launched with the JDWP includevirtualthreads option but jdk.trackAllThreads isn't used by the JDWP agent, it is instead used to determine if virtual threads created directly with the Thread API show up in thread dumps or not.

I think you are right that this change should not be needed. I was working on addressing a number of different issues with launching debuggees properly and the tests being able to discover virtual threads, and I think maybe I had done this "fix" at some point, and then did JDK-8309334, (and JDK-8309396), and thought that this fix was still needed. I thought for sure it was helping, but I've removed it and the tests still seem to pass with just JDK-8309334 and JDK-8309396.

jdb by default does not track all virtual threads. It does have a a -trackallthreads command line option, which triggers passing includevirtualthreads=y to the debug agent (assuming jdb is actually doing the launching and not just attaching). However, given that these tests do not pass includevirtualthreads=y, I didn't find myself wondering how they were passing after just fixing JDK-8309334. The answer is that jdb does track any virtual thread that it gets an event for, so as it turns out when these 4 jdb tests look for a specific thread in the threads output, it's always a thread that jdb already learned about via an event. I might do a separate PR just to force either -trackallthreads or includevirtualthreads=y, but currently that doesn't seem to be needed by any of the jdb tests.

So I think I should close this PR and update the JDK-8309334 PR to remove the 3 tests that seem to pass with it and don't also need this PR to pass. The 4th test com/sun/jdi/JdbMethodExitTest.java needs an additional fix that I need to push separately.

@AlanBateman
Copy link
Contributor

So I think I should close this PR and update the JDK-8309334 PR to remove the 3 tests that seem to pass with it and don't also need this PR to pass. The 4th test com/sun/jdi/JdbMethodExitTest.java needs an additional fix that I need to push separately.

Okay with me, thanks for confirming that jdk.trackAllThreads has no effect here.

@plummercj plummercj closed this Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfr Pull request is ready for review serviceability serviceability-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.

2 participants