From f5d36aa47afe58517f4c1cd8d90f576ac322f3de Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 25 Sep 2020 10:55:32 +0200 Subject: [PATCH 1/2] 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 900282e458..a1750518a4 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 93916a671f..3119d051f4 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 2a7527aed9..cb25ac9d35 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 272ac19524..76ae746c97 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<>(); } From bf00db3c6c4fa3f0e103558f73a792c52c5615c0 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 25 Sep 2020 11:08:51 +0200 Subject: [PATCH 2/2] Avoid AssertJ deprecation warning --- .../springframework/mock/web/MockHttpServletRequestTests.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spring-test/src/test/java/org/springframework/mock/web/MockHttpServletRequestTests.java b/spring-test/src/test/java/org/springframework/mock/web/MockHttpServletRequestTests.java index 76104fbb6f..ed837c9267 100644 --- a/spring-test/src/test/java/org/springframework/mock/web/MockHttpServletRequestTests.java +++ b/spring-test/src/test/java/org/springframework/mock/web/MockHttpServletRequestTests.java @@ -297,8 +297,7 @@ class MockHttpServletRequestTests { assertThat(cookieHeaders) .describedAs("Cookies -> Header conversion works as expected per RFC6265") - .hasSize(1) - .hasOnlyOneElementSatisfying(header -> assertThat(header).isEqualTo("foo=bar; baz=qux")); + .singleElement().isEqualTo("foo=bar; baz=qux"); } @Test