Use Lookup class loading strategy if available #895
Conversation
In order to create mock objects on Java 11, `ProxyBasedMockFactory` now uses ByteBuddy's `Lookup`-based `ClassLoadingStrategy`, if available. Resolves #843.
| * Serves as a reference point for method handle based class loading and is makes the containing package | ||
| * non-empty so generated classes can be legally defined inside of it. | ||
| */ | ||
| public class CodegenDummy { |
There was a problem hiding this comment.
How about just Target?
spock-core/src/main/java/org/spockframework/mock/runtime/ProxyBasedMockFactory.java
Show resolved
Hide resolved
| @@ -89,6 +93,9 @@ private Object createDynamicProxyMock(Class<?> mockType, List<Class<?>> addition | |||
|
|
|||
| private static final TypeCache<TypeCache.SimpleKey> CACHE = | |||
| new TypeCache.WithInlineExpunction<>(TypeCache.Sort.SOFT); | |||
| private static final Random RANDOM = new Random(); | |||
There was a problem hiding this comment.
hm Random is not thread-safe IIRC that about ThreadLocalRandom?
There was a problem hiding this comment.
Instances of java.util.Random are threadsafe. However, the concurrent use of the same java.util.Random instance across threads may encounter contention and consequent poor performance. Consider instead using java.util.concurrent.ThreadLocalRandom in multithreaded designs.
So it's thread-safe but may have poor performance... I'll change it to ThreadLocalRandom.
| if (ClassInjector.UsingReflection.isAvailable()) { | ||
| return ClassLoadingStrategy.Default.INJECTION; | ||
| } | ||
| throw new IllegalStateException("No class loading strategy available"); |
There was a problem hiding this comment.
Shouldn't we fall back to WRAPPER
There was a problem hiding this comment.
In which cases would that get used?
There was a problem hiding this comment.
If a pre Java-9 JVM does not support Unsafe but I would not know any.
|
The code should include a notice of origin pointing to Mockito What about the classloader check https://github.com/mockito/mockito/blob/4f72147c464c1a8a642d01fc3334e98e92b464cd/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassInjectionLoader.java#L75 don't we need this also? |
|
@raphw maybe you could also take a look |
Codecov Report
@@ Coverage Diff @@
## master #895 +/- ##
============================================
- Coverage 75.05% 75.02% -0.04%
+ Complexity 3427 3426 -1
============================================
Files 368 368
Lines 10552 10579 +27
Branches 1333 1342 +9
============================================
+ Hits 7920 7937 +17
- Misses 2165 2168 +3
- Partials 467 474 +7
Continue to review full report at Codecov.
|
|
Mockito sometimes creates class loaders if a mock is loaded by a class loader that cannot see Mockito. This is a problem especially in OSGi environments, therefore the indirection and the check. |
|
Looks good otherwise! |
|
@leonard84 PTAL! 🙂 |
In order to create mock objects on Java 11+, `ProxyBasedMockFactory` now uses ByteBuddy's `Lookup`-based `ClassLoadingStrategy`, if `Reflection` strategy is not available. Resolves #843
In order to create mock objects on Java 11,
ProxyBasedMockFactorynow uses ByteBuddy'sLookup-basedClassLoadingStrategy, if available.Resolves #843.