Repository: mockito/mockito
Issue: InlineByteBuddyMockMaker does not clean up stale mocks

**Repro:**

1. Mockito 2.15.2 using mockito-inline
2. With -Xmx64m run
```
@RunWith(JUnit4.class)
public class Stress {
    private static final int NUM_RUNS = 20000;

    public static class TestClass {
        public String returnA() {
            return "A";
        }
    }

    /**
     * Test that mocks that are not referenced anymore are properly cleaned up.
     */
    @Test
    public void callALotOfMocks() {
        for (int i = 0; i < NUM_RUNS; i++) {
            TestClass mock = mock(TestClass.class);
            when(mock.returnA()).thenReturn("B");

            assertEquals("B", mock.returnA());

            if (i % 1024 == 0) {
                System.out.println("Mocked " + i + "/" + NUM_RUNS);
            }
        }
    }
}
```

**Expected:**

The test eventually finishes

**Observed:**

> Mocked 0/20000
> Mocked 1024/20000
> Mocked 2048/20000
> Mocked 3072/20000
> Mocked 4096/20000
> Mocked 5120/20000
> Mocked 6144/20000
> Mocked 7168/20000
> Mocked 8192/20000
> Mocked 9216/20000
> Mocked 10240/20000
> Mocked 11264/20000
> Exception in thread "main" java.lang.OutOfMemoryError: GC overhead limit exceeded
> 	at java.lang.StringCoding$StringDecoder.decode(StringCoding.java:149)
> 	at java.lang.StringCoding.decode(StringCoding.java:193)
> 	at java.lang.String.<init>(String.java:426)
> 	at java.util.jar.Attributes.read(Attributes.java:418)
> 	at java.util.jar.Manifest.read(Manifest.java:199)
> 
> Exception: java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in thread "main"
> 
> Process finished with exit code 1

**Analysis:**

The `InlineByteBuddyMockMaker` uses a Map<WeakRef\<Mock\>, Handler> in the `mocks` field to store the mocks and their handlers. It relies that the mocks get garbage collected to remove the entry in the map. Unfortunately if there is an invocation on the mock, the handlers keep strong references to the mock in `InterceptedInvocation.mock`, `RealMethodCall.instance`, and `SerializableRealMethodCall.instance`. These are (strong) references by the handler. Hence the mock never gets cleaned up and the map grows indefinitely.

This is not a problem if the mocks get `Mockito.reset` properly or if using a mock maker that does not store the handlers.

Issue comments:
Fantastic analysis, thank you! Do you want to take a stab at fixing it?

(ping @raphw, original author of the this mock maker).
I will have a look. Great analysis indeed. We will have to weakly reference any mock instance stored in Mockito. This is a straight forwars fi, I hope.
I will do it once I have time. (The kid, you k ow.)
I saw two problems we need to keep in mind:
- How to weak references interact with serializability
- We have to be vigilant to prevent premature cleaning up of the mock. Is there any possible case where we can gain access to the the invocations / handler without a reference to the mock?
I uploaded a fix proposal as fb37a2e even though
- I don't like that we use different kind of invocations for some tests
- The tests pass and I think I serialize properly, but I am not 100% sure as I never dealt with serialization before
Hey guys! I was reviewing @moltmann's PR and I'm wondering what are the alternative options to solve the problem. Can we attach a state to the mock object so that MockMakers can do their magic without keeping weak maps of all mocks? @raphw, thoughts?
Unfortunately, I do not think that there is an alternative to weak references.

The Java instrumentation API allows the retransformation of Java byte code, it does not allow changing the shape of a class, e.g. adding fields. Naturally, the dispatcher would be placed in such a field such that the life time of a dispatcher would be implicitly bound to the mock instance. But since we cannot add this field, we have to place the instance outside of it such that we have to use weak references to reach the same effect.

Therefore, I fear that this is the only way to go. 
Thank you for explanation!

One alternative/supplemental option is to make the lifecycle of mocks more formal. For example, JUnit rule / runner / mockito session can inform the mock maker that "we're done with mock x".
@mockitoguy If you can make this work, great. I see my mockmaker to be used to a legacy test suites though as they want to just mock one final method and not create a new test suite. Some older suites might not use junit or any other fancy feature.

Hence I would like to find a solution that does not require any changes outside of mockito.
@mockitoguy Is there any further change you want to c0ad83a?

Pull request review comments:

Patch that resolved the issue:
diff --git a/gradle/java-library.gradle b/gradle/java-library.gradle
index d41e3406b8..45c72f11d2 100644
--- a/gradle/java-library.gradle
+++ b/gradle/java-library.gradle
@@ -9,6 +9,7 @@ targetCompatibility = 1.6
 
 test {
     include "**/*Test.class"
+
     testLogging {
         exceptionFormat 'full'
         showCauses true
diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodAdvice.java b/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodAdvice.java
index 02882a2274..f39a1a2462 100644
--- a/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodAdvice.java
+++ b/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodAdvice.java
@@ -16,6 +16,8 @@
 import org.mockito.internal.exceptions.stacktrace.ConditionalStackTraceFilter;
 import org.mockito.internal.invocation.RealMethod;
 import org.mockito.internal.invocation.SerializableMethod;
+import org.mockito.internal.invocation.mockref.MockReference;
+import org.mockito.internal.invocation.mockref.MockWeakReference;
 import org.mockito.internal.util.concurrent.WeakConcurrentMap;
 
 import java.io.IOException;
@@ -141,14 +143,14 @@ private static class RealMethodCall implements RealMethod {
 
         private final Method origin;
 
-        private final Object instance;
+        private final MockWeakReference<Object> instanceRef;
 
         private final Object[] arguments;
 
         private RealMethodCall(SelfCallInfo selfCallInfo, Method origin, Object instance, Object[] arguments) {
             this.selfCallInfo = selfCallInfo;
             this.origin = origin;
-            this.instance = instance;
+            this.instanceRef = new MockWeakReference<Object>(instance);
             this.arguments = arguments;
         }
 
@@ -162,8 +164,8 @@ public Object invoke() throws Throwable {
             if (!Modifier.isPublic(origin.getDeclaringClass().getModifiers() & origin.getModifiers())) {
                 origin.setAccessible(true);
             }
-            selfCallInfo.set(instance);
-            return tryInvoke(origin, instance, arguments);
+            selfCallInfo.set(instanceRef.get());
+            return tryInvoke(origin, instanceRef.get(), arguments);
         }
 
     }
@@ -174,14 +176,14 @@ private static class SerializableRealMethodCall implements RealMethod {
 
         private final SerializableMethod origin;
 
-        private final Object instance;
+        private final MockReference<Object> instanceRef;
 
         private final Object[] arguments;
 
         private SerializableRealMethodCall(String identifier, Method origin, Object instance, Object[] arguments) {
             this.origin = new SerializableMethod(origin);
             this.identifier = identifier;
-            this.instance = instance;
+            this.instanceRef = new MockWeakReference<Object>(instance);
             this.arguments = arguments;
         }
 
@@ -196,13 +198,13 @@ public Object invoke() throws Throwable {
             if (!Modifier.isPublic(method.getDeclaringClass().getModifiers() & method.getModifiers())) {
                 method.setAccessible(true);
             }
-            MockMethodDispatcher mockMethodDispatcher = MockMethodDispatcher.get(identifier, instance);
+            MockMethodDispatcher mockMethodDispatcher = MockMethodDispatcher.get(identifier, instanceRef.get());
             if (!(mockMethodDispatcher instanceof MockMethodAdvice)) {
                 throw new MockitoException("Unexpected dispatcher for advice-based super call");
             }
-            Object previous = ((MockMethodAdvice) mockMethodDispatcher).selfCallInfo.replace(instance);
+            Object previous = ((MockMethodAdvice) mockMethodDispatcher).selfCallInfo.replace(instanceRef.get());
             try {
-                return tryInvoke(method, instance, arguments);
+                return tryInvoke(method, instanceRef.get(), arguments);
             } finally {
                 ((MockMethodAdvice) mockMethodDispatcher).selfCallInfo.set(previous);
             }
diff --git a/src/main/java/org/mockito/internal/invocation/DefaultInvocationFactory.java b/src/main/java/org/mockito/internal/invocation/DefaultInvocationFactory.java
index 875a903ecd..d08f6b1ac9 100644
--- a/src/main/java/org/mockito/internal/invocation/DefaultInvocationFactory.java
+++ b/src/main/java/org/mockito/internal/invocation/DefaultInvocationFactory.java
@@ -5,6 +5,7 @@
 package org.mockito.internal.invocation;
 
 import org.mockito.internal.creation.DelegatingMethod;
+import org.mockito.internal.invocation.mockref.MockWeakReference;
 import org.mockito.internal.debugging.LocationImpl;
 import org.mockito.internal.progress.SequenceNumber;
 import org.mockito.invocation.Invocation;
@@ -33,7 +34,7 @@ private Invocation createInvocation(Object target, MockCreationSettings settings
 
     public static InterceptedInvocation createInvocation(Object mock, Method invokedMethod, Object[] arguments, RealMethod realMethod, MockCreationSettings settings, Location location) {
         return new InterceptedInvocation(
-            mock,
+            new MockWeakReference<Object>(mock),
             createMockitoMethod(invokedMethod, settings),
             arguments,
             realMethod,
diff --git a/src/main/java/org/mockito/internal/invocation/InterceptedInvocation.java b/src/main/java/org/mockito/internal/invocation/InterceptedInvocation.java
index d3a9b976cb..b9cf0721f4 100644
--- a/src/main/java/org/mockito/internal/invocation/InterceptedInvocation.java
+++ b/src/main/java/org/mockito/internal/invocation/InterceptedInvocation.java
@@ -4,6 +4,7 @@
  */
 package org.mockito.internal.invocation;
 
+import org.mockito.internal.invocation.mockref.MockReference;
 import org.mockito.internal.exceptions.VerificationAwareInvocation;
 import org.mockito.internal.reporting.PrintSettings;
 import org.mockito.invocation.Invocation;
@@ -19,7 +20,7 @@ public class InterceptedInvocation implements Invocation, VerificationAwareInvoc
 
     private static final long serialVersionUID = 475027563923510472L;
 
-    private final Object mock;
+    private final MockReference<Object> mockRef;
     private final MockitoMethod mockitoMethod;
     private final Object[] arguments, rawArguments;
     private final RealMethod realMethod;
@@ -32,13 +33,13 @@ public class InterceptedInvocation implements Invocation, VerificationAwareInvoc
     private boolean isIgnoredForVerification;
     private StubInfo stubInfo;
 
-    public InterceptedInvocation(Object mock,
+    public InterceptedInvocation(MockReference<Object> mockRef,
                                  MockitoMethod mockitoMethod,
                                  Object[] arguments,
                                  RealMethod realMethod,
                                  Location location,
                                  int sequenceNumber) {
-        this.mock = mock;
+        this.mockRef = mockRef;
         this.mockitoMethod = mockitoMethod;
         this.arguments = ArgumentsProcessor.expandArgs(mockitoMethod, arguments);
         this.rawArguments = arguments;
@@ -99,7 +100,7 @@ public void ignoreForVerification() {
 
     @Override
     public Object getMock() {
-        return mock;
+        return mockRef.get();
     }
 
     @Override
@@ -138,7 +139,7 @@ public boolean equals(Object o) {
             return false;
         }
         InterceptedInvocation other = (InterceptedInvocation) o;
-        return this.mock.equals(other.mock)
+        return this.mockRef.get().equals(other.mockRef.get())
                 && this.mockitoMethod.equals(other.mockitoMethod)
                 && this.equalArguments(other.arguments);
     }
diff --git a/src/main/java/org/mockito/internal/invocation/mockref/MockReference.java b/src/main/java/org/mockito/internal/invocation/mockref/MockReference.java
new file mode 100644
index 0000000000..0dac2ea919
--- /dev/null
+++ b/src/main/java/org/mockito/internal/invocation/mockref/MockReference.java
@@ -0,0 +1,16 @@
+/*
+ * Copyright (c) 2018 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+
+package org.mockito.internal.invocation.mockref;
+
+import java.io.Serializable;
+
+/**
+ * To avoid memory leaks for certain implementations of MockMaker,
+ * we need to use weak mock references internally. See #1313
+ */
+public interface MockReference<T> extends Serializable {
+    T get();
+}
diff --git a/src/main/java/org/mockito/internal/invocation/mockref/MockStrongReference.java b/src/main/java/org/mockito/internal/invocation/mockref/MockStrongReference.java
new file mode 100644
index 0000000000..3b20ee58fd
--- /dev/null
+++ b/src/main/java/org/mockito/internal/invocation/mockref/MockStrongReference.java
@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) 2018 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+
+package org.mockito.internal.invocation.mockref;
+
+import java.io.ObjectStreamException;
+
+public class MockStrongReference<T> implements MockReference<T> {
+
+    private final T ref;
+    private final boolean deserializeAsWeakRef;
+
+    public MockStrongReference(T ref, boolean deserializeAsWeakRef) {
+        this.ref = ref;
+        this.deserializeAsWeakRef = deserializeAsWeakRef;
+    }
+
+    @Override
+    public T get() {
+        return ref;
+    }
+
+    private Object readResolve() throws ObjectStreamException {
+        if (deserializeAsWeakRef) {
+            return new MockWeakReference<T>(ref);
+        } else {
+            return this;
+        }
+    }
+}
diff --git a/src/main/java/org/mockito/internal/invocation/mockref/MockWeakReference.java b/src/main/java/org/mockito/internal/invocation/mockref/MockWeakReference.java
new file mode 100644
index 0000000000..256745b362
--- /dev/null
+++ b/src/main/java/org/mockito/internal/invocation/mockref/MockWeakReference.java
@@ -0,0 +1,44 @@
+/*
+ * Copyright (c) 2018 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+
+package org.mockito.internal.invocation.mockref;
+
+import java.io.ObjectStreamException;
+import java.lang.ref.WeakReference;
+
+/**
+ * A weak reference that is converted into a strong reference when serialized.
+ * See {@link MockReference}.
+ */
+public class MockWeakReference<T> implements MockReference<T> {
+
+    private final WeakReference<T> ref;
+
+    public MockWeakReference(T t) {
+        this.ref = new WeakReference<T>(t);
+    }
+
+    private Object writeReplace() throws ObjectStreamException {
+        return new MockStrongReference<T>(get(), true);
+    }
+
+    @Override
+    public T get() {
+        T ref = this.ref.get();
+
+        if (ref == null) {
+            throw new IllegalStateException("The mock object was garbage collected. " +
+                "This should not happen in normal circumstances when using public API. " +
+                "Typically, the test class keeps strong reference to the mock object " +
+                "and it prevents getting the mock collected. Mockito internally needs " +
+                "to keep weak references to mock objects to avoid memory leaks for " +
+                "certain types of MockMaker implementations. If you see this exception " +
+                "using Mockito public API, please file a bug. For more information see " +
+                "issue #1313.");
+        }
+
+        return ref;
+    }
+}
diff --git a/subprojects/inline/inline.gradle b/subprojects/inline/inline.gradle
index be103615af..b8d2577ab5 100644
--- a/subprojects/inline/inline.gradle
+++ b/subprojects/inline/inline.gradle
@@ -8,3 +8,6 @@ dependencies {
 }
 
 tasks.javadoc.enabled = false
+
+//required by the "StressTest.java"
+test.maxHeapSize = "256m"
diff --git a/version.properties b/version.properties
index 7591fc5a63..91e1c5b39e 100644
--- a/version.properties
+++ b/version.properties
@@ -1,5 +1,5 @@
 #Currently building Mockito version
-version=2.15.7
+version=2.16.0
 
 #Previous version used to generate release notes delta
 previousVersion=2.15.6


Test patch:
diff --git a/src/test/java/org/mockito/internal/invocation/InvocationBuilder.java b/src/test/java/org/mockito/internal/invocation/InvocationBuilder.java
index c8943b180e..b3106a6993 100644
--- a/src/test/java/org/mockito/internal/invocation/InvocationBuilder.java
+++ b/src/test/java/org/mockito/internal/invocation/InvocationBuilder.java
@@ -6,6 +6,7 @@
 package org.mockito.internal.invocation;
 
 import org.mockito.Mockito;
+import org.mockito.internal.invocation.mockref.MockStrongReference;
 import org.mockito.internal.debugging.LocationImpl;
 import org.mockito.invocation.Invocation;
 import org.mockito.invocation.Location;
@@ -60,7 +61,7 @@ public Invocation toInvocation() {
             }
         }
 
-        Invocation i = new InterceptedInvocation(mock,
+        Invocation i = new InterceptedInvocation(new MockStrongReference<Object>(mock, false),
             new SerializableMethod(method),
             args,
             NO_OP,
diff --git a/src/test/java/org/mockito/internal/invocation/mockref/MockWeakReferenceTest.java b/src/test/java/org/mockito/internal/invocation/mockref/MockWeakReferenceTest.java
new file mode 100644
index 0000000000..1e3d8e9ab2
--- /dev/null
+++ b/src/test/java/org/mockito/internal/invocation/mockref/MockWeakReferenceTest.java
@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) 2007 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+package org.mockito.internal.invocation.mockref;
+
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+import org.mockito.internal.invocation.mockref.MockWeakReference;
+import org.mockitoutil.TestBase;
+
+import static org.junit.Assert.fail;
+
+public class MockWeakReferenceTest extends TestBase {
+
+    @Test
+    public void descriptive_exception_when_mock_was_collected() {
+        try {
+            //when
+            new MockWeakReference(null).get();
+            //then
+            fail();
+        } catch (Exception e) {
+            Assertions.assertThat(e).hasMessageContaining("The mock object was garbage collected");
+        }
+    }
+}
diff --git a/src/test/java/org/mockitoutil/TestBase.java b/src/test/java/org/mockitoutil/TestBase.java
index fe920e6541..ca3d7b8744 100644
--- a/src/test/java/org/mockitoutil/TestBase.java
+++ b/src/test/java/org/mockitoutil/TestBase.java
@@ -11,6 +11,7 @@
 import org.mockito.StateMaster;
 import org.mockito.internal.MockitoCore;
 import org.mockito.internal.configuration.ConfigurationAccess;
+import org.mockito.internal.invocation.mockref.MockStrongReference;
 import org.mockito.internal.invocation.InterceptedInvocation;
 import org.mockito.internal.debugging.LocationImpl;
 import org.mockito.internal.invocation.InvocationBuilder;
@@ -64,8 +65,9 @@ protected static Invocation invocationOf(Class<?> type, String methodName, Objec
         for (int i = 0; i < args.length; i++) {
             types[i] = args[i].getClass();
         }
-        return new InterceptedInvocation(mock(type), new SerializableMethod(type.getMethod(methodName,
-                types)), args, InterceptedInvocation.NO_OP, new LocationImpl(), 1);
+        return new InterceptedInvocation(new MockStrongReference<Object>(mock(type), false),
+            new SerializableMethod(type.getMethod(methodName, types)), args, InterceptedInvocation.NO_OP,
+            new LocationImpl(), 1);
     }
 
     protected static Invocation invocationAt(String location) {
diff --git a/subprojects/inline/src/test/java/org/mockitoinline/StressTest.java b/subprojects/inline/src/test/java/org/mockitoinline/StressTest.java
new file mode 100644
index 0000000000..c6a55064d4
--- /dev/null
+++ b/subprojects/inline/src/test/java/org/mockitoinline/StressTest.java
@@ -0,0 +1,38 @@
+/*
+ * Copyright (c) 2018 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+package org.mockitoinline;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.withSettings;
+
+public class StressTest {
+    public class TestClass {
+        public String getStuff() {
+            return "A";
+        }
+    }
+
+    @Test
+    public void call_a_lot_of_mocks() {
+        //This requires smaller heap set for the test process, see "inline.gradle"
+        for (int i = 0; i < 40000; i++) {
+            TestClass mock = mock(TestClass.class);
+            when(mock.getStuff()).thenReturn("B");
+            assertEquals("B", mock.getStuff());
+
+            TestClass serializableMock = mock(TestClass.class, withSettings().serializable());
+            when(serializableMock.getStuff()).thenReturn("C");
+            assertEquals("C", serializableMock.getStuff());
+
+            if (i % 1024 == 0) {
+                System.out.println(i + "/40000 mocks called");
+            }
+        }
+    }
+}