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

[Stuart Marks]

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
>>>>>>
>>>>>>
>>>>
>>>>