From f5d36aa47afe58517f4c1cd8d90f576ac322f3de Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 25 Sep 2020 10:55:32 +0200 Subject: [PATCH] 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 --- .../context/support/SimpleThreadScope.java | 13 ++++++++++--- .../context/support/SimpleThreadScopeTests.java | 6 +++--- .../context/support/simpleThreadScopeTests.xml | 14 +++++++++++--- .../support/SimpleTransactionScope.java | 13 ++++++++++--- 4 files changed, 34 insertions(+), 12 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/context/support/SimpleThreadScope.java b/spring-context/src/main/java/org/springframework/context/support/SimpleThreadScope.java index 900282e4587..a1750518a42 100644 --- a/spring-context/src/main/java/org/springframework/context/support/SimpleThreadScope.java +++ b/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>("SimpleThreadScope") { @Override protected Map 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 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 diff --git a/spring-context/src/test/java/org/springframework/context/support/SimpleThreadScopeTests.java b/spring-context/src/test/java/org/springframework/context/support/SimpleThreadScopeTests.java index 93916a671f9..3119d051f49 100644 --- a/spring-context/src/test/java/org/springframework/context/support/SimpleThreadScopeTests.java +++ b/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(); diff --git a/spring-context/src/test/resources/org/springframework/context/support/simpleThreadScopeTests.xml b/spring-context/src/test/resources/org/springframework/context/support/simpleThreadScopeTests.xml index 2a7527aed98..cb25ac9d353 100644 --- a/spring-context/src/test/resources/org/springframework/context/support/simpleThreadScopeTests.xml +++ b/spring-context/src/test/resources/org/springframework/context/support/simpleThreadScopeTests.xml @@ -12,10 +12,18 @@ - - + + + - + diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/SimpleTransactionScope.java b/spring-tx/src/main/java/org/springframework/transaction/support/SimpleTransactionScope.java index 272ac19524a..76ae746c971 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/SimpleTransactionScope.java +++ b/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 scopedInstances = new ConcurrentHashMap<>(); + final Map scopedInstances = new HashMap<>(); final Map destructionCallbacks = new LinkedHashMap<>(); }