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

[David Holmes]

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. ?? That said you can't be waiting on two monitors so I 
don't see how we can ever have the wrong one ??

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


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

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