[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR JDK-8152589 : java/lang/management/ThreadMXBean/Locks.java fails intermittently, blocked on
From: "Daniel D. Daugherty" <daniel.daugherty () oracle ! com>
Date: 2016-08-31 18:03:55
Message-ID: e459d84d-bdba-6ca3-548f-8ef379657243 () oracle ! com
[Download RAW message or body]
On 8/31/16 2:20 AM, Harsha Wardhana B wrote:
> Hello,
>
> Please review new webrev incorporating nits form Daniel.
>
> http://cr.openjdk.java.net/~hb/8152589/webrev.03/
Replies below.
>
> -Harsha
>
> On Wednesday 31 August 2016 01:47 AM, Daniel D. Daugherty wrote:
>> 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.
This one is not fixed.
>>>>
>>>> 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: */
This one is not fixed the way I requested. All the '*' should line up.
>>>>
>>>> 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.
This one is not fixed the way I requested. All the '*' should line up.
>>>>
>>>> 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
Partially fixed. This:
// block here for LockBThread to hold OBJB
should be rewritten as:
// block here while LockBThread holds OBJB
which makes it more clear that LockAThread is blocking on OBJB
while LockBThread holds that lock.
>>>>
>>>> L208: /*
>>>> L209: Must be invoked from within a Synchronized context
>>>> L210: */
>>>> Same comment about standard block comment format.
This one is not fixed the way I requested. All the '*' should line up.
Dan
>>>>
>>>> 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
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic