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

[Stuart Marks]

Pushed:

http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4ad204cc7433

s'marks

On 7/3/12 2:04 PM, Stuart Marks wrote:
> Webrev posted here:
>
> http://cr.openjdk.java.net/~smarks/yiming.wang/7123972/webrev.2/
>
> The changes look good. If there's no further discussion, I'll push them in a
> couple days.
>
> s'marks
>
> On 7/3/12 3:37 AM, Eric Wang wrote:
>> 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
>>>>>>
>>>>
>>
>>