RFR JDK-8152589 : java/lang/management/ThreadMXBean/Locks.java fails intermittently, blocked on wrong object
David Holmes
david.holmes at oracle.com
Tue Aug 30 02:51:44 UTC 2016
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.
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