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

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;

More information about the core-libs-dev mailing list