jmx-dev RFR 7132590: javax/management/remote/mandatory/notif/NotificationAccessControllerTest.java fails in JDK8-B22

[Daniel Fuchs]

On 8/26/14 12:41 PM, Jaroslav Bachorik wrote:
> On 08/26/2014 12:08 PM, Daniel Fuchs wrote:
>> Hi Jaroslav,
>>
>> line 143, notifs should be final, and should be of a type
>> that supports concurrent access - something like
>> CopyOnWriteArrayList or Collections.synchronizedList().
>
> Taken care of. I also took the liberty to get rid off few compiler
> warnings.
>
> http://cr.openjdk.java.net/~jbachorik/7132590/webrev.01

Looks good!

-- daniel

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