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

[David Holmes]

Good to go.

Thanks,
David

On 25/10/2013 12:10 AM, Jaroslav Bachorik wrote:
> 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-
>>>>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>