RFR JDK-8152589 : java/lang/management/ThreadMXBean/Locks.java fails intermittently, blocked on wrong object

[Harsha Wardhana B]

Hi David,

You approach to waiter object is a neat little abstraction and I will 
make it a point to use it in future fixes, if required.

revised webrev : http://cr.openjdk.java.net/~hb/8152589/webrev.02/

On Tuesday 23 August 2016 11:47 AM, David Holmes wrote:
> Hi Harsha,
>
> On 22/08/2016 6:48 PM, Harsha Wardhana B wrote:
>> Hello,
>>
>> Please review the below webrev incorporating David's comments.
>>
>> http://cr.openjdk.java.net/~hb/8152589/webrev.01/
>
> Using a static isNotified field isn't exactly what I had in mind, I 
> was thinking of something more encapsulated - something I thought 
> already existed in other tests, but now I can't find it. Eg:
>
> /* synchronization helper for two-thread wait/notify interaction
> */
> static class WaitObject {
>   boolean isNotified = false;
>   public void await() throws InterruptedException {
>     while (!isNotified)
>       wait();
>     isNotified = false;
>   }
>   public void doNotify() {
>     isNotified = true;
>     notify();
>   }
> }
>
> then OBJC would be a WaitObject and you would do:
>
> synchronized(OBJC) {
>    log("WaitingThread about to wait on objC");
>    try {
>        // Signal checker thread, about to wait on objC.
>        waiting = false;
>        p.arriveAndAwaitAdvance(); // Phase 1 (waiting)
>        waiting = true;
>        OBJC.doWait();
>     } catch (InterruptedException e) {
> ...
>
> etc.
>
> Minor nits:
> - why did you move the @library ?
It was suggested by NetBeans Jtreg plugin to fix tag order.
> - @Override, if used, should be applied consistently
Have applied the annotation consistently.
> - if you want to capitalize objA, objB, objC then ensure you also 
> update it in the comments and log statements (it really isn't 
> necessary to capitalize them, that is suggested for compile-time 
> constants and these are not - they are just static final variables).
Done.

Thanks
Harsha
>
>
> Thanks,
> David
>
>> Regards
>>
>> Harsha
>>
>>
>> On Wednesday 17 August 2016 11:45 AM, Harsha Wardhana B wrote:
>>> Hi David,
>>>
>>> I will incorporate changes suggested by you. Let's wait for few more
>>> review comments and then I will send consolidated webrev.
>>>
>>> Regards
>>> Harsha
>>>
>>> On Wednesday 17 August 2016 09:02 AM, David Holmes wrote:
>>>> On 16/08/2016 11:33 PM, Harsha Wardhana B wrote:
>>>>> Hi David,
>>>>>
>>>>> Agreed that we could fix WaitingThread the way you have said, but in
>>>>> recent past, there aren't any issues reported w.r.t WaitingThread.
>>>>
>>>> Nor are there likely to be - that's what makes spurious wakeup bugs
>>>> so difficult to detect!
>>>>
>>>>> This test has been fixed several times (3-4) for intermittent 
>>>>> failures
>>>>> and hence I would not like to meddle with code that is not causing 
>>>>> any
>>>>> problems even though there is scope for refactoring.
>>>>
>>>> It isn't refactoring it is fixing and we have fixed several tests in
>>>> a similar way.
>>>>
>>>>> The issue reported was with LockThreadB and hence I have provided
>>>>> possible fix for the same.
>>>>
>>>> That doesn't preclude fixing other issues with the test at the same
>>>> time.
>>>>
>>>> David
>>>>
>>>>> Thanks
>>>>>
>>>>> Harsha
>>>>>
>>>>>
>>>>> On Tuesday 16 August 2016 01:32 PM, David Holmes wrote:
>>>>>> Hi Harsha,
>>>>>>
>>>>>> On 16/08/2016 4:08 PM, Harsha Wardhana B wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review and provide comments for fix for issue,
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8152589
>>>>>>>
>>>>>>> having webrev located at
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~hb/8152589/webrev.00/
>>>>>>
>>>>>> These changes look quite good (though I have to admit I struggle to
>>>>>> read the lambda and stream code :) ).
>>>>>>
>>>>>> Note that like many of these kinds of tests there is an issue with
>>>>>> WaitingThread because it does not wait in a loop and so is 
>>>>>> susceptible
>>>>>> to spurious wakeups. The way to fix that is to add a "notified"
>>>>>> variable and then do:
>>>>>>
>>>>>> while (!notified)
>>>>>>   wait();
>>>>>>
>>>>>> and set notified before the notify().
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Fix details:
>>>>>>>
>>>>>>> 1. From nightly failures we see that LockThreadB was blocked on 
>>>>>>> wrong
>>>>>>> object. We now do a repeated check with timeout if any given
>>>>>>> thread is
>>>>>>> blocked on expected object. It is possible that LockThreadB might
>>>>>>> still
>>>>>>> be in Phaser call stack (Unsafe.park) when 'checkBlockedObject' is
>>>>>>> invoked.
>>>>>>>
>>>>>>> 2. The logs from lock free logger was never printed. It is not 
>>>>>>> being
>>>>>>> printed.
>>>>>>>
>>>>>>> 3. Any time we see failure, thread stack is being logged. This
>>>>>>> helps us
>>>>>>> ascertain if failure is in test case or in the component.
>>>>>>>
>>>>>>> 4. Even though we had lock free logger, several
>>>>>>> ex.printStackTrace() was
>>>>>>> used which could be responsible for failure. It is removed.
>>>>>>>
>>>>>>> 5. There were several places where tests continue to ran even after
>>>>>>> failure (testFailed flag). That is fixed.
>>>>>>>
>>>>>>> 6. Couple of other minor refactoring.
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> Harsha
>>>>>>>
>>>>>
>>>
>>