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

[Patricio Chilano]

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