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

[Eric Wang]

Hi Marks,

Thanks for your great help to review and push my first fix.:-)

Regards,
Eric
On 2012/7/6 6:19, Stuart Marks wrote:
> Pushed:
>
> http://hg.openjdk.java.net/jdk8/tl/jdk/rev/97eb7a4b1fdd
>
> s'marks
>
> On 7/3/12 6:54 PM, Eric Wang wrote:
>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>
>>