RFR JDK-8152589 : java/lang/management/ThreadMXBean/Locks.java fails intermittently, blocked on wrong object
Harsha Wardhana B
harsha.wardhana.b at oracle.com
Mon Aug 29 10:35:36 UTC 2016
Hello All,
Could I get review from one more Reviewer?
Thanks
Harsha
On Wednesday 24 August 2016 10:07 AM, harsha.wardhana.b at oracle.com wrote:
>
> Thanks David.
>
> Harsha
>
>
>
>
> On Wed, Aug 24, 2016 at 6:55 AM +0530, "David Holmes"
> <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
>
> On 23/08/2016 10:17 PM, 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.
>
> Note it only works for a single waiting thread. :)
>
> > revised webrev : http://cr.openjdk.java.net/~hb/8152589/webrev.02/
>
> This looks good, thanks for your patience with this.
>
> Cheers,
> David
>
> > 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
> >>>>>>>>
> >>>>>>
> >>>>
> >>>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160829/2d21a05a/attachment.html>
More information about the serviceability-dev
mailing list