From 81858854cecded7a00ce9a0da79f066519aa6dcf Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 14 Nov 2017 07:44:15 +0100 Subject: [PATCH 1/2] DATACMNS-1210 - Prepare issue branch. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 2473b93f59..6586a293ff 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-commons - 2.1.0.BUILD-SNAPSHOT + 2.1.0.DATACMNS-1210-SNAPSHOT Spring Data Core From 8026f8a46a1d308e0050bcd65e3db1f85141a80e Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 14 Nov 2017 07:52:36 +0100 Subject: [PATCH 2/2] 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. --- .../mapping/model/BasicPersistentEntity.java | 26 ++++--- .../model/BasicPersistentEntityUnitTests.java | 70 +++++++++++++++++++ 2 files changed, 88 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/springframework/data/mapping/model/BasicPersistentEntity.java b/src/main/java/org/springframework/data/mapping/model/BasicPersistentEntity.java index 1448945287..667b0d57fb 100644 --- a/src/main/java/org/springframework/data/mapping/model/BasicPersistentEntity.java +++ b/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 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, List

>()); 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); diff --git a/src/test/java/org/springframework/data/mapping/model/BasicPersistentEntityUnitTests.java b/src/test/java/org/springframework/data/mapping/model/BasicPersistentEntityUnitTests.java index cd858cb557..bcdee511ed 100755 --- a/src/test/java/org/springframework/data/mapping/model/BasicPersistentEntityUnitTests.java +++ b/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 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 BasicPersistentEntity createEntity(Class 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 { + + } }