From 309cdc5e43b98f9bfe8d361703f8416919c7c9d2 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 14 Nov 2017 07:52:36 +0100 Subject: [PATCH] DATACMNS-1210 - Fix concurrency issue in BasicPersitentEntity. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../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 144894528..667b0d57f 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 lombok.RequiredArgsConstructor; 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.Lazy; 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 class BasicPersistentEntity> implement 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 class BasicPersistentEntity> implement 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 cd858cb55..bcdee511e 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.assertj.core.api.Assertions.*; 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.Mock; 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.Person; 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 class BasicPersistentEntityUnitTests> { 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 class BasicPersistentEntityUnitTests> { static class AliasEntityUsingComposedAnnotation {} static class Subtype extends Entity {} + + @AccessType(Type.PROPERTY) + static class EntityWithAnnotation { + + } }