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

[Jaroslav Bachorik]

Hi David,

On 10.10.2013 06:41, David Holmes wrote:
> On 9/10/2013 9:31 PM, Jaroslav Bachorik wrote:
>> 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.
>
> Ah! Now I see it - it uses tools.jar which implies a full JDK.
>
>>>
>>> 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.
>
> Okay. I expect this will need revisiting at some point.

So, does this mean "ok, go"?

Thanks,

-JB-

>
> Thanks,
> David
> -----
>
>
>> -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-
>>>>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>