Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
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.
Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
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.
Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
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.
Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
Hi, On 11/08/2016 01:00 PM, Robbin Ehn wrote: Lost notification, I don't see that? Yes, sorry, I understand what you meant. (was thinking of notification as in Notification) Thanks! /Robbin
Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
Hi David, On 11/08/2016 12:48 PM, David Holmes wrote: Sorry didn't see Staffan's earlier reply :) Thanks anyways! 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. If you both are talking about signals + pthread_cond_wait, we can fix that with e.g. semaphore instead. (since it keeps track of the number of posts) But that's whole nother discussion... :) Lost notification, I don't see that? If the notifyAll() happened before you get here then you will wait() until jtreg does time you out - even though the notification correctly occurred. Correct, might work with move the start of synch block just after listener creation, before addListener, but maybe deadlock prone with mbsc.invoke ... /Robbin
Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
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.
Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
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.
Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
Thanks for the clarification on while loop.
On 11/08/2016 11:58 AM, Staffan Larsen wrote:
Spurious wakeups can cause wait() to finish early in which case the while loop
is needed. But the timeout to wait() doesn’t add anything. And if the while
loop is there the next statement is not needed:
112 if (li.received < 1) {
113 throw new RuntimeException("No notif received!");
Ujwal, please update webrev with the comments from Staffan above.
Thanks!
/Robbin ('r'eviewer)
On 8 Nov 2016, at 11:44, Robbin Ehn wrote:
Hi Ujwal,
synchronized(li) {
while (li.received < 1) {
li.wait(100);
}
}
I don't see why we need while loop ?
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.
/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.
Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
Spurious wakeups can cause wait() to finish early in which case the while loop
is needed. But the timeout to wait() doesn’t add anything. And if the while
loop is there the next statement is not needed:
112 if (li.received < 1) {
113 throw new RuntimeException("No notif received!");
> On 8 Nov 2016, at 11:44, Robbin Ehn wrote:
>
> Hi Ujwal,
>
> synchronized(li) {
> while (li.received < 1) {
> li.wait(100);
> }
> }
>
> I don't see why we need while loop ?
>
> 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.
>
> /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.
Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
Hi Ujwal,
synchronized(li) {
while (li.received < 1) {
li.wait(100);
}
}
I don't see why we need while loop ?
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.
/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.
Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
Looks good. -Harsha On Monday 07 November 2016 09:08 PM, Ujwal Vangapally wrote: Gentle remainder Thanks, Ujwal. On 11/4/2016 4:33 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.
Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
Gentle remainder Thanks, Ujwal. On 11/4/2016 4:33 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.
RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
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.
