RFR 8214148: [TESTBUG] serviceability/tmtools/jstack/WaitNotifyThreadTest.java is not doing what is expected
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Fri Dec 7 01:00:21 UTC 2018
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
>>>>>>>>
>>>>>>
>>>>
>>>
>
More information about the hotspot-runtime-dev
mailing list