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

Eric Wang yiming.wang at oracle.com
Wed Jun 27 06:50:32 UTC 2012


Hi David,

Thank you! I run the test several times on 3 platforms (windows, solaris 
and linux), the average execution time is 30secs to 60secs. if the test 
hang 2 minutes, there should be something wrong.

Hi Marks,

I don't have the author role, Can you please help to review and post the 
webrev 7123972.zip in attachment if it is OK, Thanks a lot!

Regards,
Eric

On 2012/6/27 14:19, David Holmes wrote:
> Eric,
>
> Ignore my comment about adding the timeouts. As Alan reminded me if 
> the test would hang then jtreg will time it out after two minutes anyway.
>
> So this is good to go as far as I am concerned.
>
> Thanks,
> David
>
> On 27/06/2012 7:51 AM, David Holmes wrote:
>> 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

-------------- next part --------------
--- old/test/ProblemList.txt	2012-06-25 15:41:20.466150117 +0800
+++ new/test/ProblemList.txt	2012-06-25 15:41:18.998075349 +0800
@@ -122,9 +122,6 @@
 
 # jdk_lang
 
-# 7123972
-java/lang/annotation/loaderLeak/Main.java			generic-all
-
 # 6944188
 java/lang/management/ThreadMXBean/ThreadStateTest.java          generic-all
 
-------------- next part --------------
--- old/test/java/lang/annotation/loaderLeak/Main.java	2012-06-25 15:41:26.005179716 +0800
+++ new/test/java/lang/annotation/loaderLeak/Main.java	2012-06-25 15:41:24.531076496 +0800
@@ -36,6 +36,8 @@
 import java.io.*;
 
 public class Main {
+    static volatile boolean finalized = false;
+    
     public static void main(String[] args) throws Exception {
         for (int i=0; i<100; i++)
             doTest(args.length != 0);
@@ -57,8 +59,11 @@
         System.gc();
         System.gc();
         loader = null;
-        System.gc();
-        System.gc();
+        while(!finalized) {
+            System.gc();
+            System.runFinalization();
+            Thread.sleep(20);
+        }
         if (c.get() != null) throw new AssertionError();
     }
 }
@@ -67,6 +72,7 @@
     private Hashtable classes = new Hashtable();
 
     public SimpleClassLoader() {
+        Main.finalized = false;
     }
     private byte getClassImplFromDataBase(String className)[] {
         byte result[];
@@ -124,4 +130,8 @@
         classes.put(className, result);
         return result;
     }
+    
+    protected void finalize() {
+        Main.finalized = true;
+    }
 }


More information about the core-libs-dev mailing list