The updated webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8134103-jdi-wrong-phase.2/
The change at L152-L153 has been reverted.
Just one sanity check is needed.
Thanks,
Serguei
On 3/2/17 11:21, [email protected] wrote:
Dan,
Thank you for reviewing!
I was waiting for your comments.
On 3/2/17 06:59, Daniel D. Daugherty wrote:
On 3/1/17 8:49 PM, [email protected] wrote:
Please, review the JDK 10 fix for:
https://bugs.openjdk.java.net/browse/JDK-8134103
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8134103-jdi-wrong-phase.1/
src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
old L152: } else if (gdata->vmDead &&
old L153: ((cmd->cmdSet) !=
JDWP_COMMAND_SET(VirtualMachine))) {
The old code used to set the error condition when the VM
is dead and the command was not in the VirtualMachine
command set. With the new code:
L150: } else if (gdata->vmDead) {
The error condition is now set for all command sets
including the VirtualMachine command set. Minimally
that means that this comment needs work:
L152: * VirtualMachine cmdSet quietly
ignores some cmds
L153: * after VM death, so, it sends
it's own errors.
since you are no longer letting the VirtualMachine cmdSet send
its own errors.
Agreed, good catch, thanks!
It's not clear to me why you are now setting the error
condition for VirtualMachine cmds instead of letting those
cmds send their own errors.
See more comments below for your summary.
Most likely, you are right here.
I specifically looked at the VM commands but overlooked the ones that
are silently ignored.
For instance: Resume (9) or exit (10).
So, I will need to restore the L152+L153.
Please, let me re-test this update and then I'll send another webrev.
Interesting that no test has caught this as there is a very tiny gap
for such an intermittent issue to appear.
src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
L243: * Immediately close out any commands enqueued from a
L244: * previously attached debugger.
Perhaps L244 can be change to:
* dead VM or a previously attached debugger.
to match the changed line of code:
L246 if (gdata->vmDead || command->sessionID !=
currentSessionID) {
Summary:
This is an intermittent issue in the debugger back-end (JDWP agent)
that impacts the nightly testing stability.
Congrats on tracking down this elusive bug!
Thanks!
The fix adds check guards of gdata->vmDead condition to the:
- debugLoop (JDWP Event Helper thread) and
- commandLoop (JDWP Transport Listener thread)
The commands are ignored in the DEAD phase.
In the debugLoop, you aren't ignoring the commands, you
are setting an error condition.
Right, I meant: ignored == not executed. :)
The check guard in the debugLoop already existed but only for
VirtualMachine
command set, so it has been extended to commands from all JDWP
command sets.
I'm reading the existing code exactly opposite of what you
say here. The gdata->vmDead check applied to all command
sets except for the VirtualMachine command set. I agree
that you've extended it to all command sets, but we're
not ignoring the commands. We are now setting the error
condition for all command sets.
Agreed.
Maybe I'm missing something here. Perhaps I've been away
from this code for too long... :-)
No, it is that I'm still learning this code with your help. :-)
Thanks a lot!
Serguei
Dan
I suspect, this bug could also cause some of the timeout and
socket related issues.
Testing:
The fix was tested with the nsk.jdi, jtreg com/sun/jdi and 100 runs
of the hotspot/test/closed/compiler/c1/6507107/HeapwalkingTest.java.
The test results are very clean.
Thanks,
Serguei
