RFR JDK-8152589 : java/lang/management/ThreadMXBean/Locks.java fails intermittently, blocked on wrong object
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Aug 29 23:08:07 UTC 2016
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.
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