RFR(S): 8134103: JVMTI_ERROR_WRONG_PHASE(112): on checking for an interface

[Daniel D. Daugherty]

On 3/2/17 2:24 PM, serguei.spitsyn at oracle.com wrote:
> 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.

This comment from earlier review is still unresolved:

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) {

Sorry I didn't notice that you didn't reply to it earlier...

I presume you are retesting... How often does
hotspot/test/closed/compiler/c1/6507107/HeapwalkingTest.java reproduce
the problem?

Dan

>
> Thanks,
> Serguei
>
>
> On 3/2/17 11:21, serguei.spitsyn at oracle.com 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, serguei.spitsyn at oracle.com 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
>>>
>>
>