Skip to content

Commit

Permalink
Excessive locking in TypeCachingBytecodeGenerator#BOOTSTRAP_LOCK
Browse files Browse the repository at this point in the history
Changed locking scheme in TypeCachingBytecodeGenerator from a
single global lock to mockType class lock.
But the mockType class lock will still ensure that a MockitoMockKey
will never be created twice.

Fixes #3035
  • Loading branch information
AndreasTu committed Aug 17, 2023
1 parent cb75cec commit c054b25
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 5 deletions.
Expand Up @@ -15,8 +15,6 @@
class TypeCachingBytecodeGenerator extends ReferenceQueue<ClassLoader>
implements BytecodeGenerator {

private static final Object BOOTSTRAP_LOCK = new Object();

private final BytecodeGenerator bytecodeGenerator;

private final TypeCache<MockitoMockKey> typeCache;
Expand All @@ -35,17 +33,27 @@ public TypeCachingBytecodeGenerator(BytecodeGenerator bytecodeGenerator, boolean
public <T> Class<T> mockClass(final MockFeatures<T> params) {
lock.readLock().lock();
try {
ClassLoader classLoader = params.mockedType.getClassLoader();
Class<T> mockedType = params.mockedType;
ClassLoader classLoader = mockedType.getClassLoader();
return (Class<T>)
typeCache.findOrInsert(
classLoader,
new MockitoMockKey(
params.mockedType,
mockedType,
params.interfaces,
params.serializableMode,
params.stripAnnotations),
() -> bytecodeGenerator.mockClass(params),
BOOTSTRAP_LOCK);
/*
* Only lock on the main mockType, instead of a global lock.
* But the mockType class lock, will still ensure that a MockitoMockKey will never be created twice.
*
* #3035: Excessive locking in TypeCachingBytecodeGenerator#BOOTSTRAP_LOCK
*
* Note: This lock is still a bit more than needed, if e.g. the same type is mocked with
* and without interfaces at the same time. But this can be neglected for simplicity.
*/
mockedType);
} catch (IllegalArgumentException exception) {
Throwable cause = exception.getCause();
if (cause instanceof RuntimeException) {
Expand Down
Expand Up @@ -5,6 +5,9 @@
package org.mockito.internal.creation.bytebuddy;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.mockito.internal.creation.bytebuddy.MockFeatures.withMockFeatures;
import static org.mockitoutil.ClassLoaders.inMemoryClassLoader;
import static org.mockitoutil.SimpleClassGenerator.makeMarkerInterface;
Expand Down Expand Up @@ -208,6 +211,36 @@ public void ensure_cache_returns_same_instance_defaultAnswer() throws Exception
assertThat(cache).isEmpty();
}

@Test
public void validate_TypeCache_locks_on_MockClass_Issue3035() {
BytecodeGenerator bytecodeGeneratorMock = mock();
TypeCachingBytecodeGenerator cachingMockBytecodeGenerator =
new TypeCachingBytecodeGenerator(bytecodeGeneratorMock, true);

MockFeatures<TestInterface> mockFeatures =
withMockFeatures(
TestInterface.class,
Collections.emptySet(),
SerializableMode.NONE,
false,
Answers.RETURNS_DEFAULTS);

when(bytecodeGeneratorMock.mockClass(mockFeatures))
.thenAnswer(
(invocation) -> {
assertThat(Thread.holdsLock(TestInterface.class))
.overridingErrorMessage(
"Lock for TestInterface was not held by TypeCachingBytecodeGenerator")
.isTrue();
return null;
});

cachingMockBytecodeGenerator.mockClass(mockFeatures);
verify(bytecodeGeneratorMock).mockClass(mockFeatures);
}

interface TestInterface {}

static class HoldingAReference {
final WeakReference<Class<?>> a;

Expand Down

0 comments on commit c054b25

Please sign in to comment.