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

[David Holmes]

On 29/06/2012 12:10 PM, Eric Wang wrote:
> 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();

Oops I missed the sleep out of my suggestion too.

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

As I said previously this isn't trying to test the GC. Broken weakrefs 
should be detected by weakref tests.

David
-----

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