jmx-dev [PATCH] JDK-8005472: com/sun/jmx/remote/NotificationMarshalVersions/TestSerializationMismatch.sh failed on windows

Wed May 15 13:12:00 PDT 2013

On Wed 15 May 2013 12:44:57 AM CEST, Stuart Marks wrote:
> Hi, sorry for the delay in my reply, and thanks for the update.
>
> A timeout of 30 seconds should be sufficient.
>
> Regarding duplicates: I was just thinking, if you're expecting exactly
> 10 notifications, you should ensure that you receive exactly 10
> notifications, and they're the right ones. But if duplicates is the
> only possible way that more than 10 notifications could occur, then
> that's probably fine. I just don't know enough about JMX to know.

According to the JMX specification you must receive each notification 
at most once so the duplicate check will detect a possible bug.

>
> Now... I took a look at the Client.java implementation, and ... groan
> (sorry) ... I see you're using the single-element array trick to work
> around the inability to mutate local variables from an anonymous inner
> class.
>
> I strongly recommend that you avoid using this trick.
>
> I don't know what thread the notification callbacks run on. If they
> are on different threads, then there are inherent memory visibility
> problems, since array elements cannot be declared volatile. There is
> also potential concurrent access to seqSet, since it's accessed from
> both callbacks.

Ok. I've changed it to use AtomicBoolean Thanks for pointing this out. 
Even though it's rather obvious I got so used to using the final array 
in anonymous inner classes that I didn't even stop and think. BTW, 
there are a lot of places in the tests using this idiom in potentially 
multithreaded environment :(

>
> (Hm, seqSet isn't modified anywhere. Should the notification sequence
> numbers be added to the set somewhere?)

Yes, my bad. Instead of *.contains() I should have used *.add(). Fixed 
now.

>
> One approach for dealing with this is to make all the mutable data
> structures thread-safe, e.g. by wrapping seqSet using
> Collections.synchronizedSet() and using an AtomicBoolean instead of
> boolean[1].

Yep, the set is synchronized. I was thinking about 
ConcurrentSkipListSet but for the sakes of the test I think the 
Collections.synchronizedSet() is just fine.

>
> Another approach would be to have the callbacks just append the
> notification objects to a synchronized list, and then have the main
> test thread do postprocessing on the list to ensure that it got the
> notifications it expected and that there were no duplicates.
>
> Either way is fine, in order to avoid the threading issues.
>
> Thanks, and sorry to belabor this review.

NP. I don't want to get imperfect code to the repo either. Thanks for 
taking your time.

Updated webrev  - 
http://cr.openjdk.java.net/~jbachorik/8005472/webrev.10

-JB-

>
> s'marks
>
>
>
> On 5/9/13 2:25 AM, Jaroslav Bachorik wrote:
>> Hi Stuart,
>>
>> On St 8. květen 2013, 01:50:22 CEST, Stuart Marks wrote:
>>> Hi Jaroslav,
>>>
>>> Great to see this shell test get rewritten!
>>>
>>> Looks like you're avoiding multiple JVM processes as well, by loading
>>> the different versions of the classes into different classloaders. It
>>> looks like a bit of trouble, but probably less than the amount of
>>> trouble caused by the shell script.
>>>
>>> I have a couple minor points.
>>>
>>> The timeout value of one second seems quite low. Under normal
>>> operation, spawning a couple threads and should proceed very quickly.
>>> However, our testing environment is quite hostile, and things that
>>> seem like they ought to proceed quickly often take considerably longer
>>> than one might think. Since you're counting notifications, and in
>>> normal operation they all come in, we don't wait for the actual
>>> timeout unless there's a failure. So it might make sense to raise the
>>> timeout to 10 or perhaps 30 seconds.
>>
>> I've adjusted the timeout for 30 seconds. Hopefully, it will be enough.
>>
>>>
>>> On the other hand, I have a question about whether counting the number
>>> of notifications is correct. Would it be possible for there to be a
>>> bug where an extra notification is sent? If so, this might mean that
>>> the test would exit prematurely, indicating success?
>>
>> It would be a severe error in the notification system implementation.
>> However, I've added a check for duplicated notifications (each
>> notification carries its sequence number) which makes the test fail in
>> case of duplication. But I don't expect it to happen.
>>
>> The updated webrev is at
>> http://cr.openjdk.java.net/~jbachorik/8005472/webrev.09
>>
>>
>> -JB-
>>
>>>
>>> s'marks
>>>
>>> On 5/6/13 2:04 AM, Jaroslav Bachorik wrote:
>>>> On Pá 3. květen 2013, 16:16:53 CEST, Daniel Fuchs wrote:
>>>>> Hi Jaroslav,
>>>>>
>>>>> In Client.java - you could consider replacing the AtomicLong
>>>>> with a CountDownLatch.
>>>>>
>>>>> This would allow you to remove the various Thread.sleep() in the
>>>>> code (in particular the one at the end).
>>>>>
>>>>> You could use CountDownLatch.await(long timeout, TimeUnit unit) to
>>>>> avoid waiting for ever in case of bugs, and the advantage is that
>>>>> the test would be able to exit as soon as the count down latch
>>>>> reaches 0, without having to wait for an arbitrary timeout.
>>>>
>>>> Great, thanks for the pointer! I've changed the test to use the
>>>> CountDownLatch.
>>>>
>>>> http://cr.openjdk.java.net/~jbachorik/8005472/webrev.08/
>>>>
>>>> -JB-
>>>>
>>>>>
>>>>> Very nice to see a shell test go away :-)
>>>>>
>>>>> -- daniel
>>>>>
>>>>>
>>>>> On 5/3/13 3:41 PM, Jaroslav Bachorik wrote:
>>>>>> Please re-review the updated webrev
>>>>>> http://cr.openjdk.java.net/~jbachorik/8005472/webrev.06
>>>>>>
>>>>>> I've replaced the shell script with the plain java test. The
>>>>>> javac API
>>>>>> is used to compile the the auxiliary classes as was recommended.
>>>>>> This
>>>>>> allowed to simplify the test.
>>>>>>
>>>>>> The test does not check for a certain string in the standard output
>>>>>> anymore - it turns out that it is possible to count the number of
>>>>>> all
>>>>>> the received JMX notifications (even though some notifications
>>>>>> can be
>>>>>> lost, we receive a special notification with the number of the lost
>>>>>> regular notifications). It is then possible to match the actual
>>>>>> number
>>>>>> of processed notifications (received + lost) against the expected
>>>>>> number
>>>>>> - different numbers mean that the notification processing thread had
>>>>>> been interrupted unexpectedly.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -JB-
>>>>>>
>>>>>> On 8.2.2013 17:37, Chris Hegarty wrote:
>>>>>>>
>>>>>>>> Jon Gibbons suggested invoking the compiler API directly from java
>>>>>>>> instead of writing a shell script. Doing this seems fairly simple,
>>>>>>>> and I
>>>>>>>> think it would be advantageous to keep things entirely in Java. I
>>>>>>>> may
>>>>>>>> attempt to rewrite the defaultSVID test using the compiler API.
>>>>>>>
>>>>>>> Here's a test that does just that.
>>>>>>>
>>>>>>> http://hg.openjdk.java.net/jdk8/tl/jdk/file/2de8c6c2d652/test/sun/misc/JarIndex/metaInfFilenames/Basic.java
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> -Chris.
>>>>>>
>>>>>
>>>>
>>>>
>>