Patch review request - Test bug 7123972 test/java/lang/annotation/loaderLeak/Main.java fails intermittently
Eric Wang
yiming.wang at oracle.com
Tue Jul 3 10:37:49 UTC 2012
Hi Stuart,
I have made updates which is same as 6948101, please help to review,
Thanks a lot!
Regards,
Eric
On 2012/6/30 6:01, Stuart Marks wrote:
> I've posted the updated webrev here:
>
> http://cr.openjdk.java.net/~smarks/yiming.wang/7123972/webrev.1/
>
> This code is pretty much the same as 6948101 and the same comments I
> had on that bug apply here as well, so please make similar updates to
> this one.
>
> Thanks.
>
> s'marks
>
> On 6/29/12 1:36 AM, Eric Wang wrote:
>> 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-07-03 18:25:07.707507490 +0800
+++ new/test/ProblemList.txt 2012-07-03 18:25:06.901427388 +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-07-03 18:25:10.784495232 +0800
+++ new/test/java/lang/annotation/loaderLeak/Main.java 2012-07-03 18:25:09.996408765 +0800
@@ -57,9 +57,17 @@
System.gc();
System.gc();
loader = null;
- System.gc();
- System.gc();
- if (c.get() != null) throw new AssertionError();
+
+ // Might require multiple calls to System.gc() for weak-references
+ // processing to be complete. If the weak-reference is not cleared as
+ // expected we will hang here until timed out by the test harness.
+ while (true) {
+ System.gc();
+ Thread.sleep(20);
+ if (c.get() == null) {
+ break;
+ }
+ }
}
}
More information about the core-libs-dev
mailing list