Patch review request - Test bug 7123972 test/java/lang/annotation/loaderLeak/Main.java fails intermittently

[David Holmes]

Thanks Eric.

So to summarize:

- we create a custom classloader, load a class through it and store a 
reference to that Class in a WeakReference
- we then drop the reference to the loader

At this point the only reference to the loader is from the Class loaded, 
which in turn is only weakly reachable.

I must confess that I'm unclear why this test should be failing in its 
original form. The first gc() should remove the strong reference to the 
loader. That leaves a weak cycle: WeakRef -> Class -> Loader -> Class. 
The second gc() should detect the cycle and clear the WeakRef. I guess 
the problem is that depending on which gc is in use the actual gc() 
calls may not in fact induce every possible GC action.

The fix seems reasonable in that regard - keep gc'ing and finalizing 
till we see the loader is gone, and so the WeakReference should be 
cleared. The original bug would cause a ref to the Class to remain (from 
the annotation) and hence the WeakRef would not be cleared. However, in 
that case the loader would also be strongly referenced and so never 
become finalizable. So if this test was now run against a JDK with the 
original bug, the test would hang. So I think we need to add a timeout 
in there as well.

David

On 25/06/2012 6:06 PM, Eric Wang wrote:
> On 2012/6/21 20:16, David Holmes wrote:
>> Hi Eric,
>>
>> On 21/06/2012 8:57 PM, Eric Wang wrote:
>>> Hi David,
>>>
>>> Thanks for your review, I have updated the code by following your
>>> suggestion. please see the attachment.
>>> I'm not sure whether there's a better way to guarantee object finalized
>>> by GC definitely within the given time. The proposed fix may work in
>>> most cases but may still throw InterruptException if execution is
>>> timeout (2 minutes of JTreg by default).
>>
>> There is no way to guarantee finalization in a specific timeframe, but
>> if a simple test hasn't executed finalizers in 2 minutes then that in
>> itself indicates a problem.
>>
>> Can you post a full webrev for this patch? I don't like seeing it out
>> of context and am wondering exactly what we are trying to GC here.
>>
>> David
>>
>>> Regards,
>>> Eric
>>>
>>> On 2012/6/21 14:32, David Holmes wrote:
>>>> Hi Eric,
>>>>
>>>> On 21/06/2012 4:05 PM, Eric Wang wrote:
>>>>> I come from Java SQE team who are interested in regression test bug
>>>>> fix.
>>>>> Here is the first simple fix for bug 7123972
>>>>> <http://monaco.us.oracle.com/detail.jsf?cr=7123972>, Can you please
>>>>> help
>>>>> to review and comment? Attachment is the patch Thanks!
>>>>>
>>>>> This bug is caused by wrong assumption that the GC is started
>>>>> immediately to recycle un-referenced objects after System.gc() called
>>>>> one or two times.
>>>>>
>>>>> The proposed solution is to make sure the un-referenced object is
>>>>> recycled by GC before checking if the reference is null.
>>>>
>>>> Your patch makes its own assumptions - specifically that finalization
>>>> must eventually run. At a minimum you should add
>>>> System.runFinalization() calls after the System.gc() inside the loop.
>>>> Even that is no guarantee in a general sense, though it should work
>>>> for hotspot.
>>>>
>>>> David
>>>>
>>>>
>>>>> Regards,
>>>>> Eric
>>>
> Hi Alan & David,
>
> Thank you for your comments, sorry for reply this mail late as i was
> just back from the dragon boat holiday.
> I have updated the code again based on your suggestions: rename the flag
> variable, increase the sleep time and remove it from problem list.
> The attachment is the full webrev for this patch, Can you please review
> again? Thanks a lot!
>
> Regards,
> Eric