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

[Stuart Marks]

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