Patch review request - Test bug 7123972 test/java/lang/annotation/loaderLeak/Main.java fails intermittently
Eric Wang
yiming.wang at oracle.com
Fri Jun 29 08:36:24 UTC 2012
Hi Stuart & David，
Attachment is the new changes to make code simply by following David
suggestion, Can you help please review again, Thanks a lot!
Regards,
Eric
On 2012/6/28 12:40, Stuart Marks wrote:
> I've posted the webrev here:
>
> http://cr.openjdk.java.net/~smarks/yiming.wang/7123972/webrev.0/
>
> I've looked at the changes and they seem fine.
>
> It's interesting that the run times take 30-60 sec in your experience.
> I've run them on my system (Linux in a virtual machine) and they
> usually run in 10-20 sec. In any case, as you say, if the test times
> out it indicates there really was a failure.
>
> I'll give people a chance to look at the webrev and if there aren't
> any more comments in another day or so I'll push in the changeset.
>
> Thanks for developing this!
>
> s'marks
>
> On 6/26/12 11:50 PM, Eric Wang wrote:
>> 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-29 16:03:38.666958301 +0800
+++ new/test/ProblemList.txt 2012-06-29 16:03:37.955871329 +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-29 16:03:41.487870022 +0800
+++ new/test/java/lang/annotation/loaderLeak/Main.java 2012-06-29 16:03:40.780873652 +0800
@@ -57,8 +57,13 @@
System.gc();
System.gc();
loader = null;
- System.gc();
- System.gc();
+ while(true) {
+ System.gc();
+ Thread.sleep(20);
+ if(c.get() == null) {
+ break;
+ }
+ }
if (c.get() != null) throw new AssertionError();
}
}
More information about the core-libs-dev
mailing list