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

[David Holmes]

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