[Spring] Instantiate TestContextManager synchronously (#2687)

In brief, the problem is that invoking `new TestContextManager(someContextConfiguration)`
for identical instances of someContextConfiguration is not actually
thread-safe. Spring caches the configuration derived from `someContextConfiguration`
and when AOT is used, will fail to apply idempotent updates configuration.

Fixes: #2686

Co-authored-by: M.P. Korstanje <[email protected]>

Author: ntthaibk

.gitignore

@@ -6,6 +6,7 @@
 .settings
 .project
 .classpath
+lib/
 
 
 # Build directories
@@ -18,7 +19,7 @@ out/
 # Build & test droppings
 pom.xml.releaseBackup
 pom.xml.versionsBackup
-release.properties
+release.propertiesF
 *.ser
 dependency-reduced-pom.xml
 *~

CHANGELOG.md

@@ -10,6 +10,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+### Fixed
+- [Spring] Instantiate `TestContextManager` synchronously ([#2686](https://github.com/cucumber/cucumber-jvm/pull/2686), [#2687](https://github.com/cucumber/cucumber-jvm/pull/2687) Thai Nguyen, M.P. Korstanje)
+
 ### Added
 - [Core] Warn when `cucumber.options` is used ([#2685](https://github.com/cucumber/cucumber-jvm/pull/2685) M.P. Korstanje)
 

cucumber-spring/src/main/java/io/cucumber/spring/SpringFactory.java

@@ -17,6 +17,8 @@
 import java.util.Collection;
 import java.util.HashSet;
 
+import static io.cucumber.spring.TestContextAdaptor.create;
+
 /**
  * Spring based implementation of ObjectFactory.
  * <p>
@@ -116,8 +118,7 @@ public void start() {
         // The application context created by the TestContextManager is
         // a singleton and reused between scenarios and shared between
         // threads.
-        TestContextManager testContextManager = new TestContextManager(withCucumberContextConfiguration);
-        testContextAdaptor = new TestContextAdaptor(testContextManager, stepClasses);
+        testContextAdaptor = create(() -> new TestContextManager(withCucumberContextConfiguration), stepClasses);
         testContextAdaptor.start();
     }
 

cucumber-spring/src/main/java/io/cucumber/spring/TestContextAdaptor.java

@@ -13,41 +13,51 @@
 import java.util.ArrayDeque;
 import java.util.Collection;
 import java.util.Deque;
+import java.util.function.Supplier;
 
 import static io.cucumber.spring.CucumberTestContext.SCOPE_CUCUMBER_GLUE;
 import static org.springframework.beans.factory.config.AutowireCapableBeanFactory.AUTOWIRE_CONSTRUCTOR;
 
 class TestContextAdaptor {
 
     private static final Object monitor = new Object();
-
     private final TestContextManager delegate;
     private final ConfigurableApplicationContext applicationContext;
-    private final Collection<Class<?>> glueClasses;
     private final Deque<Runnable> stopInvocations = new ArrayDeque<>();
     private Object delegateTestInstance;
 
-    TestContextAdaptor(
-            TestContextManager delegate,
+    static TestContextAdaptor create(
+            Supplier<TestContextManager> testContextManagerSupplier,
             Collection<Class<?>> glueClasses
     ) {
-        TestContext testContext = delegate.getTestContext();
-        ConfigurableApplicationContext applicationContext = (ConfigurableApplicationContext) testContext
-                .getApplicationContext();
-        this.delegate = delegate;
-        this.applicationContext = applicationContext;
-        this.glueClasses = glueClasses;
-    }
-
-    public final void start() {
-        // The TestContextManager delegate makes the application context
-        // available to other threads. Registering the glue however modifies the
-        // application context. To avoid concurrent modification issues (#1823,
-        // #1153, #1148, #1106) we do this serially.
         synchronized (monitor) {
+            // While under construction, the TestContextManager delegate will
+            // build a cached version of the application context configuration.
+            // Since Spring Boot 3 and in combination with AOT building this
+            // configuration is not idempotent (#2686).
+            TestContextManager delegate = testContextManagerSupplier.get();
+
+            TestContext testContext = delegate.getTestContext();
+            ConfigurableApplicationContext applicationContext = (ConfigurableApplicationContext) testContext
+                    .getApplicationContext();
+
+            // The TestContextManager delegate makes the application context
+            // available to other threads. Registering the glue however modifies
+            // the application context. To avoid concurrent modification issues
+            // (#1823, #1153, #1148, #1106) we do this serially.
             registerGlueCodeScope(applicationContext);
-            registerStepClassBeanDefinitions(applicationContext.getBeanFactory());
+            registerStepClassBeanDefinitions(applicationContext.getBeanFactory(), glueClasses);
+
+            return new TestContextAdaptor(delegate);
         }
+    }
+
+    TestContextAdaptor(TestContextManager delegate) {
+        this.delegate = delegate;
+        this.applicationContext = (ConfigurableApplicationContext) delegate.getTestContext().getApplicationContext();
+    }
+
+    final void start() {
         stopInvocations.push(this::notifyTestContextManagerAboutAfterTestClass);
         notifyContextManagerAboutBeforeTestClass();
         stopInvocations.push(this::stopCucumberTestContext);
@@ -125,7 +135,7 @@ private void notifyTestContextManagerAboutBeforeTestMethod() {
         }
     }
 
-    private void registerGlueCodeScope(ConfigurableApplicationContext context) {
+    private static void registerGlueCodeScope(ConfigurableApplicationContext context) {
         while (context != null) {
             ConfigurableListableBeanFactory beanFactory = context.getBeanFactory();
             // Scenario scope may have already been registered by another
@@ -146,14 +156,16 @@ private void notifyTestContextManagerAboutBeforeExecution() {
         }
     }
 
-    private void registerStepClassBeanDefinitions(ConfigurableListableBeanFactory beanFactory) {
+    private static void registerStepClassBeanDefinitions(
+            ConfigurableListableBeanFactory beanFactory, Collection<Class<?>> glueClasses
+    ) {
         BeanDefinitionRegistry registry = (BeanDefinitionRegistry) beanFactory;
         for (Class<?> glue : glueClasses) {
             registerStepClassBeanDefinition(registry, glue);
         }
     }
 
-    private void registerStepClassBeanDefinition(BeanDefinitionRegistry registry, Class<?> glueClass) {
+    private static void registerStepClassBeanDefinition(BeanDefinitionRegistry registry, Class<?> glueClass) {
         String beanName = glueClass.getName();
         // Step definition may have already been
         // registered as a bean by another thread.
@@ -166,7 +178,7 @@ private void registerStepClassBeanDefinition(BeanDefinitionRegistry registry, Cl
                 .getBeanDefinition());
     }
 
-    public final void stop() {
+    final void stop() {
         // Cucumber only supports 1 set of before/after semantics while JUnit
         // and Spring have 2 sets. So here we use a stack to ensure we don't
         // invoke only the matching after methods for each before methods.

cucumber-spring/src/test/java/io/cucumber/spring/TestTestContextAdaptorTest.java

@@ -19,6 +19,7 @@
 import org.springframework.test.context.TestContextManager;
 import org.springframework.test.context.TestExecutionListener;
 
+import static io.cucumber.spring.TestContextAdaptor.create;
 import static java.util.Collections.singletonList;
 import static org.junit.jupiter.api.Assertions.assertAll;
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
@@ -44,7 +45,7 @@ void verifyNoMoroInteractions() {
     @Test
     void invokesAllLiveCycleHooks() throws Exception {
         TestContextManager manager = new TestContextManager(SomeContextConfiguration.class);
-        TestContextAdaptor adaptor = new TestContextAdaptor(manager, singletonList(SomeContextConfiguration.class));
+        TestContextAdaptor adaptor = create(() -> manager, singletonList(SomeContextConfiguration.class));
         manager.registerTestExecutionListeners(listener);
         InOrder inOrder = inOrder(listener);
 
@@ -63,7 +64,7 @@ void invokesAllLiveCycleHooks() throws Exception {
     @Test
     void invokesAfterClassIfBeforeClassFailed() throws Exception {
         TestContextManager manager = new TestContextManager(SomeContextConfiguration.class);
-        TestContextAdaptor adaptor = new TestContextAdaptor(manager, singletonList(SomeContextConfiguration.class));
+        TestContextAdaptor adaptor = create(() -> manager, singletonList(SomeContextConfiguration.class));
         manager.registerTestExecutionListeners(listener);
         InOrder inOrder = inOrder(listener);
 
@@ -79,7 +80,7 @@ void invokesAfterClassIfBeforeClassFailed() throws Exception {
     @Test
     void invokesAfterClassIfPrepareTestInstanceFailed() throws Exception {
         TestContextManager manager = new TestContextManager(SomeContextConfiguration.class);
-        TestContextAdaptor adaptor = new TestContextAdaptor(manager, singletonList(SomeContextConfiguration.class));
+        TestContextAdaptor adaptor = create(() -> manager, singletonList(SomeContextConfiguration.class));
         manager.registerTestExecutionListeners(listener);
         InOrder inOrder = inOrder(listener);
 
@@ -95,7 +96,7 @@ void invokesAfterClassIfPrepareTestInstanceFailed() throws Exception {
     @Test
     void invokesAfterMethodIfBeforeMethodThrows() throws Exception {
         TestContextManager manager = new TestContextManager(SomeContextConfiguration.class);
-        TestContextAdaptor adaptor = new TestContextAdaptor(manager, singletonList(SomeContextConfiguration.class));
+        TestContextAdaptor adaptor = create(() -> manager, singletonList(SomeContextConfiguration.class));
         manager.registerTestExecutionListeners(listener);
         InOrder inOrder = inOrder(listener);
 
@@ -114,7 +115,7 @@ void invokesAfterMethodIfBeforeMethodThrows() throws Exception {
     @Test
     void invokesAfterTestExecutionIfBeforeTestExecutionThrows() throws Exception {
         TestContextManager manager = new TestContextManager(SomeContextConfiguration.class);
-        TestContextAdaptor adaptor = new TestContextAdaptor(manager, singletonList(SomeContextConfiguration.class));
+        TestContextAdaptor adaptor = create(() -> manager, singletonList(SomeContextConfiguration.class));
         manager.registerTestExecutionListeners(listener);
         InOrder inOrder = inOrder(listener);
 
@@ -134,7 +135,7 @@ void invokesAfterTestExecutionIfBeforeTestExecutionThrows() throws Exception {
     @Test
     void invokesAfterTestMethodIfAfterTestExecutionThrows() throws Exception {
         TestContextManager manager = new TestContextManager(SomeContextConfiguration.class);
-        TestContextAdaptor adaptor = new TestContextAdaptor(manager, singletonList(SomeContextConfiguration.class));
+        TestContextAdaptor adaptor = create(() -> manager, singletonList(SomeContextConfiguration.class));
         manager.registerTestExecutionListeners(listener);
         InOrder inOrder = inOrder(listener);
 
@@ -155,7 +156,7 @@ void invokesAfterTestMethodIfAfterTestExecutionThrows() throws Exception {
     @Test
     void invokesAfterTesClassIfAfterTestMethodThrows() throws Exception {
         TestContextManager manager = new TestContextManager(SomeContextConfiguration.class);
-        TestContextAdaptor adaptor = new TestContextAdaptor(manager, singletonList(SomeContextConfiguration.class));
+        TestContextAdaptor adaptor = create(() -> manager, singletonList(SomeContextConfiguration.class));
         manager.registerTestExecutionListeners(listener);
         InOrder inOrder = inOrder(listener);
 
@@ -176,7 +177,7 @@ void invokesAfterTesClassIfAfterTestMethodThrows() throws Exception {
     @Test
     void invokesAllMethodsPriorIfAfterTestClassThrows() throws Exception {
         TestContextManager manager = new TestContextManager(SomeContextConfiguration.class);
-        TestContextAdaptor adaptor = new TestContextAdaptor(manager, singletonList(SomeContextConfiguration.class));
+        TestContextAdaptor adaptor = create(() -> manager, singletonList(SomeContextConfiguration.class));
         manager.registerTestExecutionListeners(listener);
         InOrder inOrder = inOrder(listener);
 
@@ -198,7 +199,7 @@ void invokesAllMethodsPriorIfAfterTestClassThrows() throws Exception {
     @ValueSource(classes = { WithAutowiredDependency.class, WithConstructorDependency.class })
     void autowireAndPostProcessesOnlyOnce(Class<? extends Spy> testClass) {
         TestContextManager manager = new TestContextManager(testClass);
-        TestContextAdaptor adaptor = new TestContextAdaptor(manager, singletonList(testClass));
+        TestContextAdaptor adaptor = create(() -> manager, singletonList(testClass));
 
         assertAll(
             () -> assertDoesNotThrow(adaptor::start),