Revert use of Map::computeIfAbsent in thread and tx scopes

Issues gh-25038 and gh-25618 collectively introduced a regression for
thread-scoped and transaction-scoped beans.

For example, given a thread-scoped bean X that depends on another
thread-scoped bean Y, if the names of the beans (when used as map keys)
end up in the same bucket within a ConcurrentHashMap AND an attempt is
made to retrieve bean X from the ApplicationContext prior to retrieving
bean Y, then the use of Map::computeIfAbsent in SimpleThreadScope
results in recursive access to the same internal bucket in the map.

On Java 8, that scenario simply hangs. On Java 9 and higher,
ConcurrentHashMap throws an IllegalStateException pointing out that a
"Recursive update" was attempted.

In light of these findings, we are reverting the changes made to
SimpleThreadScope and SimpleTransactionScope in commits 50a4fdac6e and
148dc95eb1.

Closes gh-25801

spring-context/src/main/java/org/springframework/context/support/SimpleThreadScope.java

@@ -16,8 +16,8 @@
 
 package org.springframework.context.support;
 
+import java.util.HashMap;
 import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -59,7 +59,7 @@ public class SimpleThreadScope implements Scope {
 			new NamedThreadLocal<Map<String, Object>>("SimpleThreadScope") {
 				@Override
 				protected Map<String, Object> initialValue() {
-					return new ConcurrentHashMap<>();
+					return new HashMap<>();
 				}
 			};
 
@@ -67,7 +67,14 @@ public class SimpleThreadScope implements Scope {
 	@Override
 	public Object get(String name, ObjectFactory<?> objectFactory) {
 		Map<String, Object> scope = this.threadScope.get();
-		return scope.computeIfAbsent(name, k -> objectFactory.getObject());
+		// NOTE: Do NOT modify the following to use Map::computeIfAbsent. For details,
+		// see https://github.com/spring-projects/spring-framework/issues/25801.
+		Object scopedObject = scope.get(name);
+		if (scopedObject == null) {
+			scopedObject = objectFactory.getObject();
+			scope.put(name, scopedObject);
+		}
+		return scopedObject;
 	}
 
 	@Override

spring-context/src/test/java/org/springframework/context/support/SimpleThreadScopeTests.java

@@ -38,7 +38,7 @@ class SimpleThreadScopeTests {
 
 	@Test
 	void getFromScope() throws Exception {
-		String name = "threadScopedObject";
+		String name = "removeNodeStatusScreen";
 		TestBean bean = this.applicationContext.getBean(name, TestBean.class);
 		assertThat(bean).isNotNull();
 		assertThat(this.applicationContext.getBean(name)).isSameAs(bean);
@@ -50,8 +50,8 @@ class SimpleThreadScopeTests {
 	void getMultipleInstances() throws Exception {
 		// Arrange
 		TestBean[] beans = new TestBean[2];
-		Thread thread1 = new Thread(() -> beans[0] = applicationContext.getBean("threadScopedObject", TestBean.class));
-		Thread thread2 = new Thread(() -> beans[1] = applicationContext.getBean("threadScopedObject", TestBean.class));
+		Thread thread1 = new Thread(() -> beans[0] = applicationContext.getBean("removeNodeStatusScreen", TestBean.class));
+		Thread thread2 = new Thread(() -> beans[1] = applicationContext.getBean("removeNodeStatusScreen", TestBean.class));
 		// Act
 		thread1.start();
 		thread2.start();

spring-context/src/test/resources/org/springframework/context/support/simpleThreadScopeTests.xml

@@ -12,10 +12,18 @@
 		</property>
 	</bean>
 
-	<bean id="threadScopedObject" class="org.springframework.beans.testfixture.beans.TestBean" scope="thread">
-		<property name="spouse" ref="threadScopedObject2" />
+	<!--
+		NOTE: The bean names removeNodeStatusScreen and removeNodeStatusPresenter are seemingly
+		quite odd for TestBean instances; however, these have been chosen due to the fact that
+		they end up in the same bucket within a HashMap/ConcurrentHashMap initialized with the
+		default initial capacity.
+
+		For details see: https://github.com/spring-projects/spring-framework/issues/25801
+	 -->
+	<bean id="removeNodeStatusScreen" class="org.springframework.beans.testfixture.beans.TestBean" scope="thread">
+		<property name="spouse" ref="removeNodeStatusPresenter" />
 	</bean>
 
-	<bean id="threadScopedObject2" class="org.springframework.beans.testfixture.beans.TestBean" scope="thread" />
+	<bean id="removeNodeStatusPresenter" class="org.springframework.beans.testfixture.beans.TestBean" scope="thread" />
 
 </beans>

spring-tx/src/main/java/org/springframework/transaction/support/SimpleTransactionScope.java

@@ -16,9 +16,9 @@
 
 package org.springframework.transaction.support;
 
+import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
 
 import org.springframework.beans.factory.ObjectFactory;
 import org.springframework.beans.factory.config.Scope;
@@ -50,7 +50,14 @@ public class SimpleTransactionScope implements Scope {
 			TransactionSynchronizationManager.registerSynchronization(new CleanupSynchronization(scopedObjects));
 			TransactionSynchronizationManager.bindResource(this, scopedObjects);
 		}
-		return scopedObjects.scopedInstances.computeIfAbsent(name, k -> objectFactory.getObject());
+		// NOTE: Do NOT modify the following to use Map::computeIfAbsent. For details,
+		// see https://github.com/spring-projects/spring-framework/issues/25801.
+		Object scopedObject = scopedObjects.scopedInstances.get(name);
+		if (scopedObject == null) {
+			scopedObject = objectFactory.getObject();
+			scopedObjects.scopedInstances.put(name, scopedObject);
+		}
+		return scopedObject;
 	}
 
 	@Override
@@ -92,7 +99,7 @@ public class SimpleTransactionScope implements Scope {
 	 */
 	static class ScopedObjectsHolder {
 
-		final Map<String, Object> scopedInstances = new ConcurrentHashMap<>();
+		final Map<String, Object> scopedInstances = new HashMap<>();
 
 		final Map<String, Runnable> destructionCallbacks = new LinkedHashMap<>();
 	}