RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid

[Yekaterina Kantserova]

Hopefully the last version :)

Erik has recommended to skip the whole timeout concept. Instead the test 
will loop until a decreasing count is found. Otherwise the test will 
timeout. The default JTreg time out is 5 minutes so it would be enough 
even for slower configurations.

The new webrev can be found here: 
http://cr.openjdk.java.net/~ykantser/6977426/webrev.02/

// Katja



On 12/17/2014 12:33 PM, Daniel Fuchs wrote:
> On 17/12/14 12:05, Yekaterina Kantserova wrote:
>> Daniel,
>>
>> It's really funny to get such a feedback!
>>
>> (1) The output from test right now looks like:
>>
>> ----------System.out:(7/183)----------
>> call count = 1000
>> instance count: 1001
>> call count = 2000
>> instance count: 1001
>> Finishing early due to non-increasing instance count
>> increasing count: 1
>> decreasing or the same count: 1
>>
>> so printing the value of 'count' is already in there: 'call count ='.
>
> I meant to print the final value for count - but OK - having the
> 'call count' printed every 1000 count should be enough :-)
>
>
>> (2) In the case we want to have an extra condition I think it should be
>> 'count > 2000'. Otherwise if the end time is passed the 'count > 1000'
>> will result in having just one iteration (we will left the loop on 1100)
>> and test will fail on assert.
>
> right. my mistake. you need at least two histograms.
>
>> The test timeout value should be increased as well. Right now it's 120 +
>> 30 sec.
>>
>>   * @run main/timeout=150 TestLoggerWeakRefLeak Logger
>>
>> If the test will not be completed in 120 s the extra 30 s will not make
>> the trick and JTreg will interrupt it. What do you think will be enough?
>
> I am not sure - but I guess what you have now should be good enough
> for starter. In the problematic configurations there often is
> an additional timeout factor - so in such case the real jtreg timeout
> would I believe be something like 150*4 anyway - which should hopefully
> leave time enough for the while loop to take two histograms. Simply
> adding the extra condition to keep looping until we have at least two
> should do the trick.
>
> best regards, and thanks again for taking care of 6977426!
>
> -- daniel
>
>>
>> // Katja
>>
>>
>>
>> On 12/17/2014 10:55 AM, Daniel Fuchs wrote:
>>> Hi Katja,
>>>
>>> Sorry for not thinking of that when I replied earlier.
>>> Your new test is so much more readable than the old shell
>>> script :-)
>>>
>>> These are minor  suggestions but they might help analysis
>>> if the test fails:
>>>
>>> On 17/12/14 10:10, Yekaterina Kantserova wrote:
>>>> Hi,
>>>>
>>>> Thanks for all reviews!
>>>>
>>>> The new webrev can be found here:
>>>> http://cr.openjdk.java.net/~ykantser/6977426/webrev.01/
>>>
>>> Not that I believe it's very likely to happen, but I wonder
>>> if the condition to exit the while loop:
>>>
>>>   79         while (now < (startTime + (LOOP_TIME_SEC * 1000))) {
>>>
>>> should also make sure that count > 1000 - and continue if not.
>>> (Thinking of passed timeout related issues when tests where run
>>>  with -Xcomp on fastdebug builds during integration nightlies)
>>>
>>> Possibly printing the value of 'count' before the assert
>>> at line 103 could help with debugging too, if the test actually
>>> fails (even though the combination of increasingCount
>>> + decreasingCount would give us a hint)
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>>>
>>>> // Katja
>>>>
>>>>
>>>>
>>>> On 12/16/2014 05:41 PM, Jaroslav Bachorik wrote:
>>>>> Hi Katja,
>>>>>
>>>>> This request should probably go to core-libs as well.
>>>>>
>>>>> Other than that I have just a few nits:
>>>>>
>>>>> test/java/util/logging/TestLoggerWeakRefLeak.java
>>>>>
>>>>> L57: if ("Logger".contains(args[0])) -> "Logger".equals(args[0]) ?
>>>>> L127: // in LogManager.loggers that *arebeing* properly managed ->
>>>>> *are being*
>>>>>
>>>>> callLoggerAndCount() and callAnonymousLoggerAndCount() are using the
>>>>> 'count' variable to always return 100. Could they be made to return a
>>>>> constant instead?
>>>>>
>>>>>
>>>>> Cheers,
>>>>>
>>>>> -JB-
>>>>>
>>>>>
>>>>> On 12/16/2014 04:57 PM, Yekaterina Kantserova wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Could I please have a review of this fix.
>>>>>>
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-6977426
>>>>>> webrev: http://cr.openjdk.java.net/~ykantser/6977426/webrev.00/
>>>>>>
>>>>>> java/util/logging/LoggerWeakRefLeak.sh and
>>>>>> java/util/logging/AnonLoggerWeakRefLeak.sh are merged and 
>>>>>> rewritten in
>>>>>> java. sun/tools/common/CommonTests.sh is removed because it doesn't
>>>>>> test
>>>>>> the product but sun/tool/common library functionality.
>>>>>>
>>>>>> Thanks,
>>>>>> Katja
>>>>>
>>>>
>>>
>>
>