DATACMNS-1175 - Remove argument array caching from EntityInstantiators.

We no longer cache argument arrays in our EntityInstantiators to prevent changes to shared mutable state caused by reentrant calls.

Previously, a re-entrant call requesting an argument array of the same size as a previous call in the call stack reused the same array instance. Changes to this shared mutable state by multiple invocations caused an invalid state rendering wrong parameters for object instantiation. Removing the caching and only reusing an empty array for zero-arg constructors is the only safe approach for now.

Re-instantiation of object allocations results in a higher GC pressure but guarantee side effect-free instantiation and should be on-par with previous versions performance profile.

Original pull request: #247.

src/main/java/org/springframework/data/convert/ClassGeneratingEntityInstantiator.java

@@ -55,18 +55,7 @@ import org.springframework.util.ClassUtils;
  */
 public class ClassGeneratingEntityInstantiator implements EntityInstantiator {
 
-	private static final int ARG_CACHE_SIZE = 100;
-
-	private static final ThreadLocal<Object[][]> OBJECT_POOL = ThreadLocal.withInitial(() -> {
-
-		Object[][] cached = new Object[ARG_CACHE_SIZE][];
-
-		for (int i = 0; i < ARG_CACHE_SIZE; i++) {
-			cached[i] = new Object[i];
-		}
-
-		return cached;
-	});
+	private static final Object[] EMPTY_ARGS = new Object[0];
 
 	private final ObjectInstantiatorClassGenerator generator;
 
@@ -170,30 +159,14 @@ public class ClassGeneratingEntityInstantiator implements EntityInstantiator {
 	}
 
 	/**
-	 * Allocates an object array for instance creation. This method uses the argument array cache if possible.
+	 * Allocates an object array for instance creation.
 	 *
 	 * @param argumentCount
 	 * @return
 	 * @since 2.0
-	 * @see #ARG_CACHE_SIZE
 	 */
 	static Object[] allocateArguments(int argumentCount) {
-		return argumentCount < ARG_CACHE_SIZE ? OBJECT_POOL.get()[argumentCount] : new Object[argumentCount];
-	}
-
-	/**
-	 * Deallocates an object array used for instance creation. Parameters are cleared if the array was cached.
-	 *
-	 * @param argumentCount
-	 * @return
-	 * @since 2.0
-	 * @see #ARG_CACHE_SIZE
-	 */
-	static void deallocateArguments(Object[] params) {
-
-		if (params.length != 0 && params.length < ARG_CACHE_SIZE) {
-			Arrays.fill(params, null);
-		}
+		return argumentCount == 0 ? EMPTY_ARGS : new Object[argumentCount];
 	}
 
 	/**
@@ -250,8 +223,6 @@ public class ClassGeneratingEntityInstantiator implements EntityInstantiator {
 				return (T) instantiator.newInstance(params);
 			} catch (Exception e) {
 				throw new MappingInstantiationException(entity, Arrays.asList(params), e);
-			} finally {
-				deallocateArguments(params);
 			}
 		}
 

src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java

@@ -237,11 +237,7 @@ public class KotlinClassGeneratingEntityInstantiator extends ClassGeneratingEnti
 				params[userParameterCount + i] = defaulting[i];
 			}
 
-			try {
-				return (T) instantiator.newInstance(params);
-			} finally {
-				deallocateArguments(params);
-			}
+			return (T) instantiator.newInstance(params);
 		}
 	}
 }

src/main/java/org/springframework/data/convert/ReflectionEntityInstantiator.java

@@ -40,18 +40,7 @@ public enum ReflectionEntityInstantiator implements EntityInstantiator {
 
 	INSTANCE;
 
-	private final int ARG_CACHE_SIZE = 100;
-
-	private final ThreadLocal<Object[][]> objectPool = ThreadLocal.withInitial(() -> {
-
-		Object[][] cached = new Object[ARG_CACHE_SIZE][];
-
-		for (int i = 0; i < ARG_CACHE_SIZE; i++) {
-			cached[i] = new Object[i];
-		}
-
-		return cached;
-	});
+	private static final Object[] EMPTY_ARGS = new Object[0];
 
 	@SuppressWarnings("unchecked")
 	public <T, E extends PersistentEntity<? extends T, P>, P extends PersistentProperty<P>> T createInstance(E entity,
@@ -80,7 +69,7 @@ public enum ReflectionEntityInstantiator implements EntityInstantiator {
 		}
 		int parameterCount = constructor.getConstructor().getParameterCount();
 
-		Object[] params = parameterCount < ARG_CACHE_SIZE ? objectPool.get()[parameterCount] : new Object[parameterCount];
+		Object[] params = parameterCount == 0 ? EMPTY_ARGS : new Object[parameterCount];
 		int i = 0;
 		for (Parameter<?, P> parameter : constructor.getParameters()) {
 			params[i++] = provider.getParameterValue(parameter);
@@ -90,8 +79,6 @@ public enum ReflectionEntityInstantiator implements EntityInstantiator {
 			return BeanUtils.instantiateClass(constructor.getConstructor(), params);
 		} catch (BeanInstantiationException e) {
 			throw new MappingInstantiationException(entity, new ArrayList<>(Arrays.asList(params)), e);
-		} finally {
-			Arrays.fill(params, null);
 		}
 	}
 }

src/test/java/org/springframework/data/convert/ClassGeneratingEntityInstantiatorUnitTests.java

@@ -147,6 +147,40 @@ public class ClassGeneratingEntityInstantiatorUnitTests<P extends PersistentProp
 		}
 	}
 
+	@Test // DATACMNS-1175
+	@SuppressWarnings({ "unchecked", "rawtypes" })
+	public void createsInstancesWithRecursionAndSameCtorArgCountCorrectly() {
+
+		PersistentEntity<SampleWithReference, P> outer = new BasicPersistentEntity<>(from(SampleWithReference.class));
+		PersistentEntity<Sample, P> inner = new BasicPersistentEntity<>(from(Sample.class));
+
+		doReturn(2L, "FOO").when(provider).getParameterValue(any(Parameter.class));
+
+		ParameterValueProvider<P> recursive = new ParameterValueProvider<P>() {
+
+			@Override
+			public <T> T getParameterValue(Parameter<T, P> parameter) {
+
+				if (parameter.getName().equals("id")) {
+					return (T) Long.valueOf(1);
+				}
+
+				if (parameter.getName().equals("sample")) {
+					return (T) instance.createInstance(inner, provider);
+				}
+
+				throw new UnsupportedOperationException(parameter.getName());
+			}
+		};
+
+		SampleWithReference reference = this.instance.createInstance(outer, recursive);
+
+		assertThat(reference.id).isEqualTo(1L);
+		assertThat(reference.sample).isNotNull();
+		assertThat(reference.sample.id).isEqualTo(2L);
+		assertThat(reference.sample.name).isEqualTo("FOO");
+	}
+
 	@Test // DATACMNS-578, DATACMNS-1126
 	public void instantiateObjCtorDefault() {
 
@@ -154,7 +188,8 @@ public class ClassGeneratingEntityInstantiatorUnitTests<P extends PersistentProp
 		doReturn(PreferredConstructorDiscoverer.discover(ObjCtorDefault.class))//
 				.when(entity).getPersistenceConstructor();
 
-		IntStream.range(0, 2).forEach(i -> assertThat(this.instance.createInstance(entity, provider)).isInstanceOf(ObjCtorDefault.class));
+		IntStream.range(0, 2)
+				.forEach(i -> assertThat(this.instance.createInstance(entity, provider)).isInstanceOf(ObjCtorDefault.class));
 	}
 
 	@Test // DATACMNS-578, DATACMNS-1126
@@ -283,6 +318,18 @@ public class ClassGeneratingEntityInstantiatorUnitTests<P extends PersistentProp
 		}
 	}
 
+	static class SampleWithReference {
+
+		final Long id;
+		final Sample sample;
+
+		public SampleWithReference(Long id, Sample sample) {
+
+			this.id = id;
+			this.sample = sample;
+		}
+	}
+
 	/**
 	 * @author Thomas Darimont
 	 */