RFR JDK-8152589 : java/lang/management/ThreadMXBean/Locks.java fails intermittently, blocked on wrong object
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Aug 30 20:17:08 UTC 2016
On 8/29/16 8:51 PM, David Holmes wrote:
>
>
> On 30/08/2016 9:08 AM, Daniel D. Daugherty wrote:
>> On 8/23/16 6:17 AM, Harsha Wardhana B wrote:
>>> 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/
>>
>> test/java/lang/management/ThreadMXBean/Locks.java
>> General comment:
>> 'synchronized(foo)' and 'synchronized (foo)' are both used.
>> Please pick a style and use it consistently.
>>
>> L24: /*
>> Space added before comment block begin. Please remove it.
>>
>> L76: /*
>> L77: Handy debug function to check if error condition is because
>> of test code or not.
>> L78: */
>> Should look like this:
>>
>> L76: /*
>> L77: * Handy debug function to check if error condition is
>> because of test code or not.
>> L78: */
>>
>> That is standard block comment format.
>>
>> L114: /*
>> L115: Do slow check if thread is blocked on a lock. It is
>> possible that last thread
>> L116: to come out of Phaser might still be in Phaser call stack
>> (Unsfe.park) and
>> L117: hence might eventually acquire expected lock.
>> L118: */
>> Same comment about standard block comment format.
>>
>> Also typo: "Unsfe.park" -> "Unsafe.park".
>>
>> L129: while(p.test(result)){
>> Space before first '(' and before '{'.
>>
>> L171: // stop here for LockBThread to hold OBJB
>> Perhaps this for the comment:
>> // block here while LockBThread holds OBJB
>>
>> L208: /*
>> L209: Must be invoked from within a Synchronized context
>> L210: */
>> Same comment about standard block comment format.
>>
>> Also not sure why "Synchronized" is capitalized...
>>
>> L213: boolean isNotified = false;
>> This should probably be volatile.
>>
>> Update: WaitingThread calls OBJC.doWait() and
>> CheckerThread calls OBJC.doNotify(). Yup, it should
>> be volatile.
>
> Nope - it is only accessed under synchronization lock, which must be
> held by the caller else the wait()/notify() will throw
> IllegalMonitorStateException.
Yup. That occurred to me late last night... Sorry for the noise.
Dan
>
> David
> -----
>
>> Dan
>>
>>
>>>
>>> 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
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>
>>
More information about the serviceability-dev
mailing list