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

[David Holmes]

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