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

[David Holmes]

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 ?
- @Override, if used, should be applied 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).


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