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

[Eric Wang]

Opps, It is my carelessness, I will be more careful in the next bug fixes.
Thank you to review and change it.

Regards,
Eric

On 2012/7/4 8:58, Stuart Marks wrote:
> Sure, I can take care of that.
>
> If this is the only change, no need to generate another webrev, Eric.
>
> s'marks
>
>
> On 7/3/12 4:53 PM, David Holmes wrote:
>> Please fix the missing space in
>>
>> if(ref.get()
>>
>> before pushing.
>>
>> Thanks,
>> David
>>
>> On 4/07/2012 7:04 AM, Stuart Marks wrote:
>>> 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
>>>>>>>
>>>>>>>
>>>>>
>>>>>