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

[David Holmes]

Looks fine to me.

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.
>