RFR 8214148: [TESTBUG] serviceability/tmtools/jstack/WaitNotifyThreadTest.java is not doing what is expected

[Patricio Chilano]

Thanks David!

Patricio
On 12/3/18 11:30 PM, David Holmes wrote:
> Looks good!
>
> Thanks,
> David
>
> On 4/12/2018 2:48 am, Patricio Chilano wrote:
>> Hi David,
>>
>> On 12/3/18 2:14 AM, David Holmes wrote:
>>> Hi Patricio,
>>>
>>> On 1/12/2018 2:31 am, Patricio Chilano wrote:
>>>> Hi David,
>>>>
>>>> On 11/29/18 8:05 PM, David Holmes wrote:
>>>>> Hi Patricio,
>>>>>
>>>>> This seems very complicated and I'm not quite seeing how it all 
>>>>> goes together. The check for waiting to re-lock now seems to 
>>>>> dominant the test and obscure the original checks.
>>>>>
>>>>> I'm not sure this is worthwhile in the context of this test. It 
>>>>> might be much simpler to just get rid of the existing "waiting to 
>>>>> re-lock" check which will not be seen and then if we really want 
>>>>> to check that case add a much simpler form that just checks for that.
>>>> Ok, I actually had similar thoughts while I was adding the extra 
>>>> code. Here is the new webrev:
>>>>
>>>> http://cr.openjdk.java.net/~pchilanomate/8214148.02/webrev/
>>>>
>>>> I removed the check for the "waiting to re-lock" case. 
>>>
>>> Good - the more I look at this test the more I see the "waiting to 
>>> re-lock" case is just not relevant to it.
>>>
>>> The addition of the code to wait until the target thread is in the 
>>> right state seems good.
>>>
>>> I'm unclear on some of the changes to the checking code in 
>>> analyzeThreadStackWaiting. It seems you removed the code that 
>>> watched for finding the wrong monitor and replaced that with a call 
>>> to assertMonitorInfo. But the latter is passed the address you got 
>>> from the MonitorInfo in the first place so the check of the address 
>>> is never going to fail. ?? 
>> I just replaced that "if-else" with assertMonitorInfo() because the 
>> same check "monInfo.getType().equals("waiting on") && 
>> compareMonitorClass(monInfo)" will be done in assertMonitorInfo() and 
>> the error case is also doing the same, so the code gets simplified. 
>> Yes, the extra address check will never fail.
>>
>>> That said you can't be waiting on two monitors so I don't see how we 
>>> can ever have the wrong one ??
>> I'm not sure why this test is checking for the monitor address in 
>> assertMonitorInfo(). The only case where I see it could fail is for 
>> the RUN_METHOD case if the address is null, but that has a separate 
>> check before assertMonitorInfo(). Maybe at some point the test had 
>> more monitors, because there was also that "for" loop checking for 
>> the "waiting to re-lock in wait()" case. I can remove the test 
>> "monInfo.getMonitorAddress().equals(monitorAddress)" in 
>> assertMonitorInfo() but I don't think it hurts to keep it.
>>
>>> A few minor style nits:
>>>
>>> Pre-existing:
>>>
>>> 54             //Notify the waiting thread, so it stops waiting and 
>>> sleeps
>>>
>>> Please add a space after //
>>>
>>>  106         // Start athread that just waits
>>>
>>> s/athread/a thread/
>>>
>>>  145                     throw new RuntimeException(OBJECT_WAIT
>>>  146                             + " method has to contain one lock 
>>> record but it contains " + mi.getLocks().size());
>>>
>>> Indentation is wrong - the '+' should align with the O in OBJECT. 
>>> Break into three lines (at second + if needed)
>>>
>>> New:
>>>
>>> 154                 if(mi.getLocks().size() == 1){
>>>
>>> Space after "if", and space before {
>>>
>>>  157                 else{
>>>
>>> Space before {
>>>
>>> 158                     throw new RuntimeException(RUN_METHOD + " 
>>> method has to contain one lock record but it contains "
>>>  159                                                             + 
>>> mi.getLocks().size());
>>>
>>> Incorrect indentation - '+' should align with R in RUN
>> Done!
>>>> I'm not sure if it's okay to keep the change to 
>>>> serviceability/tmtools/jstack/utils/DefaultFormat.java then. It 
>>>> doesn't really affect this test, but it is needed for jstack to 
>>>> detect the locks that appear in the stack report with the message 
>>>> "waiting to re-lock in wait()".
>>>
>>> I'd probably revert that change at this stage.
>> Done!
>>
>> Here is the new webrev:
>>
>> http://cr.openjdk.java.net/~pchilanomate/8214148.03/webrev/
>>
>>
>> Thanks,
>> Patricio
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Patricio
>>>>> To me the simplest way to see the "re-lock in wait" case is to just:
>>>>>
>>>>> synchronized(obj) {
>>>>>    obj.notifyAll();
>>>>>    <= take stack dump here =>
>>>>> }
>>>>>
>>>>> Cheers,
>>>>> David
>>>>>
>>>>> On 30/11/2018 5:52 am, Daniel D. Daugherty wrote:
>>>>>> Adding serviceability-dev at ... since the 'tmtools' including 'jstack'
>>>>>> are owned by the Serviceability team.
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>> On 11/28/18 4:21 PM, Patricio Chilano wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Could you review this fix for test 
>>>>>>> serviceability/tmtools/jstack/WaitNotifyThreadTest.java?
>>>>>>>
>>>>>>> On one hand the test was not properly checking what it was 
>>>>>>> intended to check, since as mentioned in JBS the logic for 
>>>>>>> checking the method name was wrong. Also since there was only 
>>>>>>> one monitor in the test, the "for" loop with the message 
>>>>>>> "waiting to re-lock in wait()" was never actually reached.
>>>>>>>
>>>>>>> Additionally, with change 8150689 the message "waiting to 
>>>>>>> re-lock in wait()" is now shown in the frame where the relocking 
>>>>>>> is actually taking place, so the logic for checking that should 
>>>>>>> change.
>>>>>>>
>>>>>>> I fixed the first issues and added logic to check for the 
>>>>>>> "waiting to re-lock in wait()" case. I used the Thread.State 
>>>>>>> attribute to check desire states are reached before getting the 
>>>>>>> thread dump reports through jstack. I run the test in mach5 
>>>>>>> several times for all platforms and they all passed.
>>>>>>>
>>>>>>> Webrev URL: 
>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8214148.01/webrev
>>>>>>> Bug URL: https://bugs.openjdk.java.net/browse/JDK-8214148 
>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8150689>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Patricio
>>>>>>
>>>>
>>