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

[Stuart Marks]

Webrev now posted at

http://cr.openjdk.java.net/~smarks/yiming.wang/6948101/webrev.2/

The changes look good to me. If there's no further discussion, I'll push them 
in a couple days (U.S. holiday coming up).

s'marks

On 7/3/12 12:20 AM, Stuart Marks wrote:
> 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
>>>>
>>>>
>>
>>