-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
8232839: JDI AfterThreadDeathTest.java failed due to "FAILED: Did not get expected IllegalThreadStateException on a StepRequest.enable()" #14372
Conversation
👋 Welcome back cjplummer! A progress list of the required criteria for merging this PR into |
@plummercj The following label will be automatically applied to this pull request:
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. |
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 looks good.
Thanks,
Serguei
@plummercj 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:
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 20 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
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.
I think it makes sense!
thanks for the reviews Kevin and Serguei! /integrate |
Going to push as commit 84184f9.
Your commit was automatically rebased without conflicts. |
@plummercj Pushed as commit 84184f9. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The test waits for a ThreadDeathEvent for "main". Once that arrives, it then waits for the next ThreadStartEvent (for any thread). Once it arrives, the test tries to create and enable a StepRequest on the "main" thread. Since "main" is supposedly dead, the expectation is an IllegalThreadStateException. However, it turns out that sometimes the enabling can in fact succeed.
Just because a ThreadDeathEvent has been received for a thread does not mean you can no longer do things with the thread like suspend it or enable a StepEvent. There is a short delay in the debug agent after sending the ThreadDeathEvent before it stops tracking the thread. The thread can still be acted upon until then. The JDWP and JDI specs seem to support doing this:
What this means is that when the test receives some arbitrary ThreadStartEvent after the "main" ThreadDeathEvent has been received, the test may in fact still be able to enable a StepRequest on the "main" thread, causing the test to fail. What seems to trigger the failure is receiving an unexpected spurious ThreadStartEvent such as from the Common-Clean thread or a carrier thread, although even then it only fails some of the time. In fact if I modify the test to enable the StepRequest when it receives the ThreadDeathEvent for "main", it still almost always passes, but will fail more frequently than it normally does.
It seems if the test always waits for the ThreadStartEvent for "DestroyJavaVM", then the "main" thread is truly gone by then and the test always passes, so this is how I've chosen to fix the issue.
Tested with tier1, tier2 svc tests, and tier5 svc tests.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14372/head:pull/14372
$ git checkout pull/14372
Update a local copy of the PR:
$ git checkout pull/14372
$ git pull https://git.openjdk.org/jdk.git pull/14372/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14372
View PR using the GUI difftool:
$ git pr show -t 14372
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14372.diff
Webrev
Link to Webrev Comment