DATACMNS-65 - Using ReentrantReadWriteLock to prevent race conditions in PersistentEntity creation.

We have to lock PersistentEntity creation as instances could be prematurely handed out and iteration over the properties might interfere new properties being added to the Set iterated over.

spring-data-commons-core/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java

@@ -32,6 +32,8 @@ import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 import org.springframework.beans.factory.InitializingBean;
 import org.springframework.context.ApplicationEventPublisher;
@@ -51,6 +53,9 @@ import org.springframework.validation.Validator;
 /**
  * Base class to build mapping metadata and thus create instances of {@link PersistentEntity} and
  * {@link PersistentProperty}.
+ * <p>
+ * The implementation uses a {@link ReentrantReadWriteLock} to make sure {@link PersistentEntity} are completely
+ * populated before accessing them from outside.
  * 
  * @param E the concrete {@link PersistentEntity} type the {@link MappingContext} implementation creates
  * @param P the concrete {@link PersistentProperty} type the {@link MappingContext} implementation creates
@@ -70,6 +75,10 @@ public abstract class AbstractMappingContext<E extends MutablePersistentEntity<?
 	private boolean strict = false;
 	private SimpleTypeHolder simpleTypeHolder = new SimpleTypeHolder();
 
+	private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
+	private final Lock read = lock.readLock();
+	private final Lock write = lock.writeLock();
+
 	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.context.ApplicationEventPublisherAware#setApplicationEventPublisher(org.springframework.context.ApplicationEventPublisher)
@@ -115,7 +124,12 @@ public abstract class AbstractMappingContext<E extends MutablePersistentEntity<?
 	 * @see org.springframework.data.mapping.model.MappingContext#getPersistentEntities()
 	 */
 	public Collection<E> getPersistentEntities() {
-		return persistentEntities.values();
+		try {
+			read.lock();
+			return persistentEntities.values();
+		} finally {
+			read.unlock();
+		}
 	}
 
 	/*
@@ -132,10 +146,16 @@ public abstract class AbstractMappingContext<E extends MutablePersistentEntity<?
 	 */
 	public E getPersistentEntity(TypeInformation<?> type) {
 
-		E entity = persistentEntities.get(type);
+		try {
+			read.lock();
+			E entity = persistentEntities.get(type);
 
-		if (entity != null) {
-			return entity;
+			if (entity != null) {
+				return entity;
+			}
+
+		} finally {
+			read.unlock();
 		}
 
 		if (strict) {
@@ -144,7 +164,7 @@ public abstract class AbstractMappingContext<E extends MutablePersistentEntity<?
 
 		return addPersistentEntity(type);
 	}
-	
+
 	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.mapping.context.MappingContext#getPersistentPropertyPath(java.lang.Class, java.lang.String)
@@ -206,6 +226,9 @@ public abstract class AbstractMappingContext<E extends MutablePersistentEntity<?
 		Class<?> type = typeInformation.getType();
 
 		try {
+
+			write.lock();
+
 			final E entity = createPersistentEntity(typeInformation);
 
 			// Eagerly cache the entity as we might have to find it during recursive lookups.
@@ -260,8 +283,11 @@ public abstract class AbstractMappingContext<E extends MutablePersistentEntity<?
 			}
 
 			return entity;
+
 		} catch (IntrospectionException e) {
 			throw new MappingException(e.getMessage(), e);
+		} finally {
+			write.unlock();
 		}
 	}
 

spring-data-commons-core/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextIntegrationTest.java

@@ -0,0 +1,109 @@
+/*
+ * Copyright (c) 2011 by the original author(s).
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.data.mapping.context;
+
+import static org.mockito.Mockito.*;
+
+import java.beans.PropertyDescriptor;
+import java.lang.reflect.Field;
+
+import org.junit.Test;
+import org.springframework.data.mapping.PersistentEntity;
+import org.springframework.data.mapping.PersistentProperty;
+import org.springframework.data.mapping.PropertyHandler;
+import org.springframework.data.mapping.model.BasicPersistentEntity;
+import org.springframework.data.mapping.model.SimpleTypeHolder;
+import org.springframework.data.util.TypeInformation;
+
+/**
+ * Unit tests for {@link AbstractMappingContext}.
+ * 
+ * @author Oliver Gierke
+ */
+public class AbstractMappingContextIntegrationTest<T extends PersistentProperty<T>> {
+
+	@Test
+	public void foo() throws InterruptedException {
+
+		final DummyMappingContext context = new DummyMappingContext();
+
+		Thread a = new Thread(new Runnable() {
+			public void run() {
+				context.getPersistentEntity(Person.class);
+			}
+		});
+
+		Thread b = new Thread(new Runnable() {
+
+			public void run() {
+
+				PersistentEntity<Object, T> entity = context.getPersistentEntity(Person.class);
+				
+				entity.doWithProperties(new PropertyHandler<T>() {
+					public void doWithPersistentProperty(T persistentProperty) {
+						try {
+							Thread.sleep(1000);
+						} catch (InterruptedException e) {
+							throw new RuntimeException(e);
+						}
+					}
+				});
+			}
+		});
+
+		a.start();
+		Thread.sleep(2800);
+		b.start();
+		
+		a.join();
+		b.join();
+	}
+
+	class DummyMappingContext extends AbstractMappingContext<BasicPersistentEntity<Object, T>, T> {
+
+		@Override
+		@SuppressWarnings("unchecked")
+		protected <S> BasicPersistentEntity<Object, T> createPersistentEntity(TypeInformation<S> typeInformation) {
+			return (BasicPersistentEntity<Object, T>) new BasicPersistentEntity<S, T>(typeInformation);
+		}
+
+		@Override
+		@SuppressWarnings({ "rawtypes", "unchecked" })
+		protected T createPersistentProperty(final Field field, final PropertyDescriptor descriptor,
+				final BasicPersistentEntity<Object, T> owner, final SimpleTypeHolder simpleTypeHolder) {
+
+			PersistentProperty prop = mock(PersistentProperty.class);
+			
+			when(prop.getTypeInformation()).thenReturn((TypeInformation) owner.getTypeInformation());
+			when(prop.getName()).thenReturn(field.getName());
+
+			try {
+				Thread.sleep(800);
+			} catch (InterruptedException e) {
+				throw new RuntimeException(e);
+			}
+			
+			return (T) prop;
+		}
+	}
+
+	class Person {
+
+		String firstname;
+		String lastname;
+		String email;
+	}
+}