From c131d98b8768a69cb571302acd93f3c92760d328 Mon Sep 17 00:00:00 2001 From: Garfield Tan Date: Mon, 11 Feb 2019 14:30:10 -0800 Subject: [PATCH] Fixes #1614: Clean up mocks when session finishes. 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. --- src/main/java/org/mockito/Mockito.java | 2 +- .../MultipleTrackingMockSessionException.java | 22 +++++ .../org/mockito/internal/MockitoCore.java | 12 ++- .../bytebuddy/ByteBuddyMockMaker.java | 6 ++ .../bytebuddy/InlineByteBuddyMockMaker.java | 5 + .../bytebuddy/SubclassByteBuddyMockMaker.java | 5 + .../mockito/internal/exceptions/Reporter.java | 7 ++ .../framework/DefaultMockitoFramework.java | 68 +++++++++++++ .../framework/DefaultMockitoSession.java | 32 +++++- .../mockito/internal/junit/MockTracker.java | 55 +++++++++++ .../listeners/MockitoListenerUtil.java | 35 +++++++ .../progress/MockingProgressImpl.java | 29 +----- .../session/DefaultMockitoSessionBuilder.java | 10 +- .../java/org/mockito/plugins/MockMaker.java | 10 ++ .../session/MockitoSessionBuilder.java | 20 ++++ .../InlineByteBuddyMockMakerTest.java | 9 ++ .../DefaultMockitoFrameworkTest.java | 99 +++++++++++++++++++ .../internal/junit/MockTrackerTest.java | 45 +++++++++ .../listeners/MockitoListenerUtilTest.java | 62 ++++++++++++ .../progress/MockingProgressImplTest.java | 41 -------- .../DefaultMockitoSessionBuilderTest.java | 70 +++++++++++++ .../stubbing/StubbingWarningsTest.java | 2 +- .../creation/AndroidByteBuddyMockMaker.java | 5 + ...yclicMockMethodArgumentMemoryLeakTest.java | 43 ++++++++ .../bugs/SelfSpyReferenceMemoryLeakTest.java | 39 ++++++++ 25 files changed, 657 insertions(+), 76 deletions(-) create mode 100644 src/main/java/org/mockito/exceptions/misusing/MultipleTrackingMockSessionException.java create mode 100644 src/main/java/org/mockito/internal/junit/MockTracker.java create mode 100644 src/main/java/org/mockito/internal/listeners/MockitoListenerUtil.java create mode 100644 src/test/java/org/mockito/internal/junit/MockTrackerTest.java create mode 100644 src/test/java/org/mockito/internal/listeners/MockitoListenerUtilTest.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/Mockito.java b/src/main/java/org/mockito/Mockito.java index e2a0ade374..6cb3cbf538 100644 --- a/src/main/java/org/mockito/Mockito.java +++ b/src/main/java/org/mockito/Mockito.java @@ -2978,7 +2978,7 @@ static MockitoDebugger debug() { @Incubating @CheckReturnValue public static MockitoFramework framework() { - return new DefaultMockitoFramework(); + return MOCKITO_CORE.framework(); } /** diff --git a/src/main/java/org/mockito/exceptions/misusing/MultipleTrackingMockSessionException.java b/src/main/java/org/mockito/exceptions/misusing/MultipleTrackingMockSessionException.java new file mode 100644 index 0000000000..cea9815501 --- /dev/null +++ b/src/main/java/org/mockito/exceptions/misusing/MultipleTrackingMockSessionException.java @@ -0,0 +1,22 @@ +/* + * Copyright (c) 2019 Mockito contributors + * This program is made available under the terms of the MIT License. + */ + +package org.mockito.exceptions.misusing; + +import org.mockito.MockitoSession; +import org.mockito.exceptions.base.MockitoException; + +/** + * Reports the misuse where user tries to open more than one {@link MockitoSession} that tracks and cleans up mocks. + * User needs to finish previous session that tracks and cleans up mocks before trying to open a new one. Note this + * doesn't prevent user from opening sessions that doesn't track or clean up mocks. + * + * @since 2.24.4 + */ +public class MultipleTrackingMockSessionException extends MockitoException { + public MultipleTrackingMockSessionException(String message) { + super(message); + } +} diff --git a/src/main/java/org/mockito/internal/MockitoCore.java b/src/main/java/org/mockito/internal/MockitoCore.java index 5d39d5e093..cae8958075 100644 --- a/src/main/java/org/mockito/internal/MockitoCore.java +++ b/src/main/java/org/mockito/internal/MockitoCore.java @@ -7,8 +7,10 @@ import org.mockito.InOrder; import org.mockito.MockSettings; import org.mockito.MockingDetails; +import org.mockito.MockitoFramework; import org.mockito.exceptions.misusing.NotAMockException; import org.mockito.internal.creation.MockSettingsImpl; +import org.mockito.internal.framework.DefaultMockitoFramework; import org.mockito.internal.invocation.finder.VerifiableInvocationsFinder; import org.mockito.internal.listeners.VerificationStartedNotifier; import org.mockito.internal.progress.MockingProgress; @@ -49,6 +51,8 @@ @SuppressWarnings("unchecked") public class MockitoCore { + private DefaultMockitoFramework framework = new DefaultMockitoFramework(); + public boolean isTypeMockable(Class typeToMock) { return typeMockabilityOf(typeToMock).mockable(); } @@ -60,7 +64,7 @@ public T mock(Class typeToMock, MockSettings settings) { MockSettingsImpl impl = MockSettingsImpl.class.cast(settings); MockCreationSettings creationSettings = impl.build(typeToMock); T mock = createMock(creationSettings); - mockingProgress().mockingStarted(mock, creationSettings); + framework.mockingStarted(mock, creationSettings); return mock; } @@ -91,7 +95,7 @@ public T verify(T mock, VerificationMode mode) { MockingProgress mockingProgress = mockingProgress(); VerificationMode actualMode = mockingProgress.maybeVerifyLazily(mode); - mockingProgress.verificationStarted(new MockAwareVerificationMode(mock, actualMode, mockingProgress.verificationListeners())); + mockingProgress.verificationStarted(new MockAwareVerificationMode(mock, actualMode, framework.verificationListeners())); return mock; } @@ -215,4 +219,8 @@ public MockingDetails mockingDetails(Object toInspect) { public LenientStubber lenient() { return new DefaultLenientStubber(); } + + public DefaultMockitoFramework framework() { + return framework; + } } diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/ByteBuddyMockMaker.java b/src/main/java/org/mockito/internal/creation/bytebuddy/ByteBuddyMockMaker.java index 3b124dcaab..15f24d8230 100644 --- a/src/main/java/org/mockito/internal/creation/bytebuddy/ByteBuddyMockMaker.java +++ b/src/main/java/org/mockito/internal/creation/bytebuddy/ByteBuddyMockMaker.java @@ -40,6 +40,12 @@ public void resetMock(Object mock, MockHandler newHandler, MockCreationSettings defaultByteBuddyMockMaker.resetMock(mock, newHandler, settings); } + @Override + @Incubating + public void cleanUpMock(Object mock) { + defaultByteBuddyMockMaker.cleanUpMock(mock); + } + @Override @Incubating public TypeMockability isTypeMockable(Class type) { 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..e67aa05267 100644 --- a/src/main/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMaker.java +++ b/src/main/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMaker.java @@ -271,6 +271,11 @@ public void resetMock(Object mock, MockHandler newHandler, MockCreationSettings } } + @Override + public void cleanUpMock(Object mock) { + mocks.remove(mock); + } + @Override public TypeMockability isTypeMockable(final Class type) { return new TypeMockability() { diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassByteBuddyMockMaker.java b/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassByteBuddyMockMaker.java index 79e89f2f36..89cc1c0912 100644 --- a/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassByteBuddyMockMaker.java +++ b/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassByteBuddyMockMaker.java @@ -140,6 +140,11 @@ public void resetMock(Object mock, MockHandler newHandler, MockCreationSettings ); } + @Override + public void cleanUpMock(Object mock) { + // We don't need to do anything. When there is no strong reference to the mock GC will just clean it up. + } + @Override public TypeMockability isTypeMockable(final Class type) { return new TypeMockability() { diff --git a/src/main/java/org/mockito/internal/exceptions/Reporter.java b/src/main/java/org/mockito/internal/exceptions/Reporter.java index 3158228f22..2a7347bb5b 100644 --- a/src/main/java/org/mockito/internal/exceptions/Reporter.java +++ b/src/main/java/org/mockito/internal/exceptions/Reporter.java @@ -913,4 +913,11 @@ public static void unfinishedMockingSession() { "Previous MockitoSession was not concluded with 'finishMocking()'.", "For examples of correct usage see javadoc for MockitoSession class.")); } + + public static void multipleTrackingMockSession() { + throw new MultipleTrackingMockSessionException(join( + "Multiple open sessions that track and clean up mocks detected.", + "There can only be at most one open session that tracks and cleans up mocks,", + "please finish previous session that tracks and clean up mocks before opening a new one.")); + } } diff --git a/src/main/java/org/mockito/internal/framework/DefaultMockitoFramework.java b/src/main/java/org/mockito/internal/framework/DefaultMockitoFramework.java index 69a733c077..a795ad3709 100644 --- a/src/main/java/org/mockito/internal/framework/DefaultMockitoFramework.java +++ b/src/main/java/org/mockito/internal/framework/DefaultMockitoFramework.java @@ -4,18 +4,28 @@ */ package org.mockito.internal.framework; +import java.util.ArrayList; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; import org.mockito.MockitoFramework; import org.mockito.internal.configuration.plugins.Plugins; import org.mockito.internal.invocation.DefaultInvocationFactory; +import org.mockito.internal.listeners.MockitoListenerUtil; import org.mockito.internal.util.Checks; import org.mockito.invocation.InvocationFactory; +import org.mockito.listeners.MockCreationListener; import org.mockito.listeners.MockitoListener; +import org.mockito.listeners.VerificationListener; +import org.mockito.mock.MockCreationSettings; import org.mockito.plugins.MockitoPlugins; import static org.mockito.internal.progress.ThreadSafeMockingProgress.mockingProgress; public class DefaultMockitoFramework implements MockitoFramework { + private final Set listeners = new LinkedHashSet(); + public MockitoFramework addListener(MockitoListener listener) { Checks.checkNotNull(listener, "listener"); mockingProgress().addListener(listener); @@ -28,6 +38,64 @@ public MockitoFramework removeListener(MockitoListener listener) { return this; } + /** + * Adds a listener to Mockito. + * + *

It's different from {@link #addListener(MockitoListener)} in the way that this listener will receive + * notifications from all threads. + * + *

It also throws {@link org.mockito.exceptions.misusing.RedundantListenerException} if caller tries to add a + * listener of the same type as an added listener. + * + *

The listener will be called in the thread where the notification is sent. It's the responsibility of listeners + * to ensure thread-safety. + * + * @param listener to add to Mockito + * @return this instance of mockito framework (fluent builder pattern) + */ + public MockitoFramework addGlobalListener(MockitoListener listener) { + Checks.checkNotNull(listener, "listener"); + synchronized (listeners) { + MockitoListenerUtil.addListener(listener, listeners); + } + return this; + } + + public MockitoFramework removeGlobalListener(MockitoListener listener) { + Checks.checkNotNull(listener, "listener"); + synchronized (listeners) { + listeners.remove(listener); + } + return this; + } + + public Set verificationListeners() { + Set verificationListeners = mockingProgress().verificationListeners(); + synchronized (listeners) { + for (MockitoListener listener : listeners) { + if (listener instanceof VerificationListener) { + verificationListeners.add((VerificationListener) listener); + } + } + } + return verificationListeners; + } + + public void mockingStarted(Object mock, MockCreationSettings settings) { + mockingProgress().mockingStarted(mock, settings); + List creationListeners = new ArrayList(); + synchronized (listeners) { + for (MockitoListener listener : listeners) { + if (listener instanceof MockCreationListener) { + creationListeners.add((MockCreationListener) listener); + } + } + } + for (MockCreationListener listener : creationListeners) { + listener.onMockCreated(mock, settings); + } + } + @Override public MockitoPlugins getPlugins() { return Plugins.getPlugins(); diff --git a/src/main/java/org/mockito/internal/framework/DefaultMockitoSession.java b/src/main/java/org/mockito/internal/framework/DefaultMockitoSession.java index 6483a1cb0d..0429824d95 100644 --- a/src/main/java/org/mockito/internal/framework/DefaultMockitoSession.java +++ b/src/main/java/org/mockito/internal/framework/DefaultMockitoSession.java @@ -6,9 +6,12 @@ import org.mockito.Mockito; import org.mockito.MockitoAnnotations; +import org.mockito.MockitoFramework; import org.mockito.MockitoSession; import org.mockito.exceptions.misusing.RedundantListenerException; +import org.mockito.internal.configuration.plugins.Plugins; import org.mockito.internal.exceptions.Reporter; +import org.mockito.internal.junit.MockTracker; import org.mockito.internal.junit.TestFinishedEvent; import org.mockito.internal.junit.UniversalTestListener; import org.mockito.plugins.MockitoLogger; @@ -20,13 +23,16 @@ public class DefaultMockitoSession implements MockitoSession { private final String name; private final UniversalTestListener listener; + private final MockTracker mockTracker; - public DefaultMockitoSession(List testClassInstances, String name, Strictness strictness, MockitoLogger logger) { + public DefaultMockitoSession(List testClassInstances, String name, Strictness strictness, MockitoLogger logger, + boolean trackAndCleanUpMocks) { this.name = name; listener = new UniversalTestListener(strictness, logger); + MockitoFramework framework = Mockito.framework(); try { //So that the listener can capture mock creation events - Mockito.framework().addListener(listener); + framework.addListener(listener); } catch (RedundantListenerException e) { Reporter.unfinishedMockingSession(); } @@ -39,6 +45,18 @@ public DefaultMockitoSession(List testClassInstances, String name, Stric listener.setListenerDirty(); throw e; } + + if (trackAndCleanUpMocks && (framework instanceof DefaultMockitoFramework)) { + mockTracker = new MockTracker(Plugins.getMockMaker()); + try { + ((DefaultMockitoFramework) framework).addGlobalListener(mockTracker); + } catch (RedundantListenerException e) { + listener.setListenerDirty(); + Reporter.multipleTrackingMockSession(); + } + } else { + mockTracker = null; + } } @Override @@ -56,7 +74,11 @@ public void finishMocking(final Throwable failure) { //Cleaning up the state, we no longer need the listener hooked up //The listener implements MockCreationListener and at this point //we no longer need to listen on mock creation events. We are wrapping up the session - Mockito.framework().removeListener(listener); + MockitoFramework framework = Mockito.framework(); + framework.removeListener(listener); + if (mockTracker != null) { + ((DefaultMockitoFramework) framework).removeGlobalListener(mockTracker); + } //Emit test finished event so that validation such as strict stubbing can take place listener.testFinished(new TestFinishedEvent() { @@ -75,5 +97,9 @@ public String getTestName() { //Finally, validate user's misuse of Mockito framework. Mockito.validateMockitoUsage(); } + + if (mockTracker != null) { + mockTracker.testFinished(); + } } } diff --git a/src/main/java/org/mockito/internal/junit/MockTracker.java b/src/main/java/org/mockito/internal/junit/MockTracker.java new file mode 100644 index 0000000000..6ccc37d115 --- /dev/null +++ b/src/main/java/org/mockito/internal/junit/MockTracker.java @@ -0,0 +1,55 @@ +/* + * Copyright (c) 2019 Mockito contributors + * This program is made available under the terms of the MIT License. + */ + +package org.mockito.internal.junit; + +import java.lang.ref.WeakReference; +import java.util.ArrayList; +import java.util.List; +import org.mockito.internal.listeners.AutoCleanableListener; +import org.mockito.listeners.MockCreationListener; +import org.mockito.mock.MockCreationSettings; +import org.mockito.plugins.MockMaker; + +public class MockTracker implements MockCreationListener, AutoCleanableListener { + + private final List> mocks = new ArrayList>(); + private final MockMaker mockMaker; + private boolean listenerDirty; + + public MockTracker(MockMaker mockMaker) { + this.mockMaker = mockMaker; + } + + @Override + public void onMockCreated(Object mock, MockCreationSettings settings) { + synchronized (mocks) { + mocks.add(new WeakReference(mock)); + } + } + + public void testFinished() { + WeakReference[] localMocks; + synchronized (mocks) { + localMocks = mocks.toArray(new WeakReference[0]); + } + + for (WeakReference weakMock : localMocks) { + Object mock = weakMock.get(); + if (mock != null) { + mockMaker.cleanUpMock(mock); + } + } + } + + @Override + public boolean isListenerDirty() { + return listenerDirty; + } + + public void setListenerDirty() { + listenerDirty = true; + } +} diff --git a/src/main/java/org/mockito/internal/listeners/MockitoListenerUtil.java b/src/main/java/org/mockito/internal/listeners/MockitoListenerUtil.java new file mode 100644 index 0000000000..9d69ceefae --- /dev/null +++ b/src/main/java/org/mockito/internal/listeners/MockitoListenerUtil.java @@ -0,0 +1,35 @@ +/* + * Copyright (c) 2019 Mockito contributors + * This program is made available under the terms of the MIT License. + */ + +package org.mockito.internal.listeners; + +import java.util.LinkedList; +import java.util.List; +import java.util.Set; +import org.mockito.internal.exceptions.Reporter; +import org.mockito.listeners.MockitoListener; + +public class MockitoListenerUtil { + public static void addListener(MockitoListener listener, Set listeners) { + List delete = new LinkedList(); + for (MockitoListener existing : listeners) { + if (existing.getClass().equals(listener.getClass())) { + if (existing instanceof AutoCleanableListener && ((AutoCleanableListener) existing).isListenerDirty()) { + //dirty listener means that there was an exception even before the test started + //if we fail here with redundant mockito listener exception there will be multiple failures causing confusion + //so we simply remove the existing listener and move on + delete.add(existing); + } else { + Reporter.redundantMockitoListener(listener.getClass().getSimpleName()); + } + } + } + //delete dirty listeners so they don't occupy state/memory and don't receive notifications + for (MockitoListener toDelete : delete) { + listeners.remove(toDelete); + } + listeners.add(listener); + } +} diff --git a/src/main/java/org/mockito/internal/progress/MockingProgressImpl.java b/src/main/java/org/mockito/internal/progress/MockingProgressImpl.java index 5c4fbbc859..0aa95e41a4 100644 --- a/src/main/java/org/mockito/internal/progress/MockingProgressImpl.java +++ b/src/main/java/org/mockito/internal/progress/MockingProgressImpl.java @@ -8,8 +8,7 @@ import org.mockito.internal.configuration.GlobalConfiguration; import org.mockito.internal.debugging.Localized; import org.mockito.internal.debugging.LocationImpl; -import org.mockito.internal.exceptions.Reporter; -import org.mockito.internal.listeners.AutoCleanableListener; +import org.mockito.internal.listeners.MockitoListenerUtil; import org.mockito.invocation.Location; import org.mockito.listeners.MockCreationListener; import org.mockito.listeners.MockitoListener; @@ -20,8 +19,6 @@ import org.mockito.verification.VerificationStrategy; import java.util.LinkedHashSet; -import java.util.LinkedList; -import java.util.List; import java.util.Set; import static org.mockito.internal.exceptions.Reporter.unfinishedStubbing; @@ -73,7 +70,6 @@ public Set verificationListeners() { return verificationListeners; } - public void verificationStarted(VerificationMode verify) { validateState(); resetOngoingStubbing(); @@ -157,28 +153,7 @@ public void mockingStarted(Object mock, MockCreationSettings settings) { } public void addListener(MockitoListener listener) { - addListener(listener, listeners); - } - - static void addListener(MockitoListener listener, Set listeners) { - List delete = new LinkedList(); - for (MockitoListener existing : listeners) { - if (existing.getClass().equals(listener.getClass())) { - if (existing instanceof AutoCleanableListener && ((AutoCleanableListener) existing).isListenerDirty()) { - //dirty listener means that there was an exception even before the test started - //if we fail here with redundant mockito listener exception there will be multiple failures causing confusion - //so we simply remove the existing listener and move on - delete.add(existing); - } else { - Reporter.redundantMockitoListener(listener.getClass().getSimpleName()); - } - } - } - //delete dirty listeners so they don't occupy state/memory and don't receive notifications - for (MockitoListener toDelete : delete) { - listeners.remove(toDelete); - } - listeners.add(listener); + MockitoListenerUtil.addListener(listener, listeners); } public void removeListener(MockitoListener listener) { diff --git a/src/main/java/org/mockito/internal/session/DefaultMockitoSessionBuilder.java b/src/main/java/org/mockito/internal/session/DefaultMockitoSessionBuilder.java index f47ee15972..7250ad662f 100644 --- a/src/main/java/org/mockito/internal/session/DefaultMockitoSessionBuilder.java +++ b/src/main/java/org/mockito/internal/session/DefaultMockitoSessionBuilder.java @@ -23,6 +23,7 @@ public class DefaultMockitoSessionBuilder implements MockitoSessionBuilder { private String name; private Strictness strictness; private MockitoSessionLogger logger; + private boolean trackAndCleanMocks; @Override public MockitoSessionBuilder initMocks(Object testClassInstance) { @@ -60,6 +61,12 @@ public MockitoSessionBuilder logger(MockitoSessionLogger logger) { return this; } + @Override + public MockitoSessionBuilder trackAndCleanUpMocks() { + this.trackAndCleanMocks = true; + return this; + } + @Override public MockitoSession startMocking() { //Configure default values @@ -75,6 +82,7 @@ public MockitoSession startMocking() { } Strictness effectiveStrictness = this.strictness == null ? Strictness.STRICT_STUBS : this.strictness; MockitoLogger logger = this.logger == null ? Plugins.getMockitoLogger() : new MockitoLoggerAdapter(this.logger); - return new DefaultMockitoSession(effectiveTestClassInstances, effectiveName, effectiveStrictness, logger); + return new DefaultMockitoSession(effectiveTestClassInstances, effectiveName, effectiveStrictness, logger, + trackAndCleanMocks); } } diff --git a/src/main/java/org/mockito/plugins/MockMaker.java b/src/main/java/org/mockito/plugins/MockMaker.java index e4b0ca8b49..47ae428cd0 100644 --- a/src/main/java/org/mockito/plugins/MockMaker.java +++ b/src/main/java/org/mockito/plugins/MockMaker.java @@ -101,6 +101,16 @@ void resetMock( MockCreationSettings settings ); + /** + * 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. + * @since 2.24.4 + */ + @Incubating + void cleanUpMock(Object mock); + /** * Indicates if the given type can be mocked by this mockmaker. * diff --git a/src/main/java/org/mockito/session/MockitoSessionBuilder.java b/src/main/java/org/mockito/session/MockitoSessionBuilder.java index b3a758bac0..149b47889a 100644 --- a/src/main/java/org/mockito/session/MockitoSessionBuilder.java +++ b/src/main/java/org/mockito/session/MockitoSessionBuilder.java @@ -115,6 +115,26 @@ public interface MockitoSessionBuilder { @Incubating MockitoSessionBuilder logger(MockitoSessionLogger logger); + /** + * Enables mock tracking and cleanup. + * + *

When enabled the {@code MockitoSession} tracks all mocks and spies created in all threads between the {@code MockitoSession} + * is created and {@link MockitoSession#finishMocking()} is called. When {@link MockitoSession#finishMocking()} this + * {@code MockitoSession} cleans up internal states of all mocks and spies captured when the {@code MockitoSession} + * is open. Behaviors of any interaction to mocks and spies that have been cleaned up are undefined. + * + *

It's recommended to use this feature when using any inline mock maker. Due to limitation of JVM some mocks may + * not be correctly GCed when there are no strong references outside Mockito. Therefore enabling mock tracking and + * cleaning can prevent memory leaks. + * + *

There can only be at most one {@code MockitoSession} with this enabled at any given time. + * + * @return the same builder instance for fluent configuration of {@code MockitoSession}. + * @since 2.24.4 + */ + @Incubating + MockitoSessionBuilder trackAndCleanUpMocks(); + /** * Starts new mocking session! Creates new {@code MockitoSession} instance to initialize the session. * At this point annotated fields are initialized per {@link #initMocks(Object)} method. 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..ab9652f819 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,15 @@ 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(); + } + 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..e37db073fd 100644 --- a/src/test/java/org/mockito/internal/framework/DefaultMockitoFrameworkTest.java +++ b/src/test/java/org/mockito/internal/framework/DefaultMockitoFrameworkTest.java @@ -8,16 +8,21 @@ import org.junit.Test; import org.mockito.ArgumentMatchers; import org.mockito.MockSettings; +import org.mockito.Mockito; +import org.mockito.MockitoFramework; import org.mockito.StateMaster; import org.mockito.exceptions.misusing.RedundantListenerException; import org.mockito.listeners.MockCreationListener; import org.mockito.listeners.MockitoListener; +import org.mockito.listeners.VerificationListener; import org.mockito.mock.MockCreationSettings; import org.mockitoutil.TestBase; import java.util.List; import java.util.Set; +import static org.junit.Assert.assertTrue; +import static org.junit.Assume.assumeTrue; import static org.mockito.Mockito.*; import static org.mockitoutil.ThrowableAssert.assertThat; @@ -112,5 +117,99 @@ public void run() { "For more information, see the javadoc for RedundantListenerException class."); } + @Test(expected = IllegalArgumentException.class) + public void prevents_adding_null_global_listener() { + framework.addGlobalListener(null); + } + + @Test(expected = IllegalArgumentException.class) + public void prevents_removing_null_global_listener() { + framework.removeGlobalListener(null); + } + + @Test + public void ok_to_remove_unknown_global_listener() { + //it is safe to remove listener that was not added before + framework.removeGlobalListener(new MockitoListener() {}); + } + + @Test + public void ok_to_remove_global_listener_multiple_times() { + MockitoListener listener = new MockitoListener() {}; + + //when + framework.addListener(listener); + + //then it is ok to: + framework.removeGlobalListener(listener); + framework.removeGlobalListener(listener); + } + + @SuppressWarnings({"CheckReturnValue"}) + @Test + public void adds_global_creation_listener() { + MockitoFramework mockitoFramework = Mockito.framework(); + assumeTrue(mockitoFramework instanceof DefaultMockitoFramework); + DefaultMockitoFramework framework = (DefaultMockitoFramework)mockitoFramework; + + //given creation listener is added + MockCreationListener listener = mock(MockCreationListener.class); + framework.addGlobalListener(listener); + + //and hooked up correctly + mock(List.class); + verify(listener).onMockCreated(ArgumentMatchers.any(), any(MockCreationSettings.class)); + + //when + framework.removeGlobalListener(listener); + mock(Set.class); + + //then + verifyNoMoreInteractions(listener); + } + + @Test public void prevents_duplicate_global_listeners_of_the_same_type() { + MockitoFramework mockitoFramework = Mockito.framework(); + assumeTrue(mockitoFramework instanceof DefaultMockitoFramework); + final DefaultMockitoFramework framework = (DefaultMockitoFramework)mockitoFramework; + + //given creation listener is added + framework.addGlobalListener(new MyListener()); + + assertThat(new Runnable() { + @Override + public void run() { + framework.addGlobalListener(new MyListener()); + } + }) .throwsException(RedundantListenerException.class) + .throwsMessage("\n" + + "Problems adding Mockito listener.\n" + + "Listener of type 'MyListener' has already been added and not removed.\n" + + "It indicates that previous listener was not removed according to the API.\n" + + "When you add a listener, don't forget to remove the listener afterwards:\n" + + " Mockito.framework().removeListener(myListener);\n" + + "For more information, see the javadoc for RedundantListenerException class."); + } + + @Test + public void combines_verification_listeners() { + MockitoFramework mockitoFramework = Mockito.framework(); + assumeTrue(mockitoFramework instanceof DefaultMockitoFramework); + final DefaultMockitoFramework framework = (DefaultMockitoFramework)mockitoFramework; + + VerificationListener threadLocalListener = mock(VerificationListener.class); + framework.addListener(threadLocalListener); + + VerificationListener globalListener = mock(VerificationListener.class); + framework.addGlobalListener(globalListener); + + Set listeners = framework.verificationListeners(); + assertTrue(listeners.contains(threadLocalListener)); + assertTrue(listeners.contains(globalListener)); + + framework.removeListener(threadLocalListener); + framework.removeGlobalListener(globalListener); + } + private static class MyListener implements MockitoListener {} } diff --git a/src/test/java/org/mockito/internal/junit/MockTrackerTest.java b/src/test/java/org/mockito/internal/junit/MockTrackerTest.java new file mode 100644 index 0000000000..b9add80ac3 --- /dev/null +++ b/src/test/java/org/mockito/internal/junit/MockTrackerTest.java @@ -0,0 +1,45 @@ +/* + * Copyright (c) 2019 Mockito contributors + * This program is made available under the terms of the MIT License. + */ + +package org.mockito.internal.junit; + +import org.junit.Test; +import org.mockito.internal.creation.MockSettingsImpl; +import org.mockito.plugins.MockMaker; +import org.mockitoutil.TestBase; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +public class MockTrackerTest extends TestBase { + + @Test + public void clear_tracked_mocks_when_finish() { + MockMaker maker = mock(MockMaker.class); + + MockTracker tracker = new MockTracker(maker); + + Object mock1 = new Object(); + tracker.onMockCreated(mock1, new MockSettingsImpl()); + Object mock2 = new Object(); + tracker.onMockCreated(mock2, new MockSettingsImpl()); + + tracker.testFinished(); + + verify(maker).cleanUpMock(mock1); + verify(maker).cleanUpMock(mock2); + } + + @Test + public void set_listener_dirty() { + MockTracker tracker = new MockTracker(mock(MockMaker.class)); + + assertFalse(tracker.isListenerDirty()); + tracker.setListenerDirty(); + assertTrue(tracker.isListenerDirty()); + } +} diff --git a/src/test/java/org/mockito/internal/listeners/MockitoListenerUtilTest.java b/src/test/java/org/mockito/internal/listeners/MockitoListenerUtilTest.java new file mode 100644 index 0000000000..7abe9680f0 --- /dev/null +++ b/src/test/java/org/mockito/internal/listeners/MockitoListenerUtilTest.java @@ -0,0 +1,62 @@ +/* + * Copyright (c) 2019 Mockito contributors + * This program is made available under the terms of the MIT License. + */ + +package org.mockito.internal.listeners; + +import java.util.LinkedHashSet; +import java.util.Set; +import org.assertj.core.api.Assertions; +import org.assertj.core.api.ThrowableAssert; +import org.junit.Test; +import org.mockito.exceptions.misusing.RedundantListenerException; +import org.mockito.listeners.MockitoListener; +import org.mockitoutil.TestBase; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class MockitoListenerUtilTest extends TestBase { + @Test + public void should_not_allow_redundant_listeners() { + MockitoListener listener1 = mock(MockitoListener.class); + final MockitoListener listener2 = mock(MockitoListener.class); + + final Set listeners = new LinkedHashSet(); + + //when + MockitoListenerUtil.addListener(listener1, listeners); + + //then + Assertions.assertThatThrownBy(new ThrowableAssert.ThrowingCallable() { + public void call() { + MockitoListenerUtil.addListener(listener2, listeners); + } + }).isInstanceOf(RedundantListenerException.class); + } + + @Test + public void should_clean_up_listeners_automatically() { + MockitoListener someListener = mock(MockitoListener.class); + MyListener cleanListener = mock(MyListener.class); + MyListener dirtyListener = when(mock(MyListener.class).isListenerDirty()).thenReturn(true).getMock(); + + Set listeners = new LinkedHashSet(); + + //when + MockitoListenerUtil.addListener(someListener, listeners); + MockitoListenerUtil.addListener(dirtyListener, listeners); + + //then + Assertions.assertThat(listeners).containsExactlyInAnyOrder(someListener, dirtyListener); + + //when + MockitoListenerUtil.addListener(cleanListener, listeners); + + //then dirty listener was removed automatically + Assertions.assertThat(listeners).containsExactlyInAnyOrder(someListener, cleanListener); + } + + interface MyListener extends MockitoListener, AutoCleanableListener {} +} diff --git a/src/test/java/org/mockito/internal/progress/MockingProgressImplTest.java b/src/test/java/org/mockito/internal/progress/MockingProgressImplTest.java index 7ef03345a8..7d221d6b3c 100644 --- a/src/test/java/org/mockito/internal/progress/MockingProgressImplTest.java +++ b/src/test/java/org/mockito/internal/progress/MockingProgressImplTest.java @@ -66,45 +66,4 @@ public void shouldNotifyListenerSafely() throws Exception { mockingProgress.mockingStarted(null, null); } - @Test - public void should_not_allow_redundant_listeners() { - MockitoListener listener1 = mock(MockitoListener.class); - final MockitoListener listener2 = mock(MockitoListener.class); - - final Set listeners = new LinkedHashSet(); - - //when - MockingProgressImpl.addListener(listener1, listeners); - - //then - Assertions.assertThatThrownBy(new ThrowableAssert.ThrowingCallable() { - public void call() { - MockingProgressImpl.addListener(listener2, listeners); - } - }).isInstanceOf(RedundantListenerException.class); - } - - @Test - public void should_clean_up_listeners_automatically() { - MockitoListener someListener = mock(MockitoListener.class); - MyListener cleanListener = mock(MyListener.class); - MyListener dirtyListener = when(mock(MyListener.class).isListenerDirty()).thenReturn(true).getMock(); - - Set listeners = new LinkedHashSet(); - - //when - MockingProgressImpl.addListener(someListener, listeners); - MockingProgressImpl.addListener(dirtyListener, listeners); - - //then - Assertions.assertThat(listeners).containsExactlyInAnyOrder(someListener, dirtyListener); - - //when - MockingProgressImpl.addListener(cleanListener, listeners); - - //then dirty listener was removed automatically - Assertions.assertThat(listeners).containsExactlyInAnyOrder(someListener, cleanListener); - } - - interface MyListener extends MockitoListener, AutoCleanableListener {} } diff --git a/src/test/java/org/mockito/internal/session/DefaultMockitoSessionBuilderTest.java b/src/test/java/org/mockito/internal/session/DefaultMockitoSessionBuilderTest.java index a65ae215bf..0c9f181141 100644 --- a/src/test/java/org/mockito/internal/session/DefaultMockitoSessionBuilderTest.java +++ b/src/test/java/org/mockito/internal/session/DefaultMockitoSessionBuilderTest.java @@ -9,6 +9,7 @@ import org.mockito.Mock; import org.mockito.MockitoSession; import org.mockito.StateMaster; +import org.mockito.exceptions.misusing.MultipleTrackingMockSessionException; import org.mockito.exceptions.misusing.UnfinishedMockingSessionException; import org.mockito.quality.Strictness; import org.mockito.session.MockitoSessionLogger; @@ -17,6 +18,8 @@ import java.util.ArrayList; import java.util.List; import java.util.Set; +import java.util.concurrent.Callable; +import java.util.concurrent.FutureTask; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -43,6 +46,7 @@ public class DefaultMockitoSessionBuilderTest { new DefaultMockitoSessionBuilder().initMocks(this).startMocking().finishMocking(); new DefaultMockitoSessionBuilder().initMocks(new Object()).startMocking().finishMocking(); new DefaultMockitoSessionBuilder().strictness(Strictness.LENIENT).startMocking().finishMocking(); + new DefaultMockitoSessionBuilder().trackAndCleanUpMocks().startMocking().finishMocking(); } @Test public void creates_sessions_for_multiple_test_class_instances_for_repeated_calls() { @@ -107,6 +111,72 @@ public void run() { }).throwsException(UnfinishedMockingSessionException.class); } + @Test public void cant_open_multiple_tracking_mocks_sessions() throws Exception { + MockitoSession session = new DefaultMockitoSessionBuilder().trackAndCleanUpMocks().startMocking(); + + try { + FutureTask task = new FutureTask(new Callable() { + @Override + public Void call() { + ThrowableAssert.assertThat(new Runnable() { + public void run() { + new DefaultMockitoSessionBuilder().trackAndCleanUpMocks().startMocking(); + } + }).throwsException(MultipleTrackingMockSessionException.class); + return null; + } + }); + Thread t = new Thread(task); + t.start(); + task.get(); + } finally { + session.finishMocking(); + } + } + + @Test public void can_open_another_session_not_tracking_mocks() throws Exception { + MockitoSession session = new DefaultMockitoSessionBuilder().trackAndCleanUpMocks().startMocking(); + + try { + FutureTask task = new FutureTask(new Callable() { + @Override + public Void call() { + new DefaultMockitoSessionBuilder().startMocking().finishMocking(); + return null; + } + }); + Thread t = new Thread(task); + t.start(); + task.get(); + } finally { + session.finishMocking(); + } + } + + @Test public void cleans_listeners_after_disallow_multiple_tracking_sessions() throws Exception { + MockitoSession session = new DefaultMockitoSessionBuilder().trackAndCleanUpMocks().startMocking(); + + try { + FutureTask task = new FutureTask(new Callable() { + @Override + public Void call() { + ThrowableAssert.assertThat(new Runnable() { + public void run() { + new DefaultMockitoSessionBuilder().trackAndCleanUpMocks().startMocking(); + } + }).throwsException(MultipleTrackingMockSessionException.class); + new DefaultMockitoSessionBuilder().startMocking().finishMocking(); + return null; + } + }); + Thread t = new Thread(task); + t.start(); + task.get(); + } finally { + session.finishMocking(); + } + } + class TestClass { @Mock public Set set; diff --git a/src/test/java/org/mockitousage/stubbing/StubbingWarningsTest.java b/src/test/java/org/mockitousage/stubbing/StubbingWarningsTest.java index 6bf2041aa4..de05328e8d 100644 --- a/src/test/java/org/mockitousage/stubbing/StubbingWarningsTest.java +++ b/src/test/java/org/mockitousage/stubbing/StubbingWarningsTest.java @@ -28,7 +28,7 @@ public class StubbingWarningsTest { @Mock IMethods mock; SimpleMockitoLogger logger = new SimpleMockitoLogger(); - MockitoSession mockito = new DefaultMockitoSession(singletonList((Object) this), TEST_NAME, Strictness.WARN, logger); + MockitoSession mockito = new DefaultMockitoSession(singletonList((Object) this), TEST_NAME, Strictness.WARN, logger, false); @After public void after() { StateMaster stateMaster = new StateMaster(); diff --git a/subprojects/android/src/main/java/org/mockito/android/internal/creation/AndroidByteBuddyMockMaker.java b/subprojects/android/src/main/java/org/mockito/android/internal/creation/AndroidByteBuddyMockMaker.java index db77fb7b03..c9d1838a8d 100644 --- a/subprojects/android/src/main/java/org/mockito/android/internal/creation/AndroidByteBuddyMockMaker.java +++ b/subprojects/android/src/main/java/org/mockito/android/internal/creation/AndroidByteBuddyMockMaker.java @@ -49,6 +49,11 @@ public void resetMock(Object mock, MockHandler newHandler, MockCreationSettings delegate.resetMock(mock, newHandler, settings); } + @Override + public void cleanUpMock(Object mock) { + delegate.cleanUpMock(mock); + } + @Override public TypeMockability isTypeMockable(Class type) { return delegate.isTypeMockable(type); 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..dcd905a163 --- /dev/null +++ b/subprojects/inline/src/test/java/org/mockitoinline/bugs/CyclicMockMethodArgumentMemoryLeakTest.java @@ -0,0 +1,43 @@ +/* + * 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.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 MockitoSession session = mockitoSession().trackAndCleanUpMocks().startMocking(); + try { + final A a = mock(A.class); + a.largeArray = new int[ARRAY_LENGTH]; + final B b = mock(B.class); + + a.accept(b); + b.accept(a); + } finally { + session.finishMocking(); + } + } + } + + 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..2427a25380 --- /dev/null +++ b/subprojects/inline/src/test/java/org/mockitoinline/bugs/SelfSpyReferenceMemoryLeakTest.java @@ -0,0 +1,39 @@ +/* + * 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.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 MockitoSession session = mockitoSession().trackAndCleanUpMocks().startMocking(); + try { + final DeepRefSelfClass instance = spy(new DeepRefSelfClass()); + instance.refInstance(instance); + } finally { + session.finishMocking(); + } + } + } + + 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; + } + } +}