RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!

[Robbin Ehn]

On 11/10/2016 10:00 AM, David Holmes wrote:
> Looks fine to me.
+1

Thanks for fixing!

/Robbin
>
> Thanks,
> David
>
> On 10/11/2016 4:56 PM, Ujwal Vangapally wrote:
>> Thanks for the Review, please find the new webrev incorporating the
>> review comments.
>>
>> webrev :
>> http://cr.openjdk.java.net/~asapre/sponsorships/Ujwal/JDK-8168141/webrev.02/
>>
>>
>> -Ujwal.
>>
>> On 11/8/2016 5:18 PM, David Holmes wrote:
>>> Sorry didn't see Staffan's earlier reply :)
>>>
>>> David
>>>
>>> On 8/11/2016 9:23 PM, David Holmes wrote:
>>>> On 8/11/2016 8:44 PM, Robbin Ehn wrote:
>>>>> Hi Ujwal,
>>>>>
>>>>> synchronized(li) {
>>>>>     while (li.received < 1) {
>>>>>         li.wait(100);
>>>>>     }
>>>>> }
>>>>>
>>>>> I don't see why we need while loop ?
>>>>
>>>> You should always perform a wait() in a loop checking the condition that
>>>> is being waited upon. This guards against lost-notifications and also
>>>> spurious wakeups.
>>>>
>>>>> To me it looks like you could just do:
>>>>>
>>>>> synchronized(li) {
>>>>>     li.wait();
>>>>> }
>>>>>
>>>>> Since either we got notification and received must be bigger than 0.
>>>>> Or jtreg timed out.
>>>>
>>>> If the notifyAll() happened before you get here then you will wait()
>>>> until jtreg does time you out - even though the notification correctly
>>>> occurred.
>>>>
>>>> That said, in this particular case doing a timed-wait achieves nothing
>>>> other than waking the thread so that it can go back to waiting again.
>>>> The "received" value will only change when a notifyAll occurs so there
>>>> is no need to poll it (unless aborting due to a timeout as per the
>>>> previous version).
>>>>
>>>> Because the loop will never exit, unless the thread is interrupted, this
>>>> subsequent code has no affect:
>>>>
>>>> 112         if (li.received < 1) {
>>>> 113             throw new RuntimeException("No notif received!");
>>>>
>>>> David
>>>> -----
>>>>
>>>>> /Robbin ('r'eviewer)
>>>>>
>>>>> On 11/04/2016 12:03 PM, Ujwal Vangapally wrote:
>>>>>> Please review this small change for the bug below
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8168141
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~asapre/sponsorships/Ujwal/JDK-8168141/webrev.01/
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Ujwal.
>>