Hello All,
Could I get review from one more Reviewer?
Thanks
Harsha
On Wednesday 24 August 2016 10:07 AM, harsha.wardhan...@oracle.com wrote:
Thanks David.
Harsha
On Wed, Aug 24, 2016 at 6:55 AM +0530, "David Holmes"
<david.hol...@oracle.com <mailto:david.hol...@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
>>>>>>>>
>>>>>>
>>>>
>>>
>
