From a2fc2d695a0136e85ede0baa0c8aaa6f1af84aef Mon Sep 17 00:00:00 2001 From: Garfield Tan Date: Mon, 11 Feb 2019 14:30:10 -0800 Subject: [PATCH] Fixes #1614: Add API to clean up mocks. Due to the introduction of map from weak reference from mock instance to its invocation handler, Mockito became vunerable to memory leaks as there are multiple situations where Mockito could unintentionally hold strong references to mock instances in the map record. The strong references could be through spiedInstance for spies, and arguments used to faciliate method stubbing. Mockito could never know if the arguments passed in for method stubbing are also strongly referenced somewhere else or not, so Mockito needs to save a strong reference to these arguments to avoid premature GC. Therefore to solve cyclic strong references through arguments Mockito needs to explicitly know when mocks are out of their life, and clean up all internal strong references associated with them. This commit also fixes #1532 and fixes #1533. --- .../java/org/mockito/MockitoFramework.java | 10 +++++ .../bytebuddy/InlineByteBuddyMockMaker.java | 12 +++++- .../framework/DefaultMockitoFramework.java | 14 +++++++ .../org/mockito/plugins/InlineMockMaker.java | 25 +++++++++++ .../InlineByteBuddyMockMakerTest.java | 25 +++++++++++ .../DefaultMockitoFrameworkTest.java | 16 ++++++++ ...yclicMockMethodArgumentMemoryLeakTest.java | 41 +++++++++++++++++++ .../bugs/SelfSpyReferenceMemoryLeakTest.java | 37 +++++++++++++++++ 8 files changed, 179 insertions(+), 1 deletion(-) create mode 100644 src/main/java/org/mockito/plugins/InlineMockMaker.java create mode 100644 subprojects/inline/src/test/java/org/mockitoinline/bugs/CyclicMockMethodArgumentMemoryLeakTest.java create mode 100644 subprojects/inline/src/test/java/org/mockitoinline/bugs/SelfSpyReferenceMemoryLeakTest.java diff --git a/src/main/java/org/mockito/MockitoFramework.java b/src/main/java/org/mockito/MockitoFramework.java index 5ffe27215b..67cb41be09 100644 --- a/src/main/java/org/mockito/MockitoFramework.java +++ b/src/main/java/org/mockito/MockitoFramework.java @@ -92,4 +92,14 @@ public interface MockitoFramework { */ @Incubating InvocationFactory getInvocationFactory(); + + /** + * Clears up all existing mocks. This is useful when testing with {@link org.mockito.plugins.InlineMockMaker} which + * may hold references to mocks and leak memory. No interaction to any mock created previously is not allowed after + * calling this method. + * + * @since 2.24.8 + */ + @Incubating + void clearAllMocks(); } diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMaker.java b/src/main/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMaker.java index 8cb651a928..dfa1146f28 100644 --- a/src/main/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMaker.java +++ b/src/main/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMaker.java @@ -14,6 +14,7 @@ import org.mockito.internal.util.concurrent.WeakConcurrentMap; import org.mockito.invocation.MockHandler; import org.mockito.mock.MockCreationSettings; +import org.mockito.plugins.InlineMockMaker; import java.io.File; import java.io.FileOutputStream; @@ -89,7 +90,7 @@ * support this feature. */ @Incubating -public class InlineByteBuddyMockMaker implements ClassCreatingMockMaker { +public class InlineByteBuddyMockMaker implements ClassCreatingMockMaker, InlineMockMaker { private static final Instrumentation INSTRUMENTATION; @@ -271,6 +272,15 @@ public void resetMock(Object mock, MockHandler newHandler, MockCreationSettings } } + @Override + public void cleanUpMock(Object mock) { + if (mock == null) { + mocks.clear(); + } else { + mocks.remove(mock); + } + } + @Override public TypeMockability isTypeMockable(final Class type) { return new TypeMockability() { diff --git a/src/main/java/org/mockito/internal/framework/DefaultMockitoFramework.java b/src/main/java/org/mockito/internal/framework/DefaultMockitoFramework.java index 69a733c077..75338aff3a 100644 --- a/src/main/java/org/mockito/internal/framework/DefaultMockitoFramework.java +++ b/src/main/java/org/mockito/internal/framework/DefaultMockitoFramework.java @@ -10,6 +10,8 @@ import org.mockito.internal.util.Checks; import org.mockito.invocation.InvocationFactory; import org.mockito.listeners.MockitoListener; +import org.mockito.plugins.InlineMockMaker; +import org.mockito.plugins.MockMaker; import org.mockito.plugins.MockitoPlugins; import static org.mockito.internal.progress.ThreadSafeMockingProgress.mockingProgress; @@ -37,4 +39,16 @@ public MockitoPlugins getPlugins() { public InvocationFactory getInvocationFactory() { return new DefaultInvocationFactory(); } + + @Override + public void clearAllMocks() { + MockMaker mockMaker = Plugins.getMockMaker(); + + if (!(mockMaker instanceof InlineMockMaker)) { + return; + } + + InlineMockMaker inlineMockMaker = (InlineMockMaker) mockMaker; + inlineMockMaker.cleanUpMock(null); + } } diff --git a/src/main/java/org/mockito/plugins/InlineMockMaker.java b/src/main/java/org/mockito/plugins/InlineMockMaker.java new file mode 100644 index 0000000000..be95556d2c --- /dev/null +++ b/src/main/java/org/mockito/plugins/InlineMockMaker.java @@ -0,0 +1,25 @@ +/* + * Copyright (c) 2019 Mockito contributors + * This program is made available under the terms of the MIT License. + */ + +package org.mockito.plugins; + +import org.mockito.Incubating; + +/** + * Extension to {@link MockMaker} for mock makers that changes inline method implementations. + * @since 2.24.8 + */ +@Incubating +public interface InlineMockMaker extends MockMaker { + /** + * Cleans up internal state for specified {@code mock}. You may assume there won't be any interaction to this {@code mock} + * after this is called. + * + * @param mock the mock instance whose internal state is to be cleaned, or {@code null} to clean all existing mocks. + * @since 2.24.8 + */ + @Incubating + void cleanUpMock(Object mock); +} diff --git a/src/test/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMakerTest.java b/src/test/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMakerTest.java index dfe3bb578a..43dfb3a473 100644 --- a/src/test/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMakerTest.java +++ b/src/test/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMakerTest.java @@ -287,6 +287,31 @@ public void test_parameters_retention() throws Exception { .getOnly().getParameters().getOnly().getName()).isEqualTo("bar"); } + @Test + public void test_cleanup_mock_clears_handler() { + MockCreationSettings settings = settingsFor(GenericSubClass.class); + GenericSubClass proxy = mockMaker.createMock(settings, new MockHandlerImpl(settings)); + assertThat(mockMaker.getHandler(proxy)).isNotNull(); + mockMaker.cleanUpMock(proxy); + assertThat(mockMaker.getHandler(proxy)).isNull(); + } + + @Test + public void test_cleanup_all_mock_clears_handler() { + MockCreationSettings settings = settingsFor(GenericSubClass.class); + GenericSubClass proxy1 = mockMaker.createMock(settings, new MockHandlerImpl(settings)); + assertThat(mockMaker.getHandler(proxy1)).isNotNull(); + + settings = settingsFor(GenericSubClass.class); + GenericSubClass proxy2 = mockMaker.createMock(settings, new MockHandlerImpl(settings)); + assertThat(mockMaker.getHandler(proxy1)).isNotNull(); + + mockMaker.cleanUpMock(null); + + assertThat(mockMaker.getHandler(proxy1)).isNull(); + assertThat(mockMaker.getHandler(proxy2)).isNull(); + } + private static MockCreationSettings settingsFor(Class type, Class... extraInterfaces) { MockSettingsImpl mockSettings = new MockSettingsImpl(); mockSettings.setTypeToMock(type); diff --git a/src/test/java/org/mockito/internal/framework/DefaultMockitoFrameworkTest.java b/src/test/java/org/mockito/internal/framework/DefaultMockitoFrameworkTest.java index 0937b69992..631b6a2cb8 100644 --- a/src/test/java/org/mockito/internal/framework/DefaultMockitoFrameworkTest.java +++ b/src/test/java/org/mockito/internal/framework/DefaultMockitoFrameworkTest.java @@ -10,14 +10,18 @@ import org.mockito.MockSettings; import org.mockito.StateMaster; import org.mockito.exceptions.misusing.RedundantListenerException; +import org.mockito.internal.configuration.plugins.Plugins; import org.mockito.listeners.MockCreationListener; import org.mockito.listeners.MockitoListener; import org.mockito.mock.MockCreationSettings; +import org.mockito.plugins.InlineMockMaker; import org.mockitoutil.TestBase; import java.util.List; import java.util.Set; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.*; import static org.mockitoutil.ThrowableAssert.assertThat; @@ -112,5 +116,17 @@ public void run() { "For more information, see the javadoc for RedundantListenerException class."); } + @Test + public void clears_all_mocks() { + List list = mock(List.class); + assertTrue(mockingDetails(list).isMock()); + + framework.clearAllMocks(); + + if (Plugins.getMockMaker() instanceof InlineMockMaker) { + assertFalse(mockingDetails(list).isMock()); + } + } + private static class MyListener implements MockitoListener {} } diff --git a/subprojects/inline/src/test/java/org/mockitoinline/bugs/CyclicMockMethodArgumentMemoryLeakTest.java b/subprojects/inline/src/test/java/org/mockitoinline/bugs/CyclicMockMethodArgumentMemoryLeakTest.java new file mode 100644 index 0000000000..194e36c5eb --- /dev/null +++ b/subprojects/inline/src/test/java/org/mockitoinline/bugs/CyclicMockMethodArgumentMemoryLeakTest.java @@ -0,0 +1,41 @@ +/* + * Copyright (c) 2019 Mockito contributors + * This program is made available under the terms of the MIT License. + */ + +package org.mockitoinline.bugs; + +import org.junit.Test; +import org.mockito.MockitoSession; + +import static org.mockito.Mockito.framework; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockitoSession; + +public class CyclicMockMethodArgumentMemoryLeakTest { + private static final int ARRAY_LENGTH = 1 << 20; // 4 MB + + @Test + public void no_memory_leak_when_cyclically_calling_method_with_mocks() { + for (int i = 0; i < 100; ++i) { + final A a = mock(A.class); + a.largeArray = new int[ARRAY_LENGTH]; + final B b = mock(B.class); + + a.accept(b); + b.accept(a); + + framework().clearAllMocks(); + } + } + + private static class A { + private int[] largeArray; + + void accept(B b) {} + } + + private static class B { + void accept(A a) {} + } +} diff --git a/subprojects/inline/src/test/java/org/mockitoinline/bugs/SelfSpyReferenceMemoryLeakTest.java b/subprojects/inline/src/test/java/org/mockitoinline/bugs/SelfSpyReferenceMemoryLeakTest.java new file mode 100644 index 0000000000..23fda66403 --- /dev/null +++ b/subprojects/inline/src/test/java/org/mockitoinline/bugs/SelfSpyReferenceMemoryLeakTest.java @@ -0,0 +1,37 @@ +/* + * Copyright (c) 2019 Mockito contributors + * This program is made available under the terms of the MIT License. + */ + +package org.mockitoinline.bugs; + +import org.junit.Test; +import org.mockito.MockitoSession; + +import static org.mockito.Mockito.framework; +import static org.mockito.Mockito.mockitoSession; +import static org.mockito.Mockito.spy; + +public class SelfSpyReferenceMemoryLeakTest { + private static final int ARRAY_LENGTH = 1 << 20; // 4 MB + + @Test + public void no_memory_leak_when_spy_holds_reference_to_self() { + for (int i = 0; i < 100; ++i) { + final DeepRefSelfClass instance = spy(new DeepRefSelfClass()); + instance.refInstance(instance); + + framework().clearAllMocks(); + } + } + + private static class DeepRefSelfClass { + private final DeepRefSelfClass[] array = new DeepRefSelfClass[1]; + + private final int[] largeArray = new int[ARRAY_LENGTH]; + + private void refInstance(DeepRefSelfClass instance) { + array[0] = instance; + } + } +}