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

[Peter Levart]

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.