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
8283224: Remove THREAD_NOT_ALIVE from possible JDWP error codes #10189
Conversation
👋 Welcome back cjplummer! A progress list of the required criteria for merging this PR into |
@plummercj The following labels 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 lists. If you would like to change these labels, use the /label pull request command. |
/label remove core-libs |
@plummercj |
@@ -595,9 +595,6 @@ public void forceEarlyReturn(Value returnValue) throws InvalidTypeException, | |||
case JDWP.Error.THREAD_NOT_SUSPENDED: | |||
throw new IncompatibleThreadStateException( | |||
"Thread not suspended"); | |||
case JDWP.Error.THREAD_NOT_ALIVE: | |||
throw new IncompatibleThreadStateException( | |||
"Thread has not started or has finished"); |
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.
When doing anything here we have to take into account that the debugger (or JDI tool) may be connected to a target VM on an older or newer release. So if I read the issue correctly, the reply to the ThreadReference/ForceEarlyReturn command never returned THREAD_NOT_ALIVE so it's okay to remove the mapping to IncompatibleThreadStateException.
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.
So if I read the issue correctly, the reply to the ThreadReference/ForceEarlyReturn command never returned THREAD_NOT_ALIVE so it's okay to remove the mapping to IncompatibleThreadStateException.
It was not some recent change in the debug agent that eliminated returning THREAD_NOT_ALIVE. However, I can't say for sure that it has never returned it. In fact I would assume at some point it did return it, but that could be as far back as the JVMDI days.
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.
BTW, if any command gets a THREAD_NOT_ALIVE, the following is done:
return new InternalException("Unexpected JDWP Error: " + errorCode, errorCode);
@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 7 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ 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.
This looks good to me.
Thanks,
Serguei
Thanks Serguei and Alan! /integrate |
Going to push as commit aff9a69.
Your commit was automatically rebased without conflicts. |
@plummercj Pushed as commit aff9a69. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
THREAD_NOT_ALIVE originates from JVMTI. However, the debug agent converts it to INVALID_THREAD before passing it on to the debug agent client (the debugger, usually JDI), so debug agent users (JDI) should never see it. Currently ThreadReference.forceEarlyReturn() is the only API that even bothers to check for it, and it throws com.sun.jdi.IllegalThreadStateException, which is the same thing it already does for INVALID_THREAD.
In the JDWP spec I change the description of THREAD_NOT_ALIVE to "Not used". If you have a suggestion for better wording, please let me now. (I was thinking maybe "Unused" would be better.
In the JDI ThreadReference.forceEarlyReturn implementation, I removed the code that checks for THREAD_NOT_ALIVE since it should never occur. There is no behavior change associated with this change, and there is no JDI spec update necessary. The spec already says IllegalThreadStateException means "the thread is not suspended", and not being alive implies not suspended.
Note the JDWP spec for ThreadReference.ForceEarlyReturn used to mention THREAD_NOT_ALIVE as a possible error code, but it was removed as part of the loom work.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10189/head:pull/10189
$ git checkout pull/10189
Update a local copy of the PR:
$ git checkout pull/10189
$ git pull https://git.openjdk.org/jdk pull/10189/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10189
View PR using the GUI difftool:
$ git pr show -t 10189
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10189.diff