[ping][ping] Re: jmx-dev RFR: 8004926 sun/management/jmxremote/bootstrap/CustomLauncherTest.sh oftenly times out

Wed Oct 9 04:31:57 PDT 2013

On 9.10.2013 12:23, David Holmes wrote:
> Jaroslav,
>
> Thanks for the details description of changes - much appreciated.
>
> There is a lot to digest in there. :)

Yep, it started as a simple fix :/

>
> It isn't obvious to me why these tests require a full JDK?

IDK, LocalManagementTest.sh was listed as one requiring full JDK. Its 
requirements are the same as the ones of CustomLauncherTest.sh (now 
*.java) so it seemed logical to list it there too.

>
> I don't quite follow the libjvm lookup logic - I would expect that you
> would always want to test the libjvm that is currently running - though
> it is hard to determine that.

I'm afraid I can't be of much assistance here - I just took what was in 
the *.sh version and converted it to *.java.

-JB-

>
> Thanks,
> David
>
> On 8/10/2013 9:33 PM, Jaroslav Bachorik wrote:
>> On 8.10.2013 05:42, David Holmes wrote:
>>> Jaroslav,
>>>
>>> Can you summarise the changes please? With the conversion to Java and
>>> the infrastructure additions I can't tell what is actually fixing the
>>> original timeout issue :)
>>
>> The timeout was most caused by using the same file for communication
>> between java processes in more test cases. When those test cases were
>> run in parallel the file got rewritten silently and some of the tests
>> could end up trying to connect to incorrect port in the target
>> application. I was able to reproduce the timeout by interleaving the
>> test runs for CustomLauncherTest.sh and LocalManagementTest.sh and
>> adding an artificial delay to CusteomLauncherTest.sh to allow
>> LocalManagementTest.sh to change the port in the file.
>>
>> While it could be fixed by using a different file for each test case I
>> took the liberty of converting the shell tests to java tests. This
>> allows me to remove the communication file and, in the end, make the
>> tests more robust.
>>
>> CustomLauncherTest.java and LocalManagementTest.java are the tests
>> converted from shell to java. I decided to convert
>> LocalManagementTest.sh as well because it has the same problems as the
>> CustomLauncherTest.sh.
>>
>> The changes in the testlibrary are about introducing new methods
>> allowing the tests easily start a process and wait for a certain text
>> appearing in its stdout/stderr. Using these methods the caller can wait
>> till the callee is fully initialized and eg. ready to accept connections.
>>
>> The changes in launchers make the launchers actually executable + I am
>> adding a linux-amd64 launcher (I needed that one to work on the changes
>> locally and thought it might be nice to have one more platform covered
>> by the test).
>>
>> I've update the webrev to include changes to LocalManagementTest and
>> TEST.groups (both of those tests require JDK) -
>> http://cr.openjdk.java.net/~jbachorik/8004926/webrev.05
>>
>> -JB-
>>
>>>
>>> Thanks,
>>> David
>>>
>>> On 8/10/2013 12:14 AM, Jaroslav Bachorik wrote:
>>>> On 19.9.2013 16:33, Jaroslav Bachorik wrote:
>>>>> The updated webrev:
>>>>> http://cr.openjdk.java.net/~jbachorik/8004926/webrev.03
>>>>>
>>>>> I've moved some of the functionality to the testlibrary.
>>>>>
>>>>> -JB -
>>>>>
>>>>> On 12.9.2013 17:31, Jaroslav Bachorik wrote:
>>>>>> On 09/12/2013 05:13 PM, Dmitry Samersoff wrote:
>>>>>>> Jaroslav,
>>>>>>>
>>>>>>> CustomLauncherTest.java:
>>>>>>>
>>>>>>> 102: this check could be moved to switch at ll. 108
>>>>>>> otherwise test fails on "sunos" and "linux" because PLATFORM remains
>>>>>>> unset.
>>>>>> Good idea. Thanks.
>>>>>>
>>>>>>> 129: I would prefer don't have pattern like this one ever in shell
>>>>>>> script. Could you prepare a list of VM's to check and just loop over
>>>>>>> it?
>>>>>>> It makes test better readable. Also I think nowdays we can always
>>>>>>> use
>>>>>>> server VM.
>>>>>> I tried to mirror the original shell test as closely as possible. It
>>>>>> would be nice if we could rely on the "server" vm only. Definitely
>>>>>> more
>>>>>> readable.
>>>>>>
>>>>>> -JB-
>>>>>>
>>>>>>> -Dmitry
>>>>>>>
>>>>>>>
>>>>>>> On 2013-09-12 18:17, Jaroslav Bachorik wrote:
>>>>>>>> On 09/12/2013 10:22 AM, Jaroslav Bachorik wrote:
>>>>>>>>> On 09/12/2013 10:12 AM, Chris Hegarty wrote:
>>>>>>>>>> On 09/12/2013 04:45 AM, David Holmes wrote:
>>>>>>>>>>> Hi Jaroslav,
>>>>>>>>>>>
>>>>>>>>>>> You need a copyright notice in the new file.
>>>>>>>>>>>
>>>>>>>>>>> As written this test can only run on a full JDK - so please add
>>>>>>>>>>> it to
>>>>>>>>>>> the :needs_jdk group in TEST.groups. (Does jcmd really needs to
>>>>>>>>>>> come
>>>>>>>>>>> from the test-jdk? And use the VMOPTS passed to the test?)
>>>>>>>>>>>
>>>>>>>>>>> Is there a reason this test can't run on OSX? I know it would
>>>>>>>>>>> need
>>>>>>>>>>> further modification but was wondering if there is something
>>>>>>>>>>> inherent in
>>>>>>>>>>> the test that makes it inapplicable to OSX.
>>>>>>>>>>>
>>>>>>>>>>> I think the test would be a lot simpler if the jdk tests had the
>>>>>>>>>>> hotspot
>>>>>>>>>>> test library's process tools available. :(
>>>>>>>>>> We have some, is there an obvious gap?
>>>>>>>>>>
>>>>>>>>>> http://hg.openjdk.java.net/jdk8/tl/jdk/file/e407df8093dc/test/lib/testlibrary/jdk/testlibrary/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> Hm, thanks for the info. I should have used this library instead.
>>>>>>>>>
>>>>>>>>> Please, stand by for the updated webrev.
>>>>>>>> I was able to get rid off the JCMD. Using the testlibrary the
>>>>>>>> target
>>>>>>>> application can recognize its own PID and print it to its stdout.
>>>>>>>> The
>>>>>>>> main application then just reads the stdout to parse the PID. No
>>>>>>>> need
>>>>>>>> for JCMD any more.
>>>>>>>>
>>>>>>>> I could not find a way to remove the dependency on "test.jdk"
>>>>>>>> system
>>>>>>>> property. According to the jtreg web documentation
>>>>>>>> (http://openjdk.java.net/jtreg/vmoptions.html#cmdLineOpts) a
>>>>>>>> "test.java"
>>>>>>>> system property should be available but in fact is not. But it
>>>>>>>> seems
>>>>>>>> that the testlibrary uses "test.jdk" system property too.
>>>>>>>>
>>>>>>>> The test does not run on OSX because nobody built the launcher
>>>>>>>> binary :)
>>>>>>>> I think it is a kind of DIY so I took the liberty of adding a
>>>>>>>> linux-amd64 launcher while working on the test.
>>>>>>>>
>>>>>>>> While working with the test library I realized I was missing a
>>>>>>>> crucial
>>>>>>>> feature (at least for my purposes) - waiting for a certain
>>>>>>>> message to
>>>>>>>> appear in the stdout/stderr of the launched process. Very often I
>>>>>>>> need
>>>>>>>> to wait for the target process to get to certain point before the
>>>>>>>> test
>>>>>>>> can be allowed to continue - and the point is indicated by a
>>>>>>>> message in
>>>>>>>> stdout/stderr. Currently all the proc tools are designed to work in
>>>>>>>> "batch" mode - the whole stdout/stderr is captured in strings and
>>>>>>>> analyzed after the target process died - and are not suitable for
>>>>>>>> this
>>>>>>>> kind of usage.
>>>>>>>>
>>>>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8004926/webrev.01
>>>>>>>>
>>>>>>>>> -JB-
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> -Chris.
>>>>>>>>>>
>>>>>>>>>>> David
>>>>>>>>>>> -----
>>>>>>>>>>>
>>>>>>>>>>> On 12/09/2013 1:39 AM, Jaroslav Bachorik wrote:
>>>>>>>>>>>> Please, review the patch for an intermittently failing test.
>>>>>>>>>>>>
>>>>>>>>>>>> The test is a shell test, using files for the interprocess
>>>>>>>>>>>> synchronization. This leads to intermittent failures.
>>>>>>>>>>>>
>>>>>>>>>>>> In order to fix this the test is rewritten in Java - the
>>>>>>>>>>>> original
>>>>>>>>>>>> functionality and outputs should be 100% preserved. The
>>>>>>>>>>>> patch is
>>>>>>>>>>>> unfortunately a bit difficult to follow since there is no
>>>>>>>>>>>> similarity
>>>>>>>>>>>> between the *.sh and *.java file so one needs to go through the
>>>>>>>>>>>> new
>>>>>>>>>>>> source in whole.
>>>>>>>>>>>>
>>>>>>>>>>>> The changes in "launcher" files are all about adding
>>>>>>>>>>>> permissions to
>>>>>>>>>>>> execute (0755) and as such the webrev shows no differences.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> Issue  : JDK-8004926
>>>>>>>>>>>> Webrev :
>>>>>>>>>>>> http://cr.openjdk.java.net/~jbachorik/8004926/webrev.00
>>>>>>>>>>>>
>>>>>>>>>>>> -JB-
>>>>>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>