From e4a0cddc68a5f1eda817b8d9e0ebacb73449d7f1 Mon Sep 17 00:00:00 2001 From: Liran Binyamin Date: Wed, 3 Apr 2024 16:00:09 -0400 Subject: Reapply "New API to clean up all inline mocks after test" Bug: 140773999 Test: Presubmit Change-Id: Ia4ed03c211369b086d05134669f8e75287307f15 --- README.version | 2 +- src/main/java/org/mockito/Mockito.java | 12 ++++- src/main/java/org/mockito/MockitoFramework.java | 51 ++++++++++++++++++ .../bytebuddy/InlineByteBuddyMockMaker.java | 13 ++++- .../framework/DefaultMockitoFramework.java | 23 +++++++++ .../java/org/mockito/plugins/InlineMockMaker.java | 53 +++++++++++++++++++ .../bytebuddy/InlineByteBuddyMockMakerTest.java | 37 ++++++++++++- .../framework/DefaultMockitoFrameworkTest.java | 60 +++++++++++++++++++++- .../CyclicMockMethodArgumentMemoryLeakTest.java | 40 +++++++++++++++ .../bugs/SelfSpyReferenceMemoryLeakTest.java | 36 +++++++++++++ 10 files changed, 322 insertions(+), 5 deletions(-) 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/README.version b/README.version index fc67c47..a738946 100644 --- a/README.version +++ b/README.version @@ -9,4 +9,4 @@ Dexmaker module. The source can be updated using the update_source.sh script. Local Modifications: - None + New API to clean up all inline mocks after test (8bdfbf053ab6e4fc14a3eaecb613f5838fdf0f09) diff --git a/src/main/java/org/mockito/Mockito.java b/src/main/java/org/mockito/Mockito.java index 2d06e58..4058340 100644 --- a/src/main/java/org/mockito/Mockito.java +++ b/src/main/java/org/mockito/Mockito.java @@ -103,7 +103,8 @@ import org.mockito.verification.VerificationWithTimeout; * 43. New API for integrations: MockitoSession is usable by testing frameworks (Since 2.15.+)
* 44. Deprecated org.mockito.plugins.InstantiatorProvider as it was leaking internal API. it was replaced by org.mockito.plugins.InstantiatorProvider2 (Since 2.15.4)
* 45. New JUnit Jupiter (JUnit5+) extension
- * 46. New Mockito.lenient() and MockSettings.lenient() methods (Since 2.20.0
+ * 46. New Mockito.lenient() and MockSettings.lenient() methods (Since 2.20.0)
+ * 47. New API for clearing mock state in inline mocking (Since 2.25.0)
* * *

0. Migrating to Mockito 2

@@ -1532,6 +1533,15 @@ import org.mockito.verification.VerificationWithTimeout; * * For more information refer to {@link Mockito#lenient()}. * Let us know how do you find the new feature by opening a GitHub issue to discuss! + * + *

47. New API for clearing mock state in inline mocking (Since 2.25.0)

+ * + * In certain specific, rare scenarios (issue #1619) + * inline mocking causes memory leaks. + * There is no clean way to mitigate this problem completely. + * Hence, we introduced a new API to explicitly clear mock state (only make sense in inline mocking!). + * See example usage in {@link MockitoFramework#clearInlineMocks()}. + * If you have feedback or a better idea how to solve the problem please reach out. */ @SuppressWarnings("unchecked") public class Mockito extends ArgumentMatchers { diff --git a/src/main/java/org/mockito/MockitoFramework.java b/src/main/java/org/mockito/MockitoFramework.java index 5ffe272..58cd4b6 100644 --- a/src/main/java/org/mockito/MockitoFramework.java +++ b/src/main/java/org/mockito/MockitoFramework.java @@ -92,4 +92,55 @@ public interface MockitoFramework { */ @Incubating InvocationFactory getInvocationFactory(); + + /** + * Clears up internal state of all inline mocks. + * This method is only meaningful if inline mock maker is in use. + * Otherwise this method is a no-op and need not be used. + *

+ * This method is useful to tackle subtle memory leaks that are possible due to the nature of inline mocking + * (issue #1619). + * If you are facing those problems, call this method at the end of the test (or in "@After" method). + * See examples of using "clearInlineMocks" in Mockito test code. + * To find out why inline mock maker keeps track of the mock objects see {@link org.mockito.plugins.InlineMockMaker}. + *

+ * Mockito's "inline mocking" enables mocking final types, enums and final methods + * (read more in section 39 of {@link Mockito} javadoc). + * This method is only meaningful when {@link org.mockito.plugins.InlineMockMaker} is in use. + * If you're using a different {@link org.mockito.plugins.MockMaker} then this method is a no-op. + * + * 


+     * public class ExampleTest {
+     *
+     *     @After
+     *     public void clearMocks() {
+     *         Mockito.framework().clearInlineMocks();
+     *     }
+     *
+     *     @Test
+     *     public void someTest() {
+     *         ...
+     *     }
+     * }
+     *
+ * + * If you have feedback or a better idea how to solve the problem please reach out. + * + * @since 2.25.0 + * @see #clearInlineMock(Object) + */ + @Incubating + void clearInlineMocks(); + + /** + * Clears up internal state of specific inline mock. + * This method is a single-mock variant of {@link #clearInlineMocks()}. + * Please read javadoc for {@link #clearInlineMocks()}. + * + * @param mock to clear up + * @since 2.25.0 + * @see #clearInlineMocks() + */ + @Incubating + void clearInlineMock(Object mock); } 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 42f10ce..dfe2061 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.Platform; 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; @@ -87,7 +88,7 @@ import static org.mockito.internal.util.StringUtil.join; * support this feature. */ @Incubating -public class InlineByteBuddyMockMaker implements ClassCreatingMockMaker { +public class InlineByteBuddyMockMaker implements ClassCreatingMockMaker, InlineMockMaker { private static final Instrumentation INSTRUMENTATION; @@ -275,6 +276,16 @@ public class InlineByteBuddyMockMaker implements ClassCreatingMockMaker { } } + @Override + public void clearMock(Object mock) { + mocks.remove(mock); + } + + @Override + public void clearAllMocks() { + mocks.clear(); + } + @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 69a733c..d92fc28 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.invocation.DefaultInvocationFactory; 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,25 @@ public class DefaultMockitoFramework implements MockitoFramework { public InvocationFactory getInvocationFactory() { return new DefaultInvocationFactory(); } + + private InlineMockMaker getInlineMockMaker() { + MockMaker mockMaker = Plugins.getMockMaker(); + return (mockMaker instanceof InlineMockMaker) ? (InlineMockMaker) mockMaker : null; + } + + @Override + public void clearInlineMocks() { + InlineMockMaker mockMaker = getInlineMockMaker(); + if (mockMaker != null) { + mockMaker.clearAllMocks(); + } + } + + @Override + public void clearInlineMock(Object mock) { + InlineMockMaker mockMaker = getInlineMockMaker(); + if (mockMaker != null) { + mockMaker.clearMock(mock); + } + } } 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 0000000..ddc3ff6 --- /dev/null +++ b/src/main/java/org/mockito/plugins/InlineMockMaker.java @@ -0,0 +1,53 @@ +/* + * 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; +import org.mockito.MockitoFramework; + +/** + * Extension to {@link MockMaker} for mock makers that changes inline method implementations + * and need keep track of created mock objects. + *

+ * Mockito's default inline mock maker keeps track of created mock objects via weak reference map. + * This poses a risk of memory leaks in certain scenarios + * (issue #1619). + * There is no clean way to tackle those problems at the moment. + * Hence, {@code InlineMockMaker} interface exposes methods to explicitly clear mock references. + * Those methods are called by {@link MockitoFramework#clearInlineMocks()}. + * When the user encounters a leak, he can mitigate the problem with {@link MockitoFramework#clearInlineMocks()}. + *

+ * {@code InlineMockMaker} is for expert users and framework integrators, when custom inline mock maker is in use. + * If you have a custom {@link MockMaker} that keeps track of mock objects, + * please have your mock maker implement {@code InlineMockMaker} interface. + * This way, it can participate in {@link MockitoFramework#clearInlineMocks()} API. + * + * @since 2.25.0 + */ +@Incubating +public interface InlineMockMaker extends MockMaker { + + /** + * Clean up internal state for specified {@code mock}. You may assume there won't be any interaction to the specific + * mock after this is called. + * + * @param mock the mock instance whose internal state is to be cleaned. + * @since 2.25.0 + */ + @Incubating + void clearMock(Object mock); + + /** + * Cleans up internal state for all existing mocks. You may assume there won't be any interaction to mocks created + * previously after this is called. + * + * @since 2.25.0 + */ + @Incubating + void clearAllMocks(); + +} + 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 51c321d..3613b5f 100644 --- a/src/test/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMakerTest.java +++ b/src/test/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMakerTest.java @@ -20,7 +20,11 @@ import org.mockito.mock.MockCreationSettings; import org.mockito.mock.SerializableMode; import org.mockito.plugins.MockMaker; -import java.util.*; +import java.util.HashMap; +import java.util.List; +import java.util.Observable; +import java.util.Observer; +import java.util.Set; import java.util.regex.Pattern; import static net.bytebuddy.ClassFileVersion.JAVA_V8; @@ -287,6 +291,37 @@ public class InlineByteBuddyMockMakerTest extends AbstractByteBuddyMockMakerTest .getOnly().getParameters().getOnly().getName()).isEqualTo("bar"); } + @Test + public void test_clear_mock_clears_handler() { + MockCreationSettings settings = settingsFor(GenericSubClass.class); + GenericSubClass proxy = mockMaker.createMock(settings, new MockHandlerImpl(settings)); + assertThat(mockMaker.getHandler(proxy)).isNotNull(); + + //when + mockMaker.clearMock(proxy); + + //then + assertThat(mockMaker.getHandler(proxy)).isNull(); + } + + @Test + public void test_clear_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(); + + //when + mockMaker.clearAllMocks(); + + //then + 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 0937b69..ae21488 100644 --- a/src/test/java/org/mockito/internal/framework/DefaultMockitoFrameworkTest.java +++ b/src/test/java/org/mockito/internal/framework/DefaultMockitoFrameworkTest.java @@ -10,15 +10,26 @@ import org.mockito.ArgumentMatchers; 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.mockito.Mockito.*; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assume.assumeTrue; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockingDetails; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.withSettings; import static org.mockitoutil.ThrowableAssert.assertThat; public class DefaultMockitoFrameworkTest extends TestBase { @@ -112,5 +123,52 @@ public class DefaultMockitoFrameworkTest extends TestBase { "For more information, see the javadoc for RedundantListenerException class."); } + @Test + public void clearing_all_mocks_is_safe_regardless_of_mock_maker_type() { + List mock = mock(List.class); + + //expect + assertTrue(mockingDetails(mock).isMock()); + framework.clearInlineMocks(); + } + + @Test + public void clears_all_mocks() { + //clearing mocks only works with inline mocking + assumeTrue(Plugins.getMockMaker() instanceof InlineMockMaker); + + //given + List list1 = mock(List.class); + assertTrue(mockingDetails(list1).isMock()); + List list2 = mock(List.class); + assertTrue(mockingDetails(list2).isMock()); + + //when + framework.clearInlineMocks(); + + //then + assertFalse(mockingDetails(list1).isMock()); + assertFalse(mockingDetails(list2).isMock()); + } + + @Test + public void clears_mock() { + //clearing mocks only works with inline mocking + assumeTrue(Plugins.getMockMaker() instanceof InlineMockMaker); + + //given + List list1 = mock(List.class); + assertTrue(mockingDetails(list1).isMock()); + List list2 = mock(List.class); + assertTrue(mockingDetails(list2).isMock()); + + //when + framework.clearInlineMock(list1); + + //then + assertFalse(mockingDetails(list1).isMock()); + assertTrue(mockingDetails(list2).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 0000000..936bd01 --- /dev/null +++ b/subprojects/inline/src/test/java/org/mockitoinline/bugs/CyclicMockMethodArgumentMemoryLeakTest.java @@ -0,0 +1,40 @@ +/* + * 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 static org.mockito.Mockito.framework; +import static org.mockito.Mockito.mock; + +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().clearInlineMocks(); + } + } + + 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 0000000..0ef8e71 --- /dev/null +++ b/subprojects/inline/src/test/java/org/mockitoinline/bugs/SelfSpyReferenceMemoryLeakTest.java @@ -0,0 +1,36 @@ +/* + * 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 static org.mockito.Mockito.framework; +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().clearInlineMocks(); + } + } + + 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; + } + } +} + -- cgit v1.2.3