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

Fri Dec 7 14:59:54 UTC 2018

Thanks Coleen!

Patricio

On 12/6/18 8:00 PM, coleen.phillimore at oracle.com wrote:
> On 12/6/18 7:55 PM, Patricio Chilano wrote:
>> Hi Coleen, David,
>>
>> On 12/6/18 7:19 PM, David Holmes wrote:
>>> On 7/12/2018 8:09 am, coleen.phillimore at oracle.com wrote:
>>>>
>>>> Hi, Sorry this took a while to get to.
>>>>
>>>> In analyzeThreadStackWaiting, is it possible to loop through the 
>>>> stack and not find either OBJECT_WAIT or RUN_METHOD now?
>>>
>>> It looks like it was already possible in the sense that the loop 
>>> doesn't account for it. But that means you'd have to have a 
>>> completely wrong stack for the thread.
>>>
>>>> Could you add a count and a test that you don't drop through the 
>>>> loop, without testing both conditions?
>>>
>>> A simple boolean foundWait/foundRun would suffice I think.
>>
>> I think the loop should at least execute the "if" for the RUN_METHOD 
>> in analyzeThreadStackWaiting(), since before executing 
>> analyzeThreadStackWaiting() we are waiting for the waitThread to 
>> reach the WAITING state.  If the OBJECT_WAIT case does not execute 
>> the test will already fail since the monitorAddress will be null in 
>> the RUN_METHOD case.
>>
>> If you still think it is worth testing that I can add a check that 
>> monitorAddress should not be null before returning from 
>> analyzeThreadStackWaiting().
>
> Thanks for the explanation!  It looks good to me then.
> Coleen
>
>>
>> Thanks,
>> Patricio
>>>
>>> Cheers,
>>> David
>>>
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>> On 12/3/18 11: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
>>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>
>