Hi Jaroslav,
line 143, notifs should be final, and should be of a type
that supports concurrent access - something like
CopyOnWriteArrayList or Collections.synchronizedList().
Otherwise looks good!
best regards,
-- daniel
On 8/26/14 10:44 AM, Jaroslav Bachorik wrote:
On 08/21/2014 05:34 PM, shanliang wrote:
Jaroslav Bachorik wrote:
On 08/21/2014 03:55 PM, shanliang wrote:
Jaroslav,
The fix should be good to fix the failure.
It makes me think a special case, suppose that the test waits 2
notifications, but the test might receive one unexpected notification
with some more waiting, for example, with the old version, 2 expected
notifications arrive within the first second, and the unexpected
arrives
in the second second, but with your fix the test might end before the
unexpected notification arrives.
Hm, you mean providing a proof that extraneous notifications are not
emitted. I'm not really sure you can create such a proof for the
existing implementation - even if everything is fine within a certain
time window it does not imply that in the next n seconds an unexpected
notification wouldn't be delivered.
Indeed, it is very difficult to make sure no unexpected notification,
but the old version could by chance to get an unexpected because it
waited always certain time.
IMO, getting something right by chance is even worse than simply stating
that the test won't test for such eventuality.
Not sure that we should take care of this case.
Probably not in this test. This test just asserts that all the
expected notifications have been emitted.
No objection. This is a general issue for many other notification tests
too.
In general it is impossible to test for extraneous notifications since
there are no events cleanly defining the time boundaries for receiving a
particular notification. The result is, that even though an extraneous
notification hasn't been received in n seconds we can't be certain that
one wouldn't arrive in n+m (m is a real number and m > 0) seconds.
Could I have a (R)eviewer to take a look at this patch, please?
-JB-
Shanliang
-JB-
Thanks,
Shanliang
Jaroslav Bachorik wrote:
Please, review the following test change.
Issue : https://bugs.openjdk.java.net/browse/JDK-7132590
Webrev: http://cr.openjdk.java.net/~jbachorik/7132590/webrev.00
Currently, the test waits for an arbitrary time until it gives up on
receiving the notifications. This leads to intermittent failures in
situations when the execution is slower than anticipated (running
against a debug build etc.).
The solution is to block the test until all the expected notification
had been delivered or the test is timed out by the harness.
Thanks,
-JB-
