Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New API to clean up all inline mocks after test #1619

Merged
merged 10 commits into from Mar 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/main/java/org/mockito/Mockito.java
Expand Up @@ -103,7 +103,8 @@
* <a href="#43">43. New API for integrations: <code>MockitoSession</code> is usable by testing frameworks (Since 2.15.+)</a><br/>
* <a href="#44">44. Deprecated <code>org.mockito.plugins.InstantiatorProvider</code> as it was leaking internal API. it was replaced by <code>org.mockito.plugins.InstantiatorProvider2 (Since 2.15.4)</code></a><br/>
* <a href="#45">45. New JUnit Jupiter (JUnit5+) extension</a><br/>
* <a href="#46">46. New <code>Mockito.lenient()</code> and <code>MockSettings.lenient()</code> methods (Since 2.20.0</a><br/>
* <a href="#46">46. New <code>Mockito.lenient()</code> and <code>MockSettings.lenient()</code> methods (Since 2.20.0)</a><br/>
* <a href="#47">47. New API for clearing mock state in inline mocking (Since 2.25.0)</a><br/>
* </b>
*
* <h3 id="0">0. <a class="meaningful_link" href="#mockito2" name="mockito2">Migrating to Mockito 2</a></h3>
Expand Down Expand Up @@ -1530,6 +1531,15 @@
*
* 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!
*
* <h3 id="47">47. <a class="meaningful_link" href="#clear_inline_mocks" name="clear_inline_mocks">New API for clearing mock state in inline mocking (Since 2.25.0)</a></h3>
*
* In certain specific, rare scenarios (issue <a href="https://github.com/mockito/mockito/pull/1619">#1619</a>)
* inline mocking causes memory leaks.
* There is no clean way to mitigate this problem completely.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* There is no clean way to mitigate this problem completely.
* It was not possible to resolve this issue without introducing an additional API.
* Any inline MockMaker should therefore implement the additional interface {@link org.mockito.plugins.InlineMockMaker} to allow Mockito to properly clean up mocks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm skipping this suggestion if that's OK. I prefer skipping those details in the high level javadoc. Also, I prefer the original text grammatically.

Feel free to redo the javadoc if you want, in a separate PR - I'm happy to review/merge it.

* 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 {
Expand Down
51 changes: 51 additions & 0 deletions src/main/java/org/mockito/MockitoFramework.java
Expand Up @@ -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.
* <p>
* This method is useful to tackle subtle memory leaks that are possible due to the nature of inline mocking
* (issue <a href="https://github.com/mockito/mockito/pull/1619">#1619</a>).
* If you are facing those problems, call this method at the end of the test (or in "@After" method).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we update our JUnit runner and extension to handle this automatically? I think this is an improvement in any situation, so we can just go ahead and do it eagerly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a breaking change in thread-based parallel tests that are supported by Mockito (with limitations, as we stated in the FAQ).

If you turn this on by default, some Mockito tests will start failing. @ttanxu, discussed this somewhere in this PR (but the thread became long :).

We can always make it later if we understand the use patterns better. I suggest we expose the this API first and observe / learn how it goes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, I missed that discussion. Thanks for the clarification.

* 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}.
* <p>
* 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.
*
* <pre class="code"><code class="java">
* public class ExampleTest {
*
* &#064;After
* public void clearMocks() {
* Mockito.framework().clearInlineMocks();
* }
*
* &#064;Test
* public void someTest() {
* ...
* }
* }
* </pre>
*
* 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);
}
Expand Up @@ -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;
Expand Down Expand Up @@ -89,7 +90,7 @@
* support this feature.
*/
@Incubating
public class InlineByteBuddyMockMaker implements ClassCreatingMockMaker {
public class InlineByteBuddyMockMaker implements ClassCreatingMockMaker, InlineMockMaker {

private static final Instrumentation INSTRUMENTATION;

Expand Down Expand Up @@ -271,6 +272,16 @@ public void resetMock(Object mock, MockHandler newHandler, MockCreationSettings
}
}

@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() {
Expand Down
Expand Up @@ -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;
Expand Down Expand Up @@ -37,4 +39,25 @@ public MockitoPlugins getPlugins() {
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);
}
}
}
52 changes: 52 additions & 0 deletions src/main/java/org/mockito/plugins/InlineMockMaker.java
@@ -0,0 +1,52 @@
/*
* 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.
* <p>
* 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 <a href="https://github.com/mockito/mockito/pull/1619">#1619</a>).
* 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()}.
* <p>
* {@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();

}
Expand Up @@ -20,7 +20,11 @@
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;
Expand Down Expand Up @@ -287,6 +291,37 @@ public void test_parameters_retention() throws Exception {
.getOnly().getParameters().getOnly().getName()).isEqualTo("bar");
}

@Test
public void test_clear_mock_clears_handler() {
MockCreationSettings<GenericSubClass> settings = settingsFor(GenericSubClass.class);
GenericSubClass proxy = mockMaker.createMock(settings, new MockHandlerImpl<GenericSubClass>(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<GenericSubClass> settings = settingsFor(GenericSubClass.class);
GenericSubClass proxy1 = mockMaker.createMock(settings, new MockHandlerImpl<GenericSubClass>(settings));
assertThat(mockMaker.getHandler(proxy1)).isNotNull();

settings = settingsFor(GenericSubClass.class);
GenericSubClass proxy2 = mockMaker.createMock(settings, new MockHandlerImpl<GenericSubClass>(settings));
assertThat(mockMaker.getHandler(proxy1)).isNotNull();

//when
mockMaker.clearAllMocks();

//then
assertThat(mockMaker.getHandler(proxy1)).isNull();
assertThat(mockMaker.getHandler(proxy2)).isNull();
}

private static <T> MockCreationSettings<T> settingsFor(Class<T> type, Class<?>... extraInterfaces) {
MockSettingsImpl<T> mockSettings = new MockSettingsImpl<T>();
mockSettings.setTypeToMock(type);
Expand Down
Expand Up @@ -10,15 +10,26 @@
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 {
Expand Down Expand Up @@ -112,5 +123,52 @@ public void run() {
"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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, one of the reason why I didn't use assume at first is that it can test user can safely call clearInlineMocks() and clearMock() even when mock maker is not a inline mock maker.

That's not very important I suppose, but still would be good to cover that in tests imo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 I'm fixing it now.


//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 {}
}
@@ -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 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) {}
}
}