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

[Eric Wang]

Hi Stuart,

Thank you, The fix looks simpler than the previous one, it shoud be 
better to add a Thread.sleep(20) in the loop to avoid multiple calls on 
System.gc();
Here is a small question if it is possible that after GC, the strong ref 
remains but somehow weakref is broken，so the loop will terminate and 
the test will fake pass.

while(!finalized) {
     System.gc();
     Thread.sleep(20);
     if(ref.get() == null) { //strong ref remains but somehow weakref broken
         break;
     }
}

if (ref.get() != null) { //if weakref broken, test will fake pass
     throw new Error("TEST FAILED: impl not garbage collected");
}

Thanks,
Eric
On 2012/6/29 5:54, David Holmes wrote:
> Hi Stuart,
>
> I don't think finalization or reference queues are needed in this 
> case. We could simply do:
>
>   while(true) {
>      System.gc();
>      if (weakRef.get() == null)
>        break;
>   }
>
> We have to remember that this is not testing the correct operation of 
> weak references, nor of finalization, but simply that no unintended 
> strong references remain to the object referenced via the WeakReference.
>
> So if a strong ref remains, or somehow weakrefs are broken, then the 
> loop will not terminate on its own and we rely on the test harness 
> timing it out.
>
> David
> -----
>
> On 29/06/2012 7:20 AM, Stuart Marks wrote:
>> And here's the webrev for this one:
>>
>> http://cr.openjdk.java.net/~smarks/yiming.wang/6948101/webrev.0/
>>
>> Also looks good to me. It's pretty similar to the other fix (7123972).
>> If there are no further comments I'll push this in the next day or so.
>>
>> * * *
>>
>> Some further comments, mainly aimed at the likes of David Holmes. I'm
>> mostly "thinking out loud" at the moment.
>>
>> While I'm glad to see the reliability of these tests improved and to see
>> them come off the problem list, it would be nice to see something more
>> general that could be shared among other tests, and possibly more
>> abstract so that things can be tuned to different VM characteristics if
>> necessary.
>>
>> Basically what these tests want to do is to make sure that a certain
>> object gets garbage collected. I think this is pretty common. There are
>> several dozen tests in the jdk repo that have the word "leak" in them. I
>> bet they all use different techniques to detect leaks. :-(
>>
>> The technique used here is to get a weak reference to the object, and
>> then use the object's finalizer to set a bit telling us when
>> finalization has occurred. At this point we can assert that the weak ref
>> has been cleared. In addition to being somewhat roundabout, this also
>> seems like we're merely cross-checking the garbage collector. We could
>> easily poll for the weak ref being cleared, and then assert that the bit
>> set by finalization has indeed been set.
>>
>> It seems to me we could avoid finalization entirely and just use weak
>> refs and reference queues. Would it have the same effect and semantics
>> as the current test if we were to do the following?
>>
>> - with a target object, make a weak reference registered with a ref 
>> queue
>> - clear all references to the target object
>> - request gc
>> - request finalization (is this necessary?)
>> - wait or poll on the ref queue
>>
>> If we get our weak ref from the queue, success. If we time out, fail.
>> Note that there is a timeout mechanism ReferenceQueue.remove(timeout),
>> so we could set some timeout without waiting the full jtreg timeout if
>> we don't want to. Or we could call System.gc() in a loop using remove()
>> with a short timeout (instead of sleep), similar to the current test,
>> although it's not clear to me this is necessary.
>>
>> s'marks
>>
>>
>> On 6/27/12 1:20 AM, Eric Wang wrote:
>>> Hi David,
>>>
>>> Thank you for review!
>>>
>>> Hi Stuart,
>>>
>>> Can you please help to review and post the webrev, Thanks a lot!
>>>
>>> Eric
>>>
>>> On 2012/6/27 15:32, David Holmes wrote:
>>>> On 27/06/2012 4:57 PM, Eric Wang wrote:
>>>>> Hi David & Stuart,
>>>>>
>>>>> Thank you for the help! please review the in webrev 6948101.zip in
>>>>> attachment.
>>>>
>>>> Thanks Eric. That fix also seems fine to me.
>>>>
>>>> David
>>>>
>>>>
>>>>> Regards,
>>>>> Eric
>>>>>
>>>>> On 2012/6/27 9:14, Stuart Marks wrote:
>>>>>> Hi Eric,
>>>>>>
>>>>>> Alan Bateman asked me to help you out with this since he's going 
>>>>>> to be
>>>>>> unavailable for a couple weeks.
>>>>>>
>>>>>> I didn't see you on the OpenJDK census [1] so you might not have an
>>>>>> Author role and thus the ability to post webrevs. If indeed you 
>>>>>> don't,
>>>>>> I can post a webrev for you. I can also post a webrev for your other
>>>>>> review (7123972) if it's still necessary.
>>>>>>
>>>>>> Finally, I can push the fixes for you when the reviews are complete.
>>>>>>
>>>>>> s'marks
>>>>>>
>>>>>> [1] http://openjdk.java.net/census
>>>>>>
>>>>>> On 6/26/12 2:56 PM, David Holmes wrote:
>>>>>>> Hi Eric,
>>>>>>>
>>>>>>> On 26/06/2012 7:26 PM, Eric Wang wrote:
>>>>>>>> Please help to review the fix attached for test bug 6948101
>>>>>>>> <http://monaco.us.oracle.com/detail.jsf?cr=6948101> which is same
>>>>>>>> root
>>>>>>>> cause as bug 7123972
>>>>>>>> <http://monaco.us.oracle.com/detail.jsf?cr=7123972>.
>>>>>>>> The test makes wrong assumption that GC is started immediately to
>>>>>>>> recycle unused objects after System.gc() called.
>>>>>>>> The proposed fix is to make sure objects have been recycled by GC
>>>>>>>> before
>>>>>>>> checking if the weak reference is null.
>>>>>>>
>>>>>>> Again I really need to see a webrev to see the fix in context. 
>>>>>>> Do you
>>>>>>> have
>>>>>>> Author role and an OpenJDK user name so you can post webrevs on
>>>>>>> cr.openjdk.java.net?
>>>>>>>
>>>>>>> I suspect this may have the same issues as the other fix and 
>>>>>>> require
>>>>>>> a timeout
>>>>>>> for when the original problem is still present.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Eric
>>>>>
>>>