DATACMNS-1210 - Fix concurrency issue in BasicPersitentEntity.

Make sure to use concurrent collection implementations for properties holding mutable state. Especially the ones making use of computeIfAbsent(…).

Usage of ConcurrentReferenceHashMap allows us to move on without additional changes as it allows to store null values, whereas ConcurrentHashMap would require us to store explicit NullValue placeholders for such cases, potentially causing trouble in downstream store specific projects.

JMH Benchmarks done with the Spring Data MongoDB mapping layer showed a potential performance loss of up to 5%. However a comparison between ConcurrentReferenceHashMap and ConcurrentHashMap did not show any significant difference in performance.

Original pull request: #259.

Author: christophstrobl

src/main/java/org/springframework/data/mapping/model/BasicPersistentEntity.java

@@ -20,7 +20,17 @@
 
 import java.io.Serializable;
 import java.lang.annotation.Annotation;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.stream.Collectors;
 
 import org.springframework.core.annotation.AnnotatedElementUtils;
@@ -30,7 +40,8 @@
 import org.springframework.data.util.TypeInformation;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
-import org.springframework.util.LinkedMultiValueMap;
+import org.springframework.util.CollectionUtils;
+import org.springframework.util.ConcurrentReferenceHashMap;
 import org.springframework.util.MultiValueMap;
 import org.springframework.util.StringUtils;
 
@@ -93,9 +104,10 @@ public BasicPersistentEntity(TypeInformation<T> information, @Nullable Comparato
 		this.constructor = PreferredConstructorDiscoverer.discover(this);
 		this.associations = comparator == null ? new HashSet<>() : new TreeSet<>(new AssociationComparator<>(comparator));
 
-		this.propertyCache = new HashMap<>();
-		this.annotationCache = new HashMap<>();
-		this.propertyAnnotationCache = new LinkedMultiValueMap<>();
+		this.propertyCache = new ConcurrentReferenceHashMap<>();
+		this.annotationCache = new ConcurrentReferenceHashMap<>();
+		this.propertyAnnotationCache = CollectionUtils
+				.toMultiValueMap(new ConcurrentReferenceHashMap<Class<? extends Annotation>, List<P>>());
 		this.propertyAccessorFactory = BeanWrapperPropertyAccessorFactory.INSTANCE;
 		this.typeAlias = Lazy.of(() -> getAliasFromAnnotation(getType()));
 	}
@@ -193,9 +205,7 @@ public void addPersistentProperty(P property) {
 			persistentPropertiesCache.add(property);
 		}
 
-		if (!propertyCache.containsKey(property.getName())) {
-			propertyCache.put(property.getName(), property);
-		}
+		propertyCache.computeIfAbsent(property.getName(), key -> property);
 
 		P candidate = returnPropertyIfBetterIdPropertyCandidateOrNull(property);
 

src/test/java/org/springframework/data/mapping/model/BasicPersistentEntityUnitTests.java

@@ -19,11 +19,14 @@
 import static org.junit.Assume.*;
 import static org.mockito.Mockito.*;
 
+import java.lang.annotation.Annotation;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.util.Comparator;
 import java.util.Iterator;
 import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import org.hamcrest.CoreMatchers;
 import org.junit.Rule;
@@ -34,9 +37,12 @@
 import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
 import org.springframework.core.annotation.AliasFor;
+import org.springframework.data.annotation.AccessType;
+import org.springframework.data.annotation.AccessType.Type;
 import org.springframework.data.annotation.CreatedBy;
 import org.springframework.data.annotation.CreatedDate;
 import org.springframework.data.annotation.LastModifiedBy;
+import org.springframework.data.annotation.Persistent;
 import org.springframework.data.annotation.TypeAlias;
 import org.springframework.data.mapping.Alias;
 import org.springframework.data.mapping.Document;
@@ -49,6 +55,7 @@
 import org.springframework.data.mapping.context.SampleMappingContext;
 import org.springframework.data.mapping.context.SamplePersistentProperty;
 import org.springframework.data.util.ClassTypeInformation;
+import org.springframework.data.util.Version;
 import org.springframework.test.util.ReflectionTestUtils;
 
 /**
@@ -276,6 +283,64 @@ public void getRequiredAnnotationThrowsException() {
 		assertThatThrownBy(() -> entity.getRequiredAnnotation(Document.class)).isInstanceOf(IllegalStateException.class);
 	}
 
+	@Test // DATACMNS-1210
+	public void findAnnotationShouldBeThreadSafe() throws InterruptedException {
+
+		assumeTrue("Requires Java 9",
+				Version.parse(System.getProperty("java.version")).isGreaterThanOrEqualTo(Version.parse("9.0")));
+
+		CountDownLatch latch = new CountDownLatch(2);
+		CountDownLatch syncLatch = new CountDownLatch(1);
+
+		final AtomicBoolean failed = new AtomicBoolean(false);
+
+		PersistentEntity<EntityWithAnnotation, T> entity = new BasicPersistentEntity(
+				ClassTypeInformation.from(EntityWithAnnotation.class), null) {
+
+			@Override
+			public Annotation findAnnotation(Class annotationType) {
+
+				try {
+					syncLatch.await();
+				} catch (InterruptedException e) {
+					e.printStackTrace();
+				}
+
+				return super.findAnnotation(annotationType);
+			}
+		};
+
+		Runnable findAccessType = () -> {
+
+			try {
+				entity.findAnnotation(AccessType.class);
+			} catch (Exception e) {
+				failed.set(true);
+			} finally {
+				latch.countDown();
+			}
+		};
+
+		Runnable findPersistent = () -> {
+
+			try {
+				entity.findAnnotation(Persistent.class);
+			} catch (Exception e) {
+				failed.set(true);
+			} finally {
+				latch.countDown();
+			}
+		};
+
+		new Thread(findAccessType).start();
+		new Thread(findPersistent).start();
+
+		syncLatch.countDown();
+		latch.await();
+
+		assertThat(failed.get()).isFalse();
+	}
+
 	private <S> BasicPersistentEntity<S, T> createEntity(Class<S> type) {
 		return createEntity(type, null);
 	}
@@ -315,4 +380,9 @@ public String getProperty() {
 	static class AliasEntityUsingComposedAnnotation {}
 
 	static class Subtype extends Entity {}
+
+	@AccessType(Type.PROPERTY)
+	static class EntityWithAnnotation {
+
+	}
 }