From 0391e261bdca99075f433062cf14eff2113fc483 Mon Sep 17 00:00:00 2001 From: James Baker Date: Thu, 4 Aug 2022 19:25:37 +0100 Subject: [PATCH 1/7] Fixes #2720 Use StackWalker on Java 9+ to create Locations In terms of memory allocations, this reduces the overall memory allocations of creating a location by an order of magnitude in Java 9, and as compared to Java 8. The implementation is somewhat involved due to these desires: - Minimize the amount of work done if the Location is never used. This is done by not converting the StackFrame into a StackTraceElement, instead wrapping in an intermediate state. The StackTraceElement conversion will only occur (internally) if the .getFileName() method is called. - Ensure the implementation is still Serializable. This is ensured with a .writeReplace method. - Minimize the number of allocations, which is basically an exercise in lambda caching. - Ensuring the old mechanism still works on Java 8. Presently on Java 9+, on a stack depth of 1000 the old mechanism will allocate 40kB of RAM per call. The new one will allocate 1.5kB of RAM per call, which is a huge improvement. This is still sadly not the 'close-to-no-overhead' solution I was looking for. I therefore also added a system property that can be used to fully disable Location creation. I'm aware that this is likely not the right approach given Mockito has plugins and settings - mostly looking for guidance here given I'm not sure what would be idiomatic here. --- .../stacktrace/StackTraceCleaner.java | 25 ++ .../internal/MockedConstructionImpl.java | 4 +- .../mockito/internal/MockedStaticImpl.java | 4 +- .../creation/bytebuddy/MockMethodAdvice.java | 10 +- .../bytebuddy/MockMethodInterceptor.java | 4 +- .../creation/proxy/ProxyMockMaker.java | 9 +- ...cationImpl.java => Java8LocationImpl.java} | 10 +- .../debugging/Java9PlusLocationImpl.java | 262 ++++++++++++++++++ .../mockito/internal/debugging/Localized.java | 2 +- .../internal/debugging/LocationFactory.java | 75 +++++ .../mockito/internal/exceptions/Reporter.java | 28 +- .../stacktrace/DefaultStackTraceCleaner.java | 13 +- .../invocation/DefaultInvocationFactory.java | 4 +- .../internal/matchers/LocalizedMatcher.java | 4 +- .../progress/MockingProgressImpl.java | 4 +- .../defaultanswers/ReturnsSmartNulls.java | 5 +- .../invocation/InvocationBuilder.java | 4 +- .../defaultanswers/ReturnsSmartNullsTest.java | 6 +- .../debugging/LocationFactoryTest.java | 100 +++++++ .../internal/debugging/LocationImplTest.java | 63 ----- src/test/java/org/mockitoutil/TestBase.java | 4 +- 21 files changed, 525 insertions(+), 115 deletions(-) rename src/main/java/org/mockito/internal/debugging/{LocationImpl.java => Java8LocationImpl.java} (88%) create mode 100644 src/main/java/org/mockito/internal/debugging/Java9PlusLocationImpl.java create mode 100644 src/main/java/org/mockito/internal/debugging/LocationFactory.java create mode 100644 src/test/java/org/mockitousage/internal/debugging/LocationFactoryTest.java delete mode 100644 src/test/java/org/mockitousage/internal/debugging/LocationImplTest.java diff --git a/src/main/java/org/mockito/exceptions/stacktrace/StackTraceCleaner.java b/src/main/java/org/mockito/exceptions/stacktrace/StackTraceCleaner.java index a3229a8c80..467809aa01 100644 --- a/src/main/java/org/mockito/exceptions/stacktrace/StackTraceCleaner.java +++ b/src/main/java/org/mockito/exceptions/stacktrace/StackTraceCleaner.java @@ -25,4 +25,29 @@ public interface StackTraceCleaner { * @return whether the element should be excluded from cleaned stack trace. */ boolean isIn(StackTraceElement candidate); + + /** + * It's recommended to override this method in subclasses to avoid potentially costly re-boxing operations. + */ + default boolean isIn(StackFrameMetadata candidate) { + return isIn( + new StackTraceElement( + candidate.getClassName(), + candidate.getMethodName(), + candidate.getFileName(), + candidate.getLineNumber())); + } + + /** + * Very similar to the StackFrame class declared on the StackWalker api. + */ + interface StackFrameMetadata { + String getClassName(); + + String getMethodName(); + + String getFileName(); + + int getLineNumber(); + } } diff --git a/src/main/java/org/mockito/internal/MockedConstructionImpl.java b/src/main/java/org/mockito/internal/MockedConstructionImpl.java index 47bd8089c6..5541ba07cd 100644 --- a/src/main/java/org/mockito/internal/MockedConstructionImpl.java +++ b/src/main/java/org/mockito/internal/MockedConstructionImpl.java @@ -11,7 +11,7 @@ import org.mockito.MockedConstruction; import org.mockito.exceptions.base.MockitoException; -import org.mockito.internal.debugging.LocationImpl; +import org.mockito.internal.debugging.LocationFactory; import org.mockito.invocation.Location; import org.mockito.plugins.MockMaker; @@ -21,7 +21,7 @@ public final class MockedConstructionImpl implements MockedConstruction { private boolean closed; - private final Location location = new LocationImpl(); + private final Location location = LocationFactory.create(); protected MockedConstructionImpl(MockMaker.ConstructionMockControl control) { this.control = control; diff --git a/src/main/java/org/mockito/internal/MockedStaticImpl.java b/src/main/java/org/mockito/internal/MockedStaticImpl.java index fbfb54b004..f6705fb81a 100644 --- a/src/main/java/org/mockito/internal/MockedStaticImpl.java +++ b/src/main/java/org/mockito/internal/MockedStaticImpl.java @@ -17,7 +17,7 @@ import org.mockito.Mockito; import org.mockito.exceptions.base.MockitoAssertionError; import org.mockito.exceptions.base.MockitoException; -import org.mockito.internal.debugging.LocationImpl; +import org.mockito.internal.debugging.LocationFactory; import org.mockito.internal.listeners.VerificationStartedNotifier; import org.mockito.internal.progress.MockingProgress; import org.mockito.internal.stubbing.InvocationContainerImpl; @@ -35,7 +35,7 @@ public final class MockedStaticImpl implements MockedStatic { private boolean closed; - private final Location location = new LocationImpl(); + private final Location location = LocationFactory.create(); protected MockedStaticImpl(MockMaker.StaticMockControl control) { this.control = control; diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodAdvice.java b/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodAdvice.java index fc92e49acf..1dd744171f 100644 --- a/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodAdvice.java +++ b/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodAdvice.java @@ -51,7 +51,7 @@ import org.mockito.exceptions.base.MockitoException; import org.mockito.internal.configuration.plugins.Plugins; import org.mockito.internal.creation.bytebuddy.inject.MockMethodDispatcher; -import org.mockito.internal.debugging.LocationImpl; +import org.mockito.internal.debugging.LocationFactory; import org.mockito.internal.exceptions.stacktrace.ConditionalStackTraceFilter; import org.mockito.internal.invocation.RealMethod; import org.mockito.internal.invocation.SerializableMethod; @@ -132,11 +132,7 @@ public Callable handle(Object instance, Method origin, Object[] arguments) th } return new ReturnValueWrapper( interceptor.doIntercept( - instance, - origin, - arguments, - realMethod, - new LocationImpl(new Throwable(), true))); + instance, origin, arguments, realMethod, LocationFactory.create(true))); } @Override @@ -154,7 +150,7 @@ public Callable handleStatic(Class type, Method origin, Object[] arguments origin, arguments, new StaticMethodCall(selfCallInfo, type, origin, arguments), - new LocationImpl(new Throwable(), true))); + LocationFactory.create(true))); } @Override diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodInterceptor.java b/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodInterceptor.java index 83908cace6..406dea39a5 100644 --- a/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodInterceptor.java +++ b/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodInterceptor.java @@ -22,7 +22,7 @@ import net.bytebuddy.implementation.bind.annotation.StubValue; import net.bytebuddy.implementation.bind.annotation.SuperCall; import net.bytebuddy.implementation.bind.annotation.This; -import org.mockito.internal.debugging.LocationImpl; +import org.mockito.internal.debugging.LocationFactory; import org.mockito.internal.invocation.RealMethod; import org.mockito.invocation.Location; import org.mockito.invocation.MockHandler; @@ -53,7 +53,7 @@ private void readObject(ObjectInputStream stream) throws IOException, ClassNotFo Object doIntercept(Object mock, Method invokedMethod, Object[] arguments, RealMethod realMethod) throws Throwable { - return doIntercept(mock, invokedMethod, arguments, realMethod, new LocationImpl()); + return doIntercept(mock, invokedMethod, arguments, realMethod, LocationFactory.create()); } Object doIntercept( diff --git a/src/main/java/org/mockito/internal/creation/proxy/ProxyMockMaker.java b/src/main/java/org/mockito/internal/creation/proxy/ProxyMockMaker.java index faa97e849b..88e6886111 100644 --- a/src/main/java/org/mockito/internal/creation/proxy/ProxyMockMaker.java +++ b/src/main/java/org/mockito/internal/creation/proxy/ProxyMockMaker.java @@ -5,7 +5,7 @@ package org.mockito.internal.creation.proxy; import org.mockito.exceptions.base.MockitoException; -import org.mockito.internal.debugging.LocationImpl; +import org.mockito.internal.debugging.LocationFactory; import org.mockito.internal.invocation.RealMethod; import org.mockito.internal.util.Platform; import org.mockito.invocation.MockHandler; @@ -153,7 +153,12 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl return handler.get() .handle( createInvocation( - proxy, method, args, realMethod, settings, new LocationImpl())); + proxy, + method, + args, + realMethod, + settings, + LocationFactory.create())); } } diff --git a/src/main/java/org/mockito/internal/debugging/LocationImpl.java b/src/main/java/org/mockito/internal/debugging/Java8LocationImpl.java similarity index 88% rename from src/main/java/org/mockito/internal/debugging/LocationImpl.java rename to src/main/java/org/mockito/internal/debugging/Java8LocationImpl.java index cf255013e2..3cbffe42be 100644 --- a/src/main/java/org/mockito/internal/debugging/LocationImpl.java +++ b/src/main/java/org/mockito/internal/debugging/Java8LocationImpl.java @@ -9,7 +9,7 @@ import org.mockito.internal.exceptions.stacktrace.StackTraceFilter; import org.mockito.invocation.Location; -public class LocationImpl implements Location, Serializable { +class Java8LocationImpl implements Location, Serializable { private static final long serialVersionUID = -9054861157390980624L; // Limit the amount of objects being created, as this class is heavily instantiated: @@ -18,19 +18,19 @@ public class LocationImpl implements Location, Serializable { private String stackTraceLine; private String sourceFile; - public LocationImpl() { + public Java8LocationImpl() { this(new Throwable(), false); } - public LocationImpl(Throwable stackTraceHolder, boolean isInline) { + public Java8LocationImpl(Throwable stackTraceHolder, boolean isInline) { this(stackTraceFilter, stackTraceHolder, isInline); } - public LocationImpl(StackTraceFilter stackTraceFilter) { + public Java8LocationImpl(StackTraceFilter stackTraceFilter) { this(stackTraceFilter, new Throwable(), false); } - private LocationImpl( + private Java8LocationImpl( StackTraceFilter stackTraceFilter, Throwable stackTraceHolder, boolean isInline) { computeStackTraceInformation(stackTraceFilter, stackTraceHolder, isInline); } diff --git a/src/main/java/org/mockito/internal/debugging/Java9PlusLocationImpl.java b/src/main/java/org/mockito/internal/debugging/Java9PlusLocationImpl.java new file mode 100644 index 0000000000..27b02c3d31 --- /dev/null +++ b/src/main/java/org/mockito/internal/debugging/Java9PlusLocationImpl.java @@ -0,0 +1,262 @@ +/* + * Copyright (c) 2007 Mockito contributors + * This program is made available under the terms of the MIT License. + */ +package org.mockito.internal.debugging; + +import org.mockito.exceptions.stacktrace.StackTraceCleaner; +import org.mockito.exceptions.stacktrace.StackTraceCleaner.StackFrameMetadata; +import org.mockito.internal.configuration.plugins.Plugins; +import org.mockito.internal.exceptions.stacktrace.DefaultStackTraceCleaner; +import org.mockito.invocation.Location; + +import java.io.Serializable; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.Collections; +import java.util.List; +import java.util.Set; +import java.util.function.Function; +import java.util.function.Predicate; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +class Java9PlusLocationImpl implements Location, Serializable { + private static final long serialVersionUID = 2954388321980069195L; + private static final String STACK_WALKER = "java.lang.StackWalker"; + private static final String STACK_FRAME = STACK_WALKER + "$StackFrame"; + private static final String OPTION = STACK_WALKER + "$Option"; + private static final String SHOW_REFLECT_FRAMES = "SHOW_REFLECT_FRAMES"; + + /** + * This is an unfortunate buffer. Inside StackWalker, a buffer is created, which is resized by + * doubling. The resizing also allocates a tonne of StackFrame elements. If we traverse more than + * BUFFER_SIZE elements, the resulting resize can significantly affect the overall cost of the operation. + * If we traverse fewer than this number, we are inefficient. Empirically, 16 is enough stack frames + * for a simple stub+call operation to succeed without resizing, as measured on Java 11. + */ + private static final int BUFFER_SIZE = 16; + + private static final Class stackWalkerClazz = clazz(STACK_WALKER); + private static final Class stackFrameClazz = clazz(STACK_FRAME); + private static final Class optionClazz = clazz(OPTION); + + private static final Object stackWalker = stackWalker(); + private static final Method walk = walk(); + + private static final String PREFIX = "-> at "; + + private static final StackTraceCleaner CLEANER = + Plugins.getStackTraceCleanerProvider() + .getStackTraceCleaner(new DefaultStackTraceCleaner()); + + private static final Function toStackFrameMetadata = + MetadataShim::new; + private static final Predicate cleanerIsIn = CLEANER::isIn; + + private static final int FRAMES_TO_SKIP = framesToSkip(); + + private final StackFrameMetadata sfm; + private volatile String stackTraceLine; + + Java9PlusLocationImpl(boolean isInline) { + this.sfm = getStackFrame(isInline); + } + + @Override + public String getSourceFile() { + return sfm.getFileName(); + } + + @Override + public String toString() { + maybeInitializeStackTraceLine(); + return stackTraceLine; + } + + private void maybeInitializeStackTraceLine() { + if (stackTraceLine == null) { + synchronized (this) { + if (stackTraceLine == null) { + stackTraceLine = stackTraceLine(sfm); + } + } + } + } + + private static String stackTraceLine(StackFrameMetadata element) { + return PREFIX + element.toString(); + } + + private static StackFrameMetadata getStackFrame(boolean isInline) { + return stackWalk( + stream -> + stream.map(toStackFrameMetadata) + .skip(FRAMES_TO_SKIP) + .filter(cleanerIsIn) + .skip(isInline ? 1 : 0) + .findFirst() + .orElseThrow( + () -> + new IllegalArgumentException( + "Mockito could not find the first non-Mockito stack frame"))); + } + + /** + * In order to trigger the stack walker, we create some reflective frames. These need to be skipped so as to + * ensure there are no non-Mockito frames at the top of the stack trace. + */ + private static int framesToSkip() { + return stackWalk( + stream -> { + List metadata = + stream.map(toStackFrameMetadata) + .map(StackFrameMetadata::getClassName) + .collect(Collectors.toList()); + return metadata.indexOf(Java9PlusLocationImpl.class.getName()); + }); + } + + @SuppressWarnings("unchecked") + private static T stackWalk(Function, T> function) { + try { + return (T) walk.invoke(stackWalker, function); + } catch (InvocationTargetException | IllegalAccessException e) { + throw new RuntimeException(e); + } + } + + private static Method walk() { + try { + return stackWalkerClazz.getMethod("walk", Function.class); + } catch (NoSuchMethodException e) { + throw new RuntimeException(e); + } + } + + private static Class clazz(String name) { + try { + return Class.forName(name); + } catch (ClassNotFoundException e) { + throw new RuntimeException(e); + } + } + + @SuppressWarnings({"unchecked", "rawtypes"}) + private static Object stackWalker() { + try { + Set options = + Collections.singleton(Enum.valueOf((Class) optionClazz, SHOW_REFLECT_FRAMES)); + Method getInstance = + stackWalkerClazz.getDeclaredMethod("getInstance", Set.class, int.class); + return getInstance.invoke(null, options, BUFFER_SIZE); + } catch (InvocationTargetException | NoSuchMethodException | IllegalAccessException e) { + throw new RuntimeException(e); + } + } + + private static final class MetadataShim implements StackFrameMetadata, Serializable { + private static final long serialVersionUID = 8491903719411428648L; + private static final Method getClassName = getter("getClassName"); + private static final Method getMethodName = getter("getMethodName"); + private static final Method getFileName = getter("getFileName"); + private static final Method getLineNumber = getter("getLineNumber"); + private static final Method toString = getter(Object.class, "toString"); + + private final Object stackFrame; + + private MetadataShim(Object stackFrame) { + this.stackFrame = stackFrame; + } + + @Override + public String getClassName() { + return (String) get(getClassName); + } + + @Override + public String getMethodName() { + return (String) get(getMethodName); + } + + @Override + public String getFileName() { + return (String) get(getFileName); + } + + @Override + public int getLineNumber() { + return (int) get(getLineNumber); + } + + @Override + public String toString() { + return (String) get(toString); + } + + /** + * Ensure that this type remains serializable. + */ + private Object writeReplace() { + return new SerializableShim(toStackTraceElement()); + } + + private StackTraceElement toStackTraceElement() { + try { + Method method = stackFrameClazz.getMethod("toStackTraceElement"); + return (StackTraceElement) method.invoke(stackFrame); + } catch (InvocationTargetException | NoSuchMethodException | IllegalAccessException e) { + throw new RuntimeException(e); + } + } + + private Object get(Method handle) { + try { + return handle.invoke(stackFrame); + } catch (InvocationTargetException | IllegalAccessException e) { + throw new RuntimeException(e); + } + } + + private static Method getter(String name) { + return getter(stackFrameClazz, name); + } + + private static Method getter(Class clazz, String name) { + try { + return clazz.getDeclaredMethod(name); + } catch (Throwable t) { + throw new RuntimeException(t); + } + } + } + + private static final class SerializableShim implements StackFrameMetadata, Serializable { + private static final long serialVersionUID = 7908320459080898690L; + private final StackTraceElement ste; + + private SerializableShim(StackTraceElement ste) { + this.ste = ste; + } + + @Override + public String getClassName() { + return ste.getClassName(); + } + + @Override + public String getMethodName() { + return ste.getMethodName(); + } + + @Override + public String getFileName() { + return ste.getFileName(); + } + + @Override + public int getLineNumber() { + return ste.getLineNumber(); + } + } +} diff --git a/src/main/java/org/mockito/internal/debugging/Localized.java b/src/main/java/org/mockito/internal/debugging/Localized.java index d1d7912dc4..3abcf29555 100644 --- a/src/main/java/org/mockito/internal/debugging/Localized.java +++ b/src/main/java/org/mockito/internal/debugging/Localized.java @@ -13,7 +13,7 @@ public class Localized { public Localized(T object) { this.object = object; - location = new LocationImpl(); + location = LocationFactory.create(); } public T getObject() { diff --git a/src/main/java/org/mockito/internal/debugging/LocationFactory.java b/src/main/java/org/mockito/internal/debugging/LocationFactory.java new file mode 100644 index 0000000000..19dd9ecaf2 --- /dev/null +++ b/src/main/java/org/mockito/internal/debugging/LocationFactory.java @@ -0,0 +1,75 @@ +/* + * Copyright (c) 2007 Mockito contributors + * This program is made available under the terms of the MIT License. + */ +package org.mockito.internal.debugging; + +import org.mockito.invocation.Location; + +import java.io.Serializable; + +public final class LocationFactory { + private static final String PROPERTY = "mockito.locationFactory.disable"; + private static final Factory factory; + + static { + Factory lFactory; + if (Boolean.getBoolean(PROPERTY)) { + factory = unused -> NoLocation.INSTANCE; + } else { + try { + Class.forName("java.lang.StackWalker"); + lFactory = new Java9PlusLocationFactory(); + } catch (ClassNotFoundException e) { + lFactory = new Java8LocationFactory(); + } + factory = lFactory; + } + } + + private LocationFactory() {} + + public static Location create() { + return create(false); + } + + public static Location create(boolean inline) { + return factory.create(inline); + } + + private interface Factory { + Location create(boolean inline); + } + + private static final class Java8LocationFactory implements Factory { + @Override + public Location create(boolean inline) { + return new Java8LocationImpl(new Throwable(), inline); + } + } + + private static final class Java9PlusLocationFactory implements Factory { + + @Override + public Location create(boolean inline) { + return new Java9PlusLocationImpl(inline); + } + } + + private static final class NoLocation implements Location, Serializable { + private static final long serialVersionUID = -3807068467458099012L; + private static final NoLocation INSTANCE = new NoLocation(); + + private NoLocation() {} + + @Override + public String getSourceFile() { + return ""; + } + + @Override + public String toString() { + return "-> at <>"; + } + } +} diff --git a/src/main/java/org/mockito/internal/exceptions/Reporter.java b/src/main/java/org/mockito/internal/exceptions/Reporter.java index ea670407e7..9a5ea90d21 100644 --- a/src/main/java/org/mockito/internal/exceptions/Reporter.java +++ b/src/main/java/org/mockito/internal/exceptions/Reporter.java @@ -23,7 +23,7 @@ import org.mockito.exceptions.verification.TooManyActualInvocations; import org.mockito.exceptions.verification.VerificationInOrderFailure; import org.mockito.exceptions.verification.WantedButNotInvoked; -import org.mockito.internal.debugging.LocationImpl; +import org.mockito.internal.debugging.LocationFactory; import org.mockito.internal.exceptions.util.ScenarioPrinter; import org.mockito.internal.junit.ExceptionFactory; import org.mockito.internal.matchers.LocalizedMatcher; @@ -84,7 +84,7 @@ public static MockitoException incorrectUseOfApi() { return new MockitoException( join( "Incorrect use of API detected here:", - new LocationImpl(), + LocationFactory.create(), "", "You probably stored a reference to OngoingStubbing returned by when() and called stubbing methods like thenReturn() on this reference more than once.", "Examples of correct usage:", @@ -262,7 +262,7 @@ public static MockitoException incorrectUseOfAdditionalMatchers( "Invalid use of argument matchers inside additional matcher " + additionalMatcherName + " !", - new LocationImpl(), + LocationFactory.create(), "", expectedSubMatchersCount + " sub matchers expected, " @@ -295,7 +295,7 @@ public static MockitoException reportNoSubMatchersFound(String additionalMatcher return new InvalidUseOfMatchersException( join( "No matchers found for additional matcher " + additionalMatcherName, - new LocationImpl(), + LocationFactory.create(), "")); } @@ -322,7 +322,7 @@ public static AssertionError argumentsAreDifferent( .append("Argument(s) are different! Wanted:\n") .append(wanted) .append("\n") - .append(new LocationImpl()) + .append(LocationFactory.create()) .append("\n") .append("Actual invocations have different arguments:\n"); @@ -365,7 +365,7 @@ public static MockitoAssertionError wantedButNotInvoked( } private static String createWantedButNotInvokedMessage(DescribedInvocation wanted) { - return join("Wanted but not invoked:", wanted.toString(), new LocationImpl(), ""); + return join("Wanted but not invoked:", wanted.toString(), LocationFactory.create(), ""); } public static MockitoAssertionError wantedButNotInvokedInOrder( @@ -375,7 +375,7 @@ public static MockitoAssertionError wantedButNotInvokedInOrder( "Verification in order failure", "Wanted but not invoked:", wanted.toString(), - new LocationImpl(), + LocationFactory.create(), "Wanted anywhere AFTER following interaction:", previous.toString(), previous.getLocation(), @@ -400,7 +400,7 @@ private static String createTooManyInvocationsMessage( return join( wanted.toString(), "Wanted " + pluralize(wantedCount) + ":", - new LocationImpl(), + LocationFactory.create(), "But was " + pluralize(actualCount) + ":", createAllLocationsMessage(invocations), ""); @@ -413,7 +413,7 @@ public static MockitoAssertionError neverWantedButInvoked( join( wanted.toString(), "Never wanted here:", - new LocationImpl(), + LocationFactory.create(), "But invoked here:", createAllLocationsArgsMessage(invocations))); } @@ -463,7 +463,7 @@ private static String createTooFewInvocationsMessage( "Wanted " + discrepancy.getPluralizedWantedCount() + (discrepancy.getWantedCount() == 0 ? "." : ":"), - new LocationImpl(), + LocationFactory.create(), "But was " + discrepancy.getPluralizedActualCount() + (discrepancy.getActualCount() == 0 ? "." : ":"), @@ -496,7 +496,7 @@ public static MockitoAssertionError noMoreInteractionsWanted( return new NoInteractionsWanted( join( "No interactions wanted here:", - new LocationImpl(), + LocationFactory.create(), "But found this interaction on mock '" + MockUtil.getMockName(undesired.getMock()) + "':", @@ -508,7 +508,7 @@ public static MockitoAssertionError noMoreInteractionsWantedInOrder(Invocation u return new VerificationInOrderFailure( join( "No interactions wanted here:", - new LocationImpl(), + LocationFactory.create(), "But found this interaction on mock '" + MockUtil.getMockName(undesired.getMock()) + "':", @@ -527,7 +527,7 @@ public static MockitoAssertionError noInteractionsWanted( return new NoInteractionsWanted( join( "No interactions wanted here:", - new LocationImpl(), + LocationFactory.create(), "But found these interactions on mock '" + MockUtil.getMockName(mock) + "':", @@ -645,7 +645,7 @@ public static MockitoException smartNullPointerException(String invocation, Loca return new SmartNullPointerException( join( "You have a NullPointerException here:", - new LocationImpl(), + LocationFactory.create(), "because this method call was *not* stubbed correctly:", location, invocation, diff --git a/src/main/java/org/mockito/internal/exceptions/stacktrace/DefaultStackTraceCleaner.java b/src/main/java/org/mockito/internal/exceptions/stacktrace/DefaultStackTraceCleaner.java index 6b0575273e..6437ae4aa4 100644 --- a/src/main/java/org/mockito/internal/exceptions/stacktrace/DefaultStackTraceCleaner.java +++ b/src/main/java/org/mockito/internal/exceptions/stacktrace/DefaultStackTraceCleaner.java @@ -13,9 +13,18 @@ public class DefaultStackTraceCleaner implements StackTraceCleaner { @Override public boolean isIn(StackTraceElement e) { - if (isFromMockitoRunner(e.getClassName()) || isFromMockitoRule(e.getClassName())) { + return isIn(e.getClassName()); + } + + @Override + public boolean isIn(StackFrameMetadata e) { + return isIn(e.getClassName()); + } + + private boolean isIn(String className) { + if (isFromMockitoRunner(className) || isFromMockitoRule(className)) { return true; - } else if (isMockDispatcher(e.getClassName()) || isFromMockito(e.getClassName())) { + } else if (isMockDispatcher(className) || isFromMockito(className)) { return false; } else { return true; diff --git a/src/main/java/org/mockito/internal/invocation/DefaultInvocationFactory.java b/src/main/java/org/mockito/internal/invocation/DefaultInvocationFactory.java index 81f801518f..4921f4006d 100644 --- a/src/main/java/org/mockito/internal/invocation/DefaultInvocationFactory.java +++ b/src/main/java/org/mockito/internal/invocation/DefaultInvocationFactory.java @@ -8,7 +8,7 @@ import java.util.concurrent.Callable; import org.mockito.internal.creation.DelegatingMethod; -import org.mockito.internal.debugging.LocationImpl; +import org.mockito.internal.debugging.LocationFactory; import org.mockito.internal.invocation.mockref.MockWeakReference; import org.mockito.internal.progress.SequenceNumber; import org.mockito.invocation.Invocation; @@ -71,7 +71,7 @@ private static InterceptedInvocation createInvocation( RealMethod realMethod, MockCreationSettings settings) { return createInvocation( - mock, invokedMethod, arguments, realMethod, settings, new LocationImpl()); + mock, invokedMethod, arguments, realMethod, settings, LocationFactory.create()); } private static MockitoMethod createMockitoMethod(Method method, MockCreationSettings settings) { diff --git a/src/main/java/org/mockito/internal/matchers/LocalizedMatcher.java b/src/main/java/org/mockito/internal/matchers/LocalizedMatcher.java index 00b47de7a9..a54eb6d79e 100644 --- a/src/main/java/org/mockito/internal/matchers/LocalizedMatcher.java +++ b/src/main/java/org/mockito/internal/matchers/LocalizedMatcher.java @@ -5,7 +5,7 @@ package org.mockito.internal.matchers; import org.mockito.ArgumentMatcher; -import org.mockito.internal.debugging.LocationImpl; +import org.mockito.internal.debugging.LocationFactory; import org.mockito.invocation.Location; @SuppressWarnings("unchecked") @@ -16,7 +16,7 @@ public class LocalizedMatcher { public LocalizedMatcher(ArgumentMatcher matcher) { this.matcher = matcher; - this.location = new LocationImpl(); + this.location = LocationFactory.create(); } public Location getLocation() { diff --git a/src/main/java/org/mockito/internal/progress/MockingProgressImpl.java b/src/main/java/org/mockito/internal/progress/MockingProgressImpl.java index 991b5e4734..2585d32cfc 100644 --- a/src/main/java/org/mockito/internal/progress/MockingProgressImpl.java +++ b/src/main/java/org/mockito/internal/progress/MockingProgressImpl.java @@ -14,7 +14,7 @@ import org.mockito.internal.configuration.GlobalConfiguration; import org.mockito.internal.debugging.Localized; -import org.mockito.internal.debugging.LocationImpl; +import org.mockito.internal.debugging.LocationFactory; import org.mockito.internal.exceptions.Reporter; import org.mockito.internal.listeners.AutoCleanableListener; import org.mockito.invocation.Location; @@ -105,7 +105,7 @@ public VerificationMode pullVerificationMode() { @Override public void stubbingStarted() { validateState(); - stubbingInProgress = new LocationImpl(); + stubbingInProgress = LocationFactory.create(); } @Override diff --git a/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsSmartNulls.java b/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsSmartNulls.java index 2402f364a6..bcca2ef52a 100644 --- a/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsSmartNulls.java +++ b/src/main/java/org/mockito/internal/stubbing/defaultanswers/ReturnsSmartNulls.java @@ -10,7 +10,7 @@ import java.io.Serializable; import org.mockito.Mockito; -import org.mockito.internal.debugging.LocationImpl; +import org.mockito.internal.debugging.LocationFactory; import org.mockito.invocation.InvocationOnMock; import org.mockito.invocation.Location; import org.mockito.stubbing.Answer; @@ -57,7 +57,8 @@ public Object apply(Class type) { } return Mockito.mock( - type, new ThrowsSmartNullPointer(invocation, new LocationImpl())); + type, + new ThrowsSmartNullPointer(invocation, LocationFactory.create())); } }); } diff --git a/src/test/java/org/mockito/internal/invocation/InvocationBuilder.java b/src/test/java/org/mockito/internal/invocation/InvocationBuilder.java index 9900db9d98..d5cf3e5577 100644 --- a/src/test/java/org/mockito/internal/invocation/InvocationBuilder.java +++ b/src/test/java/org/mockito/internal/invocation/InvocationBuilder.java @@ -13,7 +13,7 @@ import java.util.List; import org.mockito.Mockito; -import org.mockito.internal.debugging.LocationImpl; +import org.mockito.internal.debugging.LocationFactory; import org.mockito.internal.invocation.mockref.MockReference; import org.mockito.internal.invocation.mockref.MockStrongReference; import org.mockito.invocation.Invocation; @@ -72,7 +72,7 @@ public Invocation toInvocation() { new SerializableMethod(method), args, NO_OP, - location == null ? new LocationImpl() : location, + location == null ? LocationFactory.create() : location, 1); if (verified) { i.markVerified(); diff --git a/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsSmartNullsTest.java b/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsSmartNullsTest.java index 984e07da2a..267c6d6966 100644 --- a/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsSmartNullsTest.java +++ b/src/test/java/org/mockito/internal/stubbing/defaultanswers/ReturnsSmartNullsTest.java @@ -20,7 +20,7 @@ import org.assertj.core.api.ThrowableAssert; import org.junit.Test; import org.mockito.exceptions.verification.SmartNullPointerException; -import org.mockito.internal.debugging.LocationImpl; +import org.mockito.internal.debugging.LocationFactory; import org.mockito.internal.invocation.InterceptedInvocation; import org.mockito.internal.invocation.SerializableMethod; import org.mockito.internal.invocation.mockref.MockStrongReference; @@ -146,7 +146,7 @@ private static InterceptedInvocation invocationMethodWithArgs(final T obj) GenericFooBar.class.getMethod("methodWithArgs", int.class, Object.class)), new Object[] {1, obj}, InterceptedInvocation.NO_OP, - new LocationImpl(), + LocationFactory.create(), 1); } @@ -269,7 +269,7 @@ private static InterceptedInvocation invocationMethodWithVarArgs(final T[] o "methodWithVarArgs", int.class, Object[].class)), new Object[] {1, obj}, InterceptedInvocation.NO_OP, - new LocationImpl(), + LocationFactory.create(), 1); } diff --git a/src/test/java/org/mockitousage/internal/debugging/LocationFactoryTest.java b/src/test/java/org/mockitousage/internal/debugging/LocationFactoryTest.java new file mode 100644 index 0000000000..95105e8a73 --- /dev/null +++ b/src/test/java/org/mockitousage/internal/debugging/LocationFactoryTest.java @@ -0,0 +1,100 @@ +/* + * Copyright (c) 2007 Mockito contributors + * This program is made available under the terms of the MIT License. + */ +package org.mockitousage.internal.debugging; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertEquals; + +import java.lang.management.ManagementFactory; +import java.util.ArrayList; +import java.util.List; +import java.util.stream.IntStream; + +import com.sun.management.ThreadMXBean; +import org.junit.Test; +import org.mockito.internal.debugging.LocationFactory; +import org.mockitoutil.TestBase; + +public class LocationFactoryTest extends TestBase { + private static final int REPEAT = 1000; + private static final int RECURSION_LIMIT = 1000; + private static final double EXPECTED_IMPROVEMENT = expectedImprovement(); + + private static final ThreadMXBean memoryBean = + (ThreadMXBean) ManagementFactory.getThreadMXBean(); + + @Test + public void shouldLocationNotContainGetStackTraceMethod() { + assertThat(LocationFactory.create().toString()) + .contains("shouldLocationNotContainGetStackTraceMethod"); + } + + @Test + public void provides_location_class() { + // when + final List files = new ArrayList(); + new Runnable() { // anonymous inner class adds stress to the check + public void run() { + files.add(LocationFactory.create().getSourceFile()); + } + }.run(); + + // then + assertEquals("LocationFactoryTest.java", files.get(0)); + } + + @Test + public void shouldAllocateMuchLessMemoryThanThrowable() { + // On Java 8, this will use the internal approach. On Java 9, the StackWalker approach will + // be used. + new Throwable().fillInStackTrace(); + LocationFactory.create(); + long baseline = + countMemoryAllocations( + () -> + recurseAndThen( + RECURSION_LIMIT, + repeat(() -> new Throwable().fillInStackTrace()))); + long actual = + countMemoryAllocations( + () -> + recurseAndThen( + RECURSION_LIMIT, + repeat(() -> LocationFactory.create(false)))); + assertThat(actual * EXPECTED_IMPROVEMENT) + .as( + "stack walker approach (%d) expected to be at least %fx better than exception approach (%d)", + actual, EXPECTED_IMPROVEMENT, baseline) + .isLessThan(baseline); + } + + private static long countMemoryAllocations(Runnable someTask) { + long threadId = Thread.currentThread().getId(); + long atStart = memoryBean.getThreadAllocatedBytes(threadId); + someTask.run(); + return memoryBean.getThreadAllocatedBytes(threadId) - atStart; + } + + private static void recurseAndThen(int count, Runnable runnable) { + if (count <= 0) { + runnable.run(); + } else { + recurseAndThen(count - 1, runnable); + } + } + + private static Runnable repeat(Runnable task) { + return () -> IntStream.range(0, REPEAT).forEach(index -> task.run()); + } + + private static double expectedImprovement() { + try { + Class.forName("java.lang.StackWalker"); + return 20; + } catch (ClassNotFoundException e) { + return 1.5; + } + } +} diff --git a/src/test/java/org/mockitousage/internal/debugging/LocationImplTest.java b/src/test/java/org/mockitousage/internal/debugging/LocationImplTest.java deleted file mode 100644 index d20bfce02a..0000000000 --- a/src/test/java/org/mockitousage/internal/debugging/LocationImplTest.java +++ /dev/null @@ -1,63 +0,0 @@ -/* - * Copyright (c) 2007 Mockito contributors - * This program is made available under the terms of the MIT License. - */ -package org.mockitousage.internal.debugging; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.Assert.assertEquals; - -import java.util.ArrayList; -import java.util.List; - -import org.junit.Test; -import org.mockito.internal.debugging.LocationImpl; -import org.mockito.internal.exceptions.stacktrace.StackTraceFilter; -import org.mockitoutil.TestBase; - -@SuppressWarnings("serial") -public class LocationImplTest extends TestBase { - - @Test - public void shouldLocationNotContainGetStackTraceMethod() { - assertThat(new LocationImpl().toString()) - .contains("shouldLocationNotContainGetStackTraceMethod"); - } - - @Test - public void shouldBeSafeInCaseForSomeReasonFilteredStackTraceIsEmpty() { - // given - StackTraceFilter filterReturningEmptyArray = - new StackTraceFilter() { - @Override - public StackTraceElement[] filter(StackTraceElement[] target, boolean keepTop) { - return new StackTraceElement[0]; - } - - @Override - public StackTraceElement filterFirst(Throwable target, boolean isInline) { - return null; - } - }; - - // when - String loc = new LocationImpl(filterReturningEmptyArray).toString(); - - // then - assertEquals("-> at <>", loc); - } - - @Test - public void provides_location_class() { - // when - final List files = new ArrayList(); - new Runnable() { // anonymous inner class adds stress to the check - public void run() { - files.add(new LocationImpl().getSourceFile()); - } - }.run(); - - // then - assertEquals("LocationImplTest.java", files.get(0)); - } -} diff --git a/src/test/java/org/mockitoutil/TestBase.java b/src/test/java/org/mockitoutil/TestBase.java index 2fe89504e4..3f772d020b 100644 --- a/src/test/java/org/mockitoutil/TestBase.java +++ b/src/test/java/org/mockitoutil/TestBase.java @@ -17,7 +17,7 @@ import org.mockito.StateMaster; import org.mockito.internal.MockitoCore; import org.mockito.internal.configuration.ConfigurationAccess; -import org.mockito.internal.debugging.LocationImpl; +import org.mockito.internal.debugging.LocationFactory; import org.mockito.internal.invocation.InterceptedInvocation; import org.mockito.internal.invocation.InvocationBuilder; import org.mockito.internal.invocation.InvocationMatcher; @@ -84,7 +84,7 @@ protected static Invocation invocationOf(Class type, String methodName, Objec new SerializableMethod(type.getMethod(methodName, types)), args, InterceptedInvocation.NO_OP, - new LocationImpl(), + LocationFactory.create(), 1); } From a5c573f914fa8531c7f4d51e12e35af01c171914 Mon Sep 17 00:00:00 2001 From: James Baker Date: Mon, 8 Aug 2022 10:44:46 +0100 Subject: [PATCH 2/7] Class loadability tests no longer fail on Java 8 --- .../ClassLoadabilityChecker.java | 71 +++++++++++++++++++ .../NoByteCodeDependenciesTest.java | 17 +---- .../NoJUnitDependenciesTest.java | 18 ++--- 3 files changed, 78 insertions(+), 28 deletions(-) create mode 100644 src/test/java/org/mockitointegration/ClassLoadabilityChecker.java diff --git a/src/test/java/org/mockitointegration/ClassLoadabilityChecker.java b/src/test/java/org/mockitointegration/ClassLoadabilityChecker.java new file mode 100644 index 0000000000..3b75689ca9 --- /dev/null +++ b/src/test/java/org/mockitointegration/ClassLoadabilityChecker.java @@ -0,0 +1,71 @@ +/* + * Copyright (c) 2017 Mockito contributors + * This program is made available under the terms of the MIT License. + */ +package org.mockitointegration; + +import java.util.HashSet; +import java.util.Set; + +/** + * Check that classes can be loaded and initialized on a provided classloader. Used + * for checking that Mockito has no dependency on libraries like JUnit. + *

+ * Some classes are excluded from this checking - namely, classes that fail due to + * the absence of Java classes. It's assumed that this is due to a specific optional + * dependency on APIs available in certain Java versions and so other elements of the + * test Matrix will check that those classes do not depend on JUnit or ByteBuddy. We + * exclude based on the failure of a ClassNotFoundException, or a NoClassDefFoundError + * caused by the failing to load of a failing parent class. + */ +public final class ClassLoadabilityChecker { + private static final boolean INITIALIZE_CLASSES = true; + private final Set excludedClasses = new HashSet<>(); + private final ClassLoader classLoader; + private final String purpose; + + public ClassLoadabilityChecker(ClassLoader classLoader, String purpose) { + this.classLoader = classLoader; + this.purpose = purpose; + } + + public void checkLoadability(String className) { + try { + Class.forName(className, INITIALIZE_CLASSES, classLoader); + } catch (ClassNotFoundException | NoClassDefFoundError e) { + if (isFailureExcluded(className, e)) { + return; + } + e.printStackTrace(); + throw new AssertionError( + String.format("'%s' has some dependency to %s", + className, purpose)); + } + } + + private boolean isFailureExcluded(String loadedClass, Throwable thrown) { + if (thrown == null) { + return false; + } + if (thrown instanceof ClassNotFoundException) { + ClassNotFoundException cnf = (ClassNotFoundException) thrown; + if (cnf.getMessage().startsWith("java.")) { + excludedClasses.add(loadedClass); + return true; + } + } else if (thrown instanceof NoClassDefFoundError) { + NoClassDefFoundError ncdf = (NoClassDefFoundError) thrown; + // if Foo fails due to depending on a Java class, Foo$Bar will fail with a NCDFE + int lastInnerClass = loadedClass.lastIndexOf('$'); + if (lastInnerClass != -1) { + String parent = loadedClass.substring(0, lastInnerClass); + if (excludedClasses.contains(parent) && ncdf.getMessage().contains(parent)) { + excludedClasses.add(loadedClass); + return true; + } + } + } + + return isFailureExcluded(loadedClass, thrown.getCause()); + } +} diff --git a/src/test/java/org/mockitointegration/NoByteCodeDependenciesTest.java b/src/test/java/org/mockitointegration/NoByteCodeDependenciesTest.java index 3a2908dc1c..1f5a4dc195 100644 --- a/src/test/java/org/mockitointegration/NoByteCodeDependenciesTest.java +++ b/src/test/java/org/mockitointegration/NoByteCodeDependenciesTest.java @@ -43,21 +43,10 @@ public void pure_mockito_should_not_depend_bytecode_libraries() throws Exception pureMockitoAPIClasses.remove( "org.mockito.internal.util.reflection.InstrumentationMemberAccessor"); + ClassLoadabilityChecker checker = new ClassLoadabilityChecker( + classLoader_without_bytecode_libraries, "ByteBuddy or Objenesis"); for (String pureMockitoAPIClass : pureMockitoAPIClasses) { - checkDependency(classLoader_without_bytecode_libraries, pureMockitoAPIClass); - } - } - - private void checkDependency(ClassLoader classLoader, String pureMockitoAPIClass) - throws ClassNotFoundException { - try { - Class.forName(pureMockitoAPIClass, true, classLoader); - } catch (Throwable e) { - e.printStackTrace(); - throw new AssertionError( - String.format( - "'%s' has some dependency to Byte Buddy or Objenesis", - pureMockitoAPIClass)); + checker.checkLoadability(pureMockitoAPIClass); } } } diff --git a/src/test/java/org/mockitointegration/NoJUnitDependenciesTest.java b/src/test/java/org/mockitointegration/NoJUnitDependenciesTest.java index 7b156f0aa3..8798b09ccb 100644 --- a/src/test/java/org/mockitointegration/NoJUnitDependenciesTest.java +++ b/src/test/java/org/mockitointegration/NoJUnitDependenciesTest.java @@ -42,27 +42,17 @@ public void pure_mockito_should_not_depend_JUnit___ByteBuddy() throws Exception .omit("runners", "junit", "JUnit", "opentest4j") .listOwnedClasses(); + ClassLoadabilityChecker checker = new ClassLoadabilityChecker(classLoader_without_JUnit, "JUnit"); + // The later class is required to be initialized before any inline mock maker classes can be // loaded. - checkDependency( - classLoader_without_JUnit, + checker.checkLoadability( "org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker"); pureMockitoAPIClasses.remove( "org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker"); for (String pureMockitoAPIClass : pureMockitoAPIClasses) { - checkDependency(classLoader_without_JUnit, pureMockitoAPIClass); - } - } - - private void checkDependency(ClassLoader classLoader_without_JUnit, String pureMockitoAPIClass) - throws ClassNotFoundException { - try { - Class.forName(pureMockitoAPIClass, true, classLoader_without_JUnit); - } catch (Throwable e) { - e.printStackTrace(); - throw new AssertionError( - String.format("'%s' has some dependency to JUnit", pureMockitoAPIClass)); + checker.checkLoadability(pureMockitoAPIClass); } } } From 46b4033b49afac078b2ac5fd8787aa32d829debe Mon Sep 17 00:00:00 2001 From: James Baker Date: Sat, 13 Aug 2022 14:55:37 +0100 Subject: [PATCH 3/7] PR comments --- .../debugging/Java9PlusLocationImpl.java | 72 +++++++++++++++---- .../internal/debugging/LocationFactory.java | 44 +++--------- .../ClassLoadabilityChecker.java | 5 +- .../NoByteCodeDependenciesTest.java | 5 +- .../NoJUnitDependenciesTest.java | 3 +- 5 files changed, 74 insertions(+), 55 deletions(-) diff --git a/src/main/java/org/mockito/internal/debugging/Java9PlusLocationImpl.java b/src/main/java/org/mockito/internal/debugging/Java9PlusLocationImpl.java index 27b02c3d31..219835513e 100644 --- a/src/main/java/org/mockito/internal/debugging/Java9PlusLocationImpl.java +++ b/src/main/java/org/mockito/internal/debugging/Java9PlusLocationImpl.java @@ -4,6 +4,7 @@ */ package org.mockito.internal.debugging; +import org.mockito.exceptions.base.MockitoException; import org.mockito.exceptions.stacktrace.StackTraceCleaner; import org.mockito.exceptions.stacktrace.StackTraceCleaner.StackFrameMetadata; import org.mockito.internal.configuration.plugins.Plugins; @@ -23,6 +24,11 @@ class Java9PlusLocationImpl implements Location, Serializable { private static final long serialVersionUID = 2954388321980069195L; + + private static final String UNEXPECTED_ERROR_SUFFIX = + "\nThis is unexpected and is likely due to a change in either Java's StackWalker or Reflection APIs." + + "\nIt's worth trying to upgrade to a newer version of Mockito, or otherwise to file a bug report."; + private static final String STACK_WALKER = "java.lang.StackWalker"; private static final String STACK_FRAME = STACK_WALKER + "$StackFrame"; private static final String OPTION = STACK_WALKER + "$Option"; @@ -50,8 +56,15 @@ class Java9PlusLocationImpl implements Location, Serializable { Plugins.getStackTraceCleanerProvider() .getStackTraceCleaner(new DefaultStackTraceCleaner()); + /** + * In Java, allocating lambdas is cheap, but not free. stream.map(this::doSomething) + * will allocate a Function object each time the function is called (although not + * per element). By assigning these Functions and Predicates to variables, we can + * avoid the memory allocation. + */ private static final Function toStackFrameMetadata = MetadataShim::new; + private static final Predicate cleanerIsIn = CLEANER::isIn; private static final int FRAMES_TO_SKIP = framesToSkip(); @@ -70,22 +83,18 @@ public String getSourceFile() { @Override public String toString() { - maybeInitializeStackTraceLine(); - return stackTraceLine; + return stackTraceLine(); } - private void maybeInitializeStackTraceLine() { + private String stackTraceLine() { if (stackTraceLine == null) { synchronized (this) { if (stackTraceLine == null) { - stackTraceLine = stackTraceLine(sfm); + stackTraceLine = PREFIX + sfm.toString(); } } } - } - - private static String stackTraceLine(StackFrameMetadata element) { - return PREFIX + element.toString(); + return stackTraceLine; } private static StackFrameMetadata getStackFrame(boolean isInline) { @@ -97,9 +106,24 @@ private static StackFrameMetadata getStackFrame(boolean isInline) { .skip(isInline ? 1 : 0) .findFirst() .orElseThrow( - () -> - new IllegalArgumentException( - "Mockito could not find the first non-Mockito stack frame"))); + () -> new MockitoException(noStackTraceFailureMessage()))); + } + + private static boolean usingDefaultStackTraceCleaner() { + return CLEANER instanceof DefaultStackTraceCleaner; + } + + private static String noStackTraceFailureMessage() { + if (usingDefaultStackTraceCleaner()) { + return "Mockito could not find the first non-Mockito stack frame." + + UNEXPECTED_ERROR_SUFFIX; + } else { + String cleanerType = CLEANER.getClass().getName(); + String fmt = + "Mockito could not find the first non-Mockito stack frame. A custom stack frame cleaner \n" + + "(type %s) is in use and this has mostly likely filtered out all the relevant stack frames."; + return String.format(fmt, cleanerType); + } } /** @@ -121,8 +145,24 @@ private static int framesToSkip() { private static T stackWalk(Function, T> function) { try { return (T) walk.invoke(stackWalker, function); - } catch (InvocationTargetException | IllegalAccessException e) { - throw new RuntimeException(e); + } catch (IllegalAccessException e) { + throw new MockitoException( + "Unexpected access exception while stack walking." + UNEXPECTED_ERROR_SUFFIX, + e); + } catch (InvocationTargetException e) { + throw new MockitoException(stackWalkFailureMessage()); + } + } + + private static String stackWalkFailureMessage() { + if (usingDefaultStackTraceCleaner()) { + return "Caught an unexpected exception while stack walking." + UNEXPECTED_ERROR_SUFFIX; + } else { + String className = CLEANER.getClass().getName(); + String fmt = + "Caught an unexpected exception while stack walking." + + "\nThis is likely caused by the custom stack trace cleaner in use (class %s)."; + return String.format(fmt, className); } } @@ -150,8 +190,10 @@ private static Object stackWalker() { Method getInstance = stackWalkerClazz.getDeclaredMethod("getInstance", Set.class, int.class); return getInstance.invoke(null, options, BUFFER_SIZE); - } catch (InvocationTargetException | NoSuchMethodException | IllegalAccessException e) { - throw new RuntimeException(e); + } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) { + throw new MockitoException( + "Mockito received an exception while trying to acquire a StackWalker." + + UNEXPECTED_ERROR_SUFFIX); } } diff --git a/src/main/java/org/mockito/internal/debugging/LocationFactory.java b/src/main/java/org/mockito/internal/debugging/LocationFactory.java index 19dd9ecaf2..3ad2c910d9 100644 --- a/src/main/java/org/mockito/internal/debugging/LocationFactory.java +++ b/src/main/java/org/mockito/internal/debugging/LocationFactory.java @@ -9,23 +9,7 @@ import java.io.Serializable; public final class LocationFactory { - private static final String PROPERTY = "mockito.locationFactory.disable"; - private static final Factory factory; - - static { - Factory lFactory; - if (Boolean.getBoolean(PROPERTY)) { - factory = unused -> NoLocation.INSTANCE; - } else { - try { - Class.forName("java.lang.StackWalker"); - lFactory = new Java9PlusLocationFactory(); - } catch (ClassNotFoundException e) { - lFactory = new Java8LocationFactory(); - } - factory = lFactory; - } - } + private static final Factory factory = createLocationFactory(); private LocationFactory() {} @@ -41,6 +25,15 @@ private interface Factory { Location create(boolean inline); } + private static Factory createLocationFactory() { + try { + Class.forName("java.lang.StackWalker"); + return new Java9PlusLocationFactory(); + } catch (ClassNotFoundException e) { + return new Java8LocationFactory(); + } + } + private static final class Java8LocationFactory implements Factory { @Override public Location create(boolean inline) { @@ -55,21 +48,4 @@ public Location create(boolean inline) { return new Java9PlusLocationImpl(inline); } } - - private static final class NoLocation implements Location, Serializable { - private static final long serialVersionUID = -3807068467458099012L; - private static final NoLocation INSTANCE = new NoLocation(); - - private NoLocation() {} - - @Override - public String getSourceFile() { - return ""; - } - - @Override - public String toString() { - return "-> at <>"; - } - } } diff --git a/src/test/java/org/mockitointegration/ClassLoadabilityChecker.java b/src/test/java/org/mockitointegration/ClassLoadabilityChecker.java index 3b75689ca9..5f02232c61 100644 --- a/src/test/java/org/mockitointegration/ClassLoadabilityChecker.java +++ b/src/test/java/org/mockitointegration/ClassLoadabilityChecker.java @@ -31,15 +31,14 @@ public ClassLoadabilityChecker(ClassLoader classLoader, String purpose) { public void checkLoadability(String className) { try { - Class.forName(className, INITIALIZE_CLASSES, classLoader); + Class.forName(className, INITIALIZE_CLASSES, classLoader); } catch (ClassNotFoundException | NoClassDefFoundError e) { if (isFailureExcluded(className, e)) { return; } e.printStackTrace(); throw new AssertionError( - String.format("'%s' has some dependency to %s", - className, purpose)); + String.format("'%s' has some dependency to %s", className, purpose)); } } diff --git a/src/test/java/org/mockitointegration/NoByteCodeDependenciesTest.java b/src/test/java/org/mockitointegration/NoByteCodeDependenciesTest.java index 1f5a4dc195..3db1395066 100644 --- a/src/test/java/org/mockitointegration/NoByteCodeDependenciesTest.java +++ b/src/test/java/org/mockitointegration/NoByteCodeDependenciesTest.java @@ -43,8 +43,9 @@ public void pure_mockito_should_not_depend_bytecode_libraries() throws Exception pureMockitoAPIClasses.remove( "org.mockito.internal.util.reflection.InstrumentationMemberAccessor"); - ClassLoadabilityChecker checker = new ClassLoadabilityChecker( - classLoader_without_bytecode_libraries, "ByteBuddy or Objenesis"); + ClassLoadabilityChecker checker = + new ClassLoadabilityChecker( + classLoader_without_bytecode_libraries, "ByteBuddy or Objenesis"); for (String pureMockitoAPIClass : pureMockitoAPIClasses) { checker.checkLoadability(pureMockitoAPIClass); } diff --git a/src/test/java/org/mockitointegration/NoJUnitDependenciesTest.java b/src/test/java/org/mockitointegration/NoJUnitDependenciesTest.java index 8798b09ccb..503d859617 100644 --- a/src/test/java/org/mockitointegration/NoJUnitDependenciesTest.java +++ b/src/test/java/org/mockitointegration/NoJUnitDependenciesTest.java @@ -42,7 +42,8 @@ public void pure_mockito_should_not_depend_JUnit___ByteBuddy() throws Exception .omit("runners", "junit", "JUnit", "opentest4j") .listOwnedClasses(); - ClassLoadabilityChecker checker = new ClassLoadabilityChecker(classLoader_without_JUnit, "JUnit"); + ClassLoadabilityChecker checker = + new ClassLoadabilityChecker(classLoader_without_JUnit, "JUnit"); // The later class is required to be initialized before any inline mock maker classes can be // loaded. From b403d8c015cf8af36d775f36ab2008721d120272 Mon Sep 17 00:00:00 2001 From: James Baker Date: Sat, 13 Aug 2022 15:39:01 +0100 Subject: [PATCH 4/7] Refactor test organization --- .../debugging/LocationFactoryTest.java | 59 --------------- .../LocationFactoryAllocationRateTest.java | 73 +++++++++++++++++++ ...emoryOnLargeStackTraceInvocationsTest.java | 9 +-- 3 files changed, 75 insertions(+), 66 deletions(-) create mode 100644 subprojects/memory-test/src/test/java/org/mockito/memorytest/LocationFactoryAllocationRateTest.java diff --git a/src/test/java/org/mockitousage/internal/debugging/LocationFactoryTest.java b/src/test/java/org/mockitousage/internal/debugging/LocationFactoryTest.java index 95105e8a73..7ae0614df9 100644 --- a/src/test/java/org/mockitousage/internal/debugging/LocationFactoryTest.java +++ b/src/test/java/org/mockitousage/internal/debugging/LocationFactoryTest.java @@ -18,12 +18,6 @@ import org.mockitoutil.TestBase; public class LocationFactoryTest extends TestBase { - private static final int REPEAT = 1000; - private static final int RECURSION_LIMIT = 1000; - private static final double EXPECTED_IMPROVEMENT = expectedImprovement(); - - private static final ThreadMXBean memoryBean = - (ThreadMXBean) ManagementFactory.getThreadMXBean(); @Test public void shouldLocationNotContainGetStackTraceMethod() { @@ -44,57 +38,4 @@ public void run() { // then assertEquals("LocationFactoryTest.java", files.get(0)); } - - @Test - public void shouldAllocateMuchLessMemoryThanThrowable() { - // On Java 8, this will use the internal approach. On Java 9, the StackWalker approach will - // be used. - new Throwable().fillInStackTrace(); - LocationFactory.create(); - long baseline = - countMemoryAllocations( - () -> - recurseAndThen( - RECURSION_LIMIT, - repeat(() -> new Throwable().fillInStackTrace()))); - long actual = - countMemoryAllocations( - () -> - recurseAndThen( - RECURSION_LIMIT, - repeat(() -> LocationFactory.create(false)))); - assertThat(actual * EXPECTED_IMPROVEMENT) - .as( - "stack walker approach (%d) expected to be at least %fx better than exception approach (%d)", - actual, EXPECTED_IMPROVEMENT, baseline) - .isLessThan(baseline); - } - - private static long countMemoryAllocations(Runnable someTask) { - long threadId = Thread.currentThread().getId(); - long atStart = memoryBean.getThreadAllocatedBytes(threadId); - someTask.run(); - return memoryBean.getThreadAllocatedBytes(threadId) - atStart; - } - - private static void recurseAndThen(int count, Runnable runnable) { - if (count <= 0) { - runnable.run(); - } else { - recurseAndThen(count - 1, runnable); - } - } - - private static Runnable repeat(Runnable task) { - return () -> IntStream.range(0, REPEAT).forEach(index -> task.run()); - } - - private static double expectedImprovement() { - try { - Class.forName("java.lang.StackWalker"); - return 20; - } catch (ClassNotFoundException e) { - return 1.5; - } - } } diff --git a/subprojects/memory-test/src/test/java/org/mockito/memorytest/LocationFactoryAllocationRateTest.java b/subprojects/memory-test/src/test/java/org/mockito/memorytest/LocationFactoryAllocationRateTest.java new file mode 100644 index 0000000000..45baaacd84 --- /dev/null +++ b/subprojects/memory-test/src/test/java/org/mockito/memorytest/LocationFactoryAllocationRateTest.java @@ -0,0 +1,73 @@ +package org.mockito.memorytest; + +import com.sun.management.ThreadMXBean; +import org.junit.Test; +import org.mockito.internal.debugging.LocationFactory; + +import java.lang.management.ManagementFactory; +import java.util.stream.IntStream; + +import static org.assertj.core.api.Assertions.assertThat; + +public class LocationFactoryAllocationRateTest { + private static final int REPEAT = 1000; + private static final int RECURSION_LIMIT = 1000; + private static final double EXPECTED_IMPROVEMENT = expectedImprovement(); + + private static final ThreadMXBean memoryBean = + (ThreadMXBean) ManagementFactory.getThreadMXBean(); + + + @Test + public void shouldAllocateMuchLessMemoryThanThrowable() { + // On Java 8, this will use the internal approach. On Java 9, the StackWalker approach will + // be used. + new Throwable().fillInStackTrace(); + LocationFactory.create(); + long baseline = + countMemoryAllocations( + () -> + recurseAndThen( + RECURSION_LIMIT, + repeat(() -> new Throwable().fillInStackTrace()))); + long actual = + countMemoryAllocations( + () -> + recurseAndThen( + RECURSION_LIMIT, + repeat(() -> LocationFactory.create(false)))); + assertThat(actual * EXPECTED_IMPROVEMENT) + .as( + "stack walker approach (%d) expected to be at least %fx better than exception approach (%d)", + actual, EXPECTED_IMPROVEMENT, baseline) + .isLessThan(baseline); + } + + private static long countMemoryAllocations(Runnable someTask) { + long threadId = Thread.currentThread().getId(); + long atStart = memoryBean.getThreadAllocatedBytes(threadId); + someTask.run(); + return memoryBean.getThreadAllocatedBytes(threadId) - atStart; + } + + private static void recurseAndThen(int count, Runnable runnable) { + if (count <= 0) { + runnable.run(); + } else { + recurseAndThen(count - 1, runnable); + } + } + + private static Runnable repeat(Runnable task) { + return () -> IntStream.range(0, REPEAT).forEach(index -> task.run()); + } + + private static double expectedImprovement() { + try { + Class.forName("java.lang.StackWalker"); + return 20; + } catch (ClassNotFoundException e) { + return 1.5; + } + } +} diff --git a/subprojects/memory-test/src/test/java/org/mockito/memorytest/ShouldNotStarveMemoryOnLargeStackTraceInvocationsTest.java b/subprojects/memory-test/src/test/java/org/mockito/memorytest/ShouldNotStarveMemoryOnLargeStackTraceInvocationsTest.java index d3a6903e21..80af6ff7f2 100644 --- a/subprojects/memory-test/src/test/java/org/mockito/memorytest/ShouldNotStarveMemoryOnLargeStackTraceInvocationsTest.java +++ b/subprojects/memory-test/src/test/java/org/mockito/memorytest/ShouldNotStarveMemoryOnLargeStackTraceInvocationsTest.java @@ -5,6 +5,7 @@ package org.mockito.memorytest; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assumptions.assumeThat; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -13,7 +14,6 @@ import org.junit.Ignore; import org.junit.Test; -@Ignore("https://github.com/mockito/mockito/issues/2478") public class ShouldNotStarveMemoryOnLargeStackTraceInvocationsTest { private static final int STACK_TRACE_DEPTH = 1000; @@ -23,12 +23,7 @@ public class ShouldNotStarveMemoryOnLargeStackTraceInvocationsTest { static { try { - Class.forName("sun.misc.SharedSecrets") - .getMethod("getJavaLangAccess") - .invoke(null); - Class.forName("sun.misc.JavaLangAccess") - .getMethod("getStackTraceElement", Throwable.class, int.class); - + Class.forName("java.lang.StackWalker"); supported = true; } catch (Exception ignored) { } From 8dc8978d49f8f649b3a43f8cd4af785b4d020dde Mon Sep 17 00:00:00 2001 From: James Baker Date: Sat, 13 Aug 2022 15:41:55 +0100 Subject: [PATCH 5/7] ./gradlew check works now --- .../mockito/internal/debugging/LocationFactory.java | 2 -- .../internal/debugging/LocationFactoryTest.java | 13 +++++-------- .../LocationFactoryAllocationRateTest.java | 4 ++++ ...tarveMemoryOnLargeStackTraceInvocationsTest.java | 2 -- 4 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/mockito/internal/debugging/LocationFactory.java b/src/main/java/org/mockito/internal/debugging/LocationFactory.java index 3ad2c910d9..daafddedaf 100644 --- a/src/main/java/org/mockito/internal/debugging/LocationFactory.java +++ b/src/main/java/org/mockito/internal/debugging/LocationFactory.java @@ -6,8 +6,6 @@ import org.mockito.invocation.Location; -import java.io.Serializable; - public final class LocationFactory { private static final Factory factory = createLocationFactory(); diff --git a/src/test/java/org/mockitousage/internal/debugging/LocationFactoryTest.java b/src/test/java/org/mockitousage/internal/debugging/LocationFactoryTest.java index 7ae0614df9..072c13fc02 100644 --- a/src/test/java/org/mockitousage/internal/debugging/LocationFactoryTest.java +++ b/src/test/java/org/mockitousage/internal/debugging/LocationFactoryTest.java @@ -4,18 +4,15 @@ */ package org.mockitousage.internal.debugging; -import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.Assert.assertEquals; +import org.junit.Test; +import org.mockito.internal.debugging.LocationFactory; +import org.mockitoutil.TestBase; -import java.lang.management.ManagementFactory; import java.util.ArrayList; import java.util.List; -import java.util.stream.IntStream; -import com.sun.management.ThreadMXBean; -import org.junit.Test; -import org.mockito.internal.debugging.LocationFactory; -import org.mockitoutil.TestBase; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertEquals; public class LocationFactoryTest extends TestBase { diff --git a/subprojects/memory-test/src/test/java/org/mockito/memorytest/LocationFactoryAllocationRateTest.java b/subprojects/memory-test/src/test/java/org/mockito/memorytest/LocationFactoryAllocationRateTest.java index 45baaacd84..26b36ca9af 100644 --- a/subprojects/memory-test/src/test/java/org/mockito/memorytest/LocationFactoryAllocationRateTest.java +++ b/subprojects/memory-test/src/test/java/org/mockito/memorytest/LocationFactoryAllocationRateTest.java @@ -1,3 +1,7 @@ +/* + * Copyright (c) 2022 Mockito contributors + * This program is made available under the terms of the MIT License. + */ package org.mockito.memorytest; import com.sun.management.ThreadMXBean; diff --git a/subprojects/memory-test/src/test/java/org/mockito/memorytest/ShouldNotStarveMemoryOnLargeStackTraceInvocationsTest.java b/subprojects/memory-test/src/test/java/org/mockito/memorytest/ShouldNotStarveMemoryOnLargeStackTraceInvocationsTest.java index 80af6ff7f2..5c8bac2630 100644 --- a/subprojects/memory-test/src/test/java/org/mockito/memorytest/ShouldNotStarveMemoryOnLargeStackTraceInvocationsTest.java +++ b/subprojects/memory-test/src/test/java/org/mockito/memorytest/ShouldNotStarveMemoryOnLargeStackTraceInvocationsTest.java @@ -5,13 +5,11 @@ package org.mockito.memorytest; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assumptions.assumeThat; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import org.junit.Assume; -import org.junit.Ignore; import org.junit.Test; public class ShouldNotStarveMemoryOnLargeStackTraceInvocationsTest { From 60fd2cb414fde279d9900a2d59b82887172943ff Mon Sep 17 00:00:00 2001 From: James Baker Date: Sat, 13 Aug 2022 16:23:16 +0100 Subject: [PATCH 6/7] Linkage Error --- .../java/org/mockitointegration/ClassLoadabilityChecker.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/mockitointegration/ClassLoadabilityChecker.java b/src/test/java/org/mockitointegration/ClassLoadabilityChecker.java index 5f02232c61..d19c51fb74 100644 --- a/src/test/java/org/mockitointegration/ClassLoadabilityChecker.java +++ b/src/test/java/org/mockitointegration/ClassLoadabilityChecker.java @@ -32,7 +32,7 @@ public ClassLoadabilityChecker(ClassLoader classLoader, String purpose) { public void checkLoadability(String className) { try { Class.forName(className, INITIALIZE_CLASSES, classLoader); - } catch (ClassNotFoundException | NoClassDefFoundError e) { + } catch (ClassNotFoundException | LinkageError e) { if (isFailureExcluded(className, e)) { return; } From af8454e5e5decc771e122e6fb77ff08136db7c65 Mon Sep 17 00:00:00 2001 From: James Baker Date: Sat, 13 Aug 2022 16:44:39 +0100 Subject: [PATCH 7/7] Remove unused constructors --- .../org/mockito/internal/debugging/Java8LocationImpl.java | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/main/java/org/mockito/internal/debugging/Java8LocationImpl.java b/src/main/java/org/mockito/internal/debugging/Java8LocationImpl.java index 3cbffe42be..e8ee387c0a 100644 --- a/src/main/java/org/mockito/internal/debugging/Java8LocationImpl.java +++ b/src/main/java/org/mockito/internal/debugging/Java8LocationImpl.java @@ -18,18 +18,10 @@ class Java8LocationImpl implements Location, Serializable { private String stackTraceLine; private String sourceFile; - public Java8LocationImpl() { - this(new Throwable(), false); - } - public Java8LocationImpl(Throwable stackTraceHolder, boolean isInline) { this(stackTraceFilter, stackTraceHolder, isInline); } - public Java8LocationImpl(StackTraceFilter stackTraceFilter) { - this(stackTraceFilter, new Throwable(), false); - } - private Java8LocationImpl( StackTraceFilter stackTraceFilter, Throwable stackTraceHolder, boolean isInline) { computeStackTraceInformation(stackTraceFilter, stackTraceHolder, isInline);