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