code review request: Test case for JDK-7042126 HashMap.clone() memory leak

[Mike Duigou]

Looks good to me. Longer term I do think we should consider importing the utility that Martin suggests rather than doing something similar ad hoc as we have done in this test.

Mike

On Jan 31 2013, at 10:39 , David Buck wrote:

> Hi Mike (and others)!
> 
> Sounds like we have a (sort of) consensus. I have updated the test case and retested. Please have a look and let me know what you think.
> 
> http://cr.openjdk.java.net/~dbuck/7042126/webrev.02/
> 
> Cheers,
> -Buck
> 
> On 2013/02/01 3:03, Mike Duigou wrote:
>> I like this suggestion. David's most recent webrev with while(true) loop could easily be extended to create linked list of reference arrays in each loop. ie.
>> 
>> Object[] chain = null;
>> while (wr.get() != null) {
>>     try {
>>         Object[] allocate = new Object[1000000];
>> 	allocate[0] = chain;
>> 	chain = allocate;
>>     } catch( OutOfMemoryError ) {
>> 	chain = null;
>>     }
>> 
>>     System.gc();
>>     Thread.sleep(100);
>> }
>> 
>> Mike
>> 
>> On Jan 31 2013, at 05:47 , Peter Levart wrote:
>> 
>>> Hi David,
>>> 
>>> You could combine the B and C in the following pattern:
>>> 
>>> - Create a WeakReference to the observed object;
>>> - Execute the potentially "leaky" operation;
>>> - Trigger gc with after check of WeakReference. Maybe a couple of times.
>>> - If that does not clear the reference, fill-in heap until OOME is thrown and after that check the reference.
>>> 
>>> Regards, Peter
>>> 
>>> On 01/31/2013 10:40 AM, David Buck wrote:
>>>> Hi!
>>>> 
>>>> I was curious to see what others have done in the past and took a look at about 15 different testcases for memory leaks in the jdk tree and basically found 3 patterns:
>>>> 
>>>> Pattern A: Use a loop that would exercise the relevant code for a finite number of runs. If an OOME is not thrown, the test passes. In order for the test to finish quickly, most people seem to specify a very small Xmx to limit the number of iterations needed. (example: java/util/concurrent/BlockingQueue/PollMemoryLeak.java)
>>>> 
>>>> Pattern B: Code similar to what I have submitted for this review that tries to leak one object, explicitly triggers a GC event, and then confirms that the WeakReference was nulled out. One thing to note is that in every case I found of this the author chose to run multiple GC events, usually somewhere between 5 to 10000 iterations. (example: javax/management/Introspector/ClassLeakTest.java)
>>>> 
>>>> Pattern C: These cases used a WeakReference like Pattern B above, but instead of depending on an explicit call to System.gc(), would intentionally fill the heap until an OOME is thrown to trigger at least 1 "full" gc event. (example: java/rmi/server/UnicastRemoteObject/unexportObject/UnexportLeak.java)
>>>> 
>>>> Patterns A and B were most common (with pattern B being slightly more common), and I only found Pattern C 2 times
>>>> 
>>>> Strictly speaking, all 3 patters are problematic as they each depend on undefined behavior in some way or another. Patterns A and C require the JVM to handle OOMEs somewhat gracefully (Pattern A when we fail, Pattern C when we pass), and pattern B requires an explicit call to System.gc to collect an arbitrary object.
>>>> 
>>>> The fact that we do not routinely see false positives from these cases indicates that in practice, they all seem to work OK for the time being.
>>>> 
>>>> After thinking about this for a bit, I actually really like pattern C. Even though the JVM is not technically guaranteed to gracefully handle all OOME conditions, we try very hard to be as robust as possible in the face of Java-heap OOME. Again, the fact that we do not routinely see false positives from these tests proves that in practice, this works very well. Even if an otherwise "correct" change (say in the JVM) resulted in Pattern C suddenly failing, it is something we would almost certainly want to investigate as it would indicate that the robustness of the JVM has somehow regressed.
>>>> 
>>>> I plan to modify my testcase to follow pattern C and resubmit for review. If anyone has any other ideas or comments, I would be grateful for your input.
>>>> 
>>>> Cheers,
>>>> -Buck
>>>> 
>>>> On 2013/01/30 22:32, David Buck wrote:
>>>>> Hi Alan!
>>>>> 
>>>>> Thank you for your help.
>>>>> 
>>>>> TL;DR version:
>>>>> 
>>>>> Strictly speaking (going only by the specifications), this could give us
>>>>> false positives, but I believe this is VERY unlikely to actually happen
>>>>> in real life.
>>>>> 
>>>>> Long version:
>>>>> 
>>>>> Yes, I gave this some thought myself. For example, on JRockit, if the
>>>>> object were in old space and System.gc() only did a young collection
>>>>> (the default behavior for JRockit), this test would result in a false
>>>>> positive. In fact, as the JVM is allowed by the specification to
>>>>> completely ignore explicit GC calls, we could never guarantee that we
>>>>> would the WeakReference would always get nulled out.
>>>>> 
>>>>> That said, in pactice this works very well for both HotSpot and JRockit.
>>>>> Every scenario I have tried it out on (with both JVMs) has provided the
>>>>> expected result every single time (i.e. failing when expected; never
>>>>> resulting in false positive otherwise). It seems that both of Oracle's
>>>>> JVMs as currently implemented are very unlikely to run into any issues
>>>>> here. Marking the test cases as "othervm" also helps to remove most edge
>>>>> case scenarios where you could still somehow imagine this failing. (For
>>>>> example, on a JRockit-like JVM, other tests running concurrently could
>>>>> trigger a gc in the middle of this test resulting in the HashMap and its
>>>>> contents being promoted to old space and the null reference not being
>>>>> cleared during the call to System.gc() as expected.)
>>>>> 
>>>>> One option would be to mark this as a manually-run test if we wanted to
>>>>> be extra cautious. What do you think?
>>>>> 
>>>>>> Minor nit, should be WeakReference<Object>  to avoid the raw type.
>>>>> 
>>>>> I will update the webrev once we have decided what (if anything) to do
>>>>> regarding the risk of false positives.
>>>>> 
>>>>> Cheers,
>>>>> -Buck
>>>>> 
>>>>> On 2013/01/30 22:09, Alan Bateman wrote:
>>>>>> On 29/01/2013 23:36, David Buck wrote:
>>>>>>> Hi!
>>>>>>> 
>>>>>>> This is a review request to add only the test case for the following
>>>>>>> OracleJDK issue :
>>>>>>> 
>>>>>>> [ 7042126 : (alt-rt) HashMap.clone implementation should be
>>>>>>> re-examined ]
>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7042126
>>>>>>> 
>>>>>>> * please note: I just marked the bug as "public" this morning, so
>>>>>>> there will be a short delay before the above link is available.
>>>>>>> 
>>>>>>> The issue (root cause) is not in OpenJDK (i.e. the problem was
>>>>>>> OracleJDK specific), but the test case is valid for any Java SE
>>>>>>> implementation so it should go into OpenJDK so we can prevent a
>>>>>>> similar issue from ever happening in both releases moving forward. The
>>>>>>> test case simply helps ensure that the contents of a HashMap are not
>>>>>>> leaked when we clone the HashMap.
>>>>>>> 
>>>>>>> webrev:
>>>>>>> [ Code Review for jdk ]
>>>>>>> http://cr.openjdk.java.net/~dbuck/7042126/webrev.00/
>>>>>> How robust is this test? I'm concerned that it might fail intermittently
>>>>>> if the System.gc doesn't immediately GC the now un-references entries in
>>>>>> hm.
>>>>>> 
>>>>>> Minor nit, should be WeakReference<Object>  to avoid the raw type.
>>>>>> 
>>>>>> -Alan.
>>> 
>>