From f6e95923231e68c0f43ca27a5bfae12e0761b33f Mon Sep 17 00:00:00 2001 From: Max Grabenhorst Date: Sat, 24 Nov 2018 18:15:18 +0100 Subject: [PATCH 1/4] Fixes #1541: Prevent premature garbage collection of mock object when using 'One-liner stubs' by using strong ref in BaseStubbing --- .../internal/stubbing/BaseStubbing.java | 20 +++++++++ .../stubbing/ConsecutiveStubbing.java | 18 +++----- .../stubbing/OngoingStubbingImpl.java | 12 +----- .../mockitoinline/OneLinerStubStressTest.java | 41 +++++++++++++++++++ 4 files changed, 68 insertions(+), 23 deletions(-) create mode 100644 subprojects/inline/src/test/java/org/mockitoinline/OneLinerStubStressTest.java diff --git a/src/main/java/org/mockito/internal/stubbing/BaseStubbing.java b/src/main/java/org/mockito/internal/stubbing/BaseStubbing.java index 6dd99cd802..754a07f525 100644 --- a/src/main/java/org/mockito/internal/stubbing/BaseStubbing.java +++ b/src/main/java/org/mockito/internal/stubbing/BaseStubbing.java @@ -11,10 +11,24 @@ import org.mockito.internal.stubbing.answers.CallsRealMethods; import org.mockito.internal.stubbing.answers.Returns; import org.mockito.internal.stubbing.answers.ThrowsException; +import org.mockito.stubbing.Answer; import org.mockito.stubbing.OngoingStubbing; public abstract class BaseStubbing implements OngoingStubbing { + + // Keep strong ref to mock preventing premature garbage collection when using 'One-liner stubs'. See #1541. + private final Object strongMockRef; + + public BaseStubbing(InvocationContainerImpl invocationContainer) { + this.strongMockRef = invocationContainer.invokedMock(); + } + + @Override + public OngoingStubbing then(Answer answer) { + return thenAnswer(answer); + } + @Override public OngoingStubbing thenReturn(T value) { return thenAnswer(new Returns(value)); @@ -79,6 +93,12 @@ public OngoingStubbing thenThrow(Class toBeThrown, Class public OngoingStubbing thenCallRealMethod() { return thenAnswer(new CallsRealMethods()); } + + @Override + @SuppressWarnings("unchecked") + public M getMock() { + return (M) this.strongMockRef; + } } diff --git a/src/main/java/org/mockito/internal/stubbing/ConsecutiveStubbing.java b/src/main/java/org/mockito/internal/stubbing/ConsecutiveStubbing.java index a5cdc04829..dfb5022531 100644 --- a/src/main/java/org/mockito/internal/stubbing/ConsecutiveStubbing.java +++ b/src/main/java/org/mockito/internal/stubbing/ConsecutiveStubbing.java @@ -8,23 +8,17 @@ import org.mockito.stubbing.OngoingStubbing; public class ConsecutiveStubbing extends BaseStubbing { - private final InvocationContainerImpl invocationContainerImpl; - public ConsecutiveStubbing(InvocationContainerImpl invocationContainerImpl) { - this.invocationContainerImpl = invocationContainerImpl; + private final InvocationContainerImpl invocationContainer; + + public ConsecutiveStubbing(InvocationContainerImpl invocationContainer) { + super(invocationContainer); + this.invocationContainer = invocationContainer; } public OngoingStubbing thenAnswer(Answer answer) { - invocationContainerImpl.addConsecutiveAnswer(answer); + invocationContainer.addConsecutiveAnswer(answer); return this; } - public OngoingStubbing then(Answer answer) { - return thenAnswer(answer); - } - - @SuppressWarnings("unchecked") - public M getMock() { - return (M) invocationContainerImpl.invokedMock(); - } } diff --git a/src/main/java/org/mockito/internal/stubbing/OngoingStubbingImpl.java b/src/main/java/org/mockito/internal/stubbing/OngoingStubbingImpl.java index cd27c392dd..f1d1ef6035 100644 --- a/src/main/java/org/mockito/internal/stubbing/OngoingStubbingImpl.java +++ b/src/main/java/org/mockito/internal/stubbing/OngoingStubbingImpl.java @@ -19,6 +19,7 @@ public class OngoingStubbingImpl extends BaseStubbing { private Strictness strictness; public OngoingStubbingImpl(InvocationContainerImpl invocationContainer) { + super(invocationContainer); this.invocationContainer = invocationContainer; } @@ -32,22 +33,11 @@ public OngoingStubbing thenAnswer(Answer answer) { return new ConsecutiveStubbing(invocationContainer); } - @Override - public OngoingStubbing then(Answer answer) { - return thenAnswer(answer); - } - public List getRegisteredInvocations() { //TODO interface for tests return invocationContainer.getInvocations(); } - @Override - @SuppressWarnings("unchecked") - public M getMock() { - return (M) invocationContainer.invokedMock(); - } - public void setStrictness(Strictness strictness) { this.strictness = strictness; } diff --git a/subprojects/inline/src/test/java/org/mockitoinline/OneLinerStubStressTest.java b/subprojects/inline/src/test/java/org/mockitoinline/OneLinerStubStressTest.java new file mode 100644 index 0000000000..6cda6e422b --- /dev/null +++ b/subprojects/inline/src/test/java/org/mockitoinline/OneLinerStubStressTest.java @@ -0,0 +1,41 @@ +/* + * 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; + +public class OneLinerStubStressTest { + + public class OneLinerStubTestClass { + public String getStuff() { + return "A"; + } + } + + private static String generateLargeString() { + final int length = 1000000; + final StringBuilder stringBuilder = new StringBuilder(length); + for (int i = 0; i <= length; i++) { + stringBuilder.append("B"); + } + return stringBuilder.toString(); + } + + @Test + public void call_a_lot_of_mocks_using_one_line_stubbing() { + //This requires smaller heap set for the test process, see "inline.gradle" + final String returnValue = generateLargeString(); + for (int i = 0; i < 50000; i++) { + // make sure that mock object does not get cleaned up prematurely + final OneLinerStubTestClass mock = + when(mock(OneLinerStubTestClass.class).getStuff()).thenReturn(returnValue).getMock(); + assertEquals(returnValue, mock.getStuff()); + } + } +} From 40aae78266f57c6b598ef6eae7fac1b9afca27c6 Mon Sep 17 00:00:00 2001 From: Szczepan Faber Date: Sun, 25 Nov 2018 08:37:36 -0800 Subject: [PATCH 2/4] Stressed memory some more to get consistent failure --- .../src/test/java/org/mockitoinline/OneLinerStubStressTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subprojects/inline/src/test/java/org/mockitoinline/OneLinerStubStressTest.java b/subprojects/inline/src/test/java/org/mockitoinline/OneLinerStubStressTest.java index 6cda6e422b..44ff95959e 100644 --- a/subprojects/inline/src/test/java/org/mockitoinline/OneLinerStubStressTest.java +++ b/subprojects/inline/src/test/java/org/mockitoinline/OneLinerStubStressTest.java @@ -19,7 +19,7 @@ public String getStuff() { } private static String generateLargeString() { - final int length = 1000000; + final int length = 2000000; final StringBuilder stringBuilder = new StringBuilder(length); for (int i = 0; i <= length; i++) { stringBuilder.append("B"); From 88a2d3d02571afe575150673942c88548272d28f Mon Sep 17 00:00:00 2001 From: Szczepan Faber Date: Sun, 25 Nov 2018 08:28:53 -0800 Subject: [PATCH 3/4] Ensured that retryTest uses the same memory setting This way we can reliably run stress tests from 'inline' submodule --- subprojects/inline/inline.gradle | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/subprojects/inline/inline.gradle b/subprojects/inline/inline.gradle index b8d2577ab5..22db17b067 100644 --- a/subprojects/inline/inline.gradle +++ b/subprojects/inline/inline.gradle @@ -9,5 +9,6 @@ dependencies { tasks.javadoc.enabled = false -//required by the "StressTest.java" +//required by the "StressTest.java" and "OneLinerStubStressTest.java" test.maxHeapSize = "256m" +retryTest.maxHeapSize = "256m" From 38dc92d45410c5afb9f9c9902362944146d7dc21 Mon Sep 17 00:00:00 2001 From: Szczepan Faber Date: Sun, 25 Nov 2018 08:41:25 -0800 Subject: [PATCH 4/4] Simplified code, passing only mock as parameter Also made the constructors package-protected, followed by IDEA hint. It's always a good idea to reduce the scope. --- .../org/mockito/internal/stubbing/BaseStubbing.java | 12 ++++++------ .../internal/stubbing/ConsecutiveStubbing.java | 4 ++-- .../internal/stubbing/OngoingStubbingImpl.java | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/mockito/internal/stubbing/BaseStubbing.java b/src/main/java/org/mockito/internal/stubbing/BaseStubbing.java index 754a07f525..4c86cbd05d 100644 --- a/src/main/java/org/mockito/internal/stubbing/BaseStubbing.java +++ b/src/main/java/org/mockito/internal/stubbing/BaseStubbing.java @@ -4,24 +4,24 @@ */ package org.mockito.internal.stubbing; -import static org.mockito.internal.exceptions.Reporter.notAnException; -import static org.mockito.internal.progress.ThreadSafeMockingProgress.mockingProgress; -import static org.objenesis.ObjenesisHelper.newInstance; - import org.mockito.internal.stubbing.answers.CallsRealMethods; import org.mockito.internal.stubbing.answers.Returns; import org.mockito.internal.stubbing.answers.ThrowsException; import org.mockito.stubbing.Answer; import org.mockito.stubbing.OngoingStubbing; +import static org.mockito.internal.exceptions.Reporter.notAnException; +import static org.mockito.internal.progress.ThreadSafeMockingProgress.mockingProgress; +import static org.objenesis.ObjenesisHelper.newInstance; + public abstract class BaseStubbing implements OngoingStubbing { // Keep strong ref to mock preventing premature garbage collection when using 'One-liner stubs'. See #1541. private final Object strongMockRef; - public BaseStubbing(InvocationContainerImpl invocationContainer) { - this.strongMockRef = invocationContainer.invokedMock(); + BaseStubbing(Object mock) { + this.strongMockRef = mock; } @Override diff --git a/src/main/java/org/mockito/internal/stubbing/ConsecutiveStubbing.java b/src/main/java/org/mockito/internal/stubbing/ConsecutiveStubbing.java index dfb5022531..32ef8eedfd 100644 --- a/src/main/java/org/mockito/internal/stubbing/ConsecutiveStubbing.java +++ b/src/main/java/org/mockito/internal/stubbing/ConsecutiveStubbing.java @@ -11,8 +11,8 @@ public class ConsecutiveStubbing extends BaseStubbing { private final InvocationContainerImpl invocationContainer; - public ConsecutiveStubbing(InvocationContainerImpl invocationContainer) { - super(invocationContainer); + ConsecutiveStubbing(InvocationContainerImpl invocationContainer) { + super(invocationContainer.invokedMock()); this.invocationContainer = invocationContainer; } diff --git a/src/main/java/org/mockito/internal/stubbing/OngoingStubbingImpl.java b/src/main/java/org/mockito/internal/stubbing/OngoingStubbingImpl.java index f1d1ef6035..419160421e 100644 --- a/src/main/java/org/mockito/internal/stubbing/OngoingStubbingImpl.java +++ b/src/main/java/org/mockito/internal/stubbing/OngoingStubbingImpl.java @@ -19,7 +19,7 @@ public class OngoingStubbingImpl extends BaseStubbing { private Strictness strictness; public OngoingStubbingImpl(InvocationContainerImpl invocationContainer) { - super(invocationContainer); + super(invocationContainer.invokedMock()); this.invocationContainer = invocationContainer; }