[PATCH] Review Request - Test bug: 6948101 java/rmi/transport/pinLastArguments/PinLastArguments.java failing intermittently

[Stuart Marks]

This approach looks good to me. Please go ahead and update 7123972 as well, and 
I'll post these webrevs tomorrow (my time).

Thanks.

s'marks

On 7/2/12 11:41 PM, Eric Wang wrote:
> David,
>
> I have added the comments before the loop, please help to review. If it is OK,
> I'll update the 7123972 as well.
>
> Thanks,
> Eric
>
> On 2012/7/3 12:22, David Holmes wrote:
>> Hi Eric,
>>
>> On 3/07/2012 1:28 PM, Eric Wang wrote:
>>> Hi Stuart and David,
>>>
>>> Thanks for the suggestion about the timeout. so there are three ways to
>>> handle the timeout.
>>>
>>> 1. Add comment to explain that test failed due to default timeout
>>> 2. Specify the the timeout option
>>> 3. Timeout programly
>>>
>>> Is it ok for you that i'd like to choose the second way "explict
>>> timeout" as the test looks simply and lucid?
>>
>> If by #2 you mean add something like:
>>
>> @run main/timeout=10
>>
>> then no. Earlier this year there were changes to a bunch of tests that set
>> timeouts shorter than the jtreg default, to delete the timeouts. Given that,
>> my misgivings about relying on the harness are not relevant so we need only
>> do #1 and add a comment to make it clear that a failing test will be
>> indicated via the harness timeout.
>>
>> David
>> -----
>>
>>> Regards,
>>> Eric
>>>
>>> On 2012/7/3 10:38, David Holmes wrote:
>>>> On 3/07/2012 7:08 AM, Stuart Marks wrote:
>>>>> On 7/1/12 5:39 PM, David Holmes wrote:
>>>>>>> Now, how can the test fail? If ref is never cleared, the while loop
>>>>>>> will
>>>>>>> never terminate, and we rely on jtreg to timeout and kill this
>>>>>>> test. It
>>>>>>> would be good to have a brief comment above the while loop that
>>>>>>> explains
>>>>>>> this.
>>>>>>
>>>>>> Agreed. Something like
>>>>>>
>>>>>> // Might require multiple calls to System.gc() for weak-references
>>>>>> processing
>>>>>> to be complete.
>>>>>> // If the weak-reference is not cleared as expected we will hang here
>>>>>> // until timed out by the test harness
>>>>>>
>>>>>> Though now I write that I'm unhappy that this will only behave nicely
>>>>>> if run
>>>>>> from jtreg. We do use explicit timeouts in other tests.
>>>>>
>>>>> So, are you unhappy enough that we should ask Eric to add an explicit
>>>>> timeout?
>>>>
>>>> No. But I'm not the authoritive voice in this area :)
>>>>
>>>>> I don't think it would be that difficult. Perhaps a technique like the
>>>>> following would suffice. Get System.currentTimeMillis() before the loop,
>>>>> and call it again each time through the loop, and fail if the difference
>>>>> is greater than the timeout (e.g., 60 sec). Doesn't involve other
>>>>> threads or even Timer/TimerTask.
>>>>
>>>> Or simply limit the number of loops so that N*sleepTime = timeout.
>>>> This then requires adding back the check for null outside the loop.
>>>>
>>>>> On the other hand if one is running this test directly instead of
>>>>> through jtreg, one can just ^C it if it's taking an unexpectedly long
>>>>> time.
>>>>
>>>> True.
>>>>
>>>> David
>>>> -----
>>>>
>>>>
>>>>> s'marks
>>>
>>>
>
>