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 04:51:22 UTC 2016
On 30/08/2016 1:52 PM, Harsha Wardhana B wrote:
> David,
>
> Does accessing under synchronization lock guarantee visibility to
> variable modifications?
Yes. As long as all accesses to the variable are with the
synchronization lock held then you are guaranteed to see all changes.
volatile is only needed for lock-free accesses.
David
> -Harsha
>
> On Tuesday 30 August 2016 08:21 AM, 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.
>>
>> 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