Spring Thread-scoped bean creation freezes if dependent bean is retrieved before dependency bean

Affects: 5.2.9

---

It seems that if a dependent thread-scoped bean is retrieved from the ApplicationContext before the thread-scoped dependency, then bean creation could lock up in an infinite loop, never to return again.

Regression bug. Works fine with spring-framework 5.2.7, fails with spring-framework 5.2.9. May be a bug in spring-test rather than spring-context. May be hard to reproduce, hoping that this limited information is sufficient to lead to a fix.

SimpleThreadScope likely relevant since SimpleThreadScope has an infinite loop - the bean creation freezes in:

// org.springframework.context.support.SimpleThreadScope.class, the scope map is empty
public Object get(String name, ObjectFactory<?> objectFactory) {
    
  Map<String, Object> scope = (Map)this.threadScope.get();
    
    return scope.computeIfAbsent(name, (k) -> {
        
  return objectFactory.getObject();
    
});


Standalone test case:

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
import org.springframework.beans.factory.config.Scope;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.support.GenericApplicationContext;
import org.springframework.context.support.SimpleThreadScope;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.TestContext;
import org.springframework.test.context.TestExecutionListeners;
import org.springframework.test.context.junit.jupiter.SpringExtension;
import org.springframework.test.context.support.AbstractTestExecutionListener;
import org.springframework.test.context.support.AnnotationConfigContextLoader;
import org.springframework.test.context.support.DependencyInjectionTestExecutionListener;

@ExtendWith(SpringExtension.class)
@ContextConfiguration(classes={MyTest.MyTestConfig.class},
        loader= AnnotationConfigContextLoader.class)
@TestExecutionListeners({MyTest.MyTestExecutionListener.class,
        DependencyInjectionTestExecutionListener.class})
public class MyTest {
    @Autowired
    private MyView removeNodeStatusScreen;

    @Test
    void justRun() {
        System.out.println("This test should run");
    }

    @Configuration
    static class MyTestConfig {
        @Bean
        @org.springframework.context.annotation.Scope("thread")
        // The bean name seems to matter, if I change the name the test passes
        public MyView removeNodeStatusScreen(
                MyPresenter myPresenter) {
            return new MyView(myPresenter);
        }

        @Bean
        @org.springframework.context.annotation.Scope("thread")
        // The bean name seems to matter, if I change the name the test passes
        public MyPresenter removeNodeStatusPresenter() {
            return new MyPresenter();
        }
    }

    static class MyTestExecutionListener
            extends AbstractTestExecutionListener {

        @Override
        public void prepareTestInstance(TestContext testContext) throws Exception {
            if (testContext.getApplicationContext() instanceof GenericApplicationContext) {
                GenericApplicationContext context =
                        (GenericApplicationContext) testContext.getApplicationContext();
                ConfigurableListableBeanFactory beanFactory = context
                        .getBeanFactory();
                Scope viewScope = new SimpleThreadScope();
                beanFactory.registerScope("thread", viewScope);
            }
        }
    }

    public static class MyPresenter {
        public MyPresenter() {
        }
    }

    public static class MyView {
        public MyView(MyPresenter myPresenter) {
        }
    }
}

2 possible workarounds found:

Workaround 1: Add @DependsOn annotation to configuration class. However, @DependsOn would not be necessary if there were no bug:

@Configuration
public class MyConfig {
  @ViewScope
  @Bean 
  @DependsOn("myPresenter")
  public MyView myView(MyPresenter myPresenter) {
    return new MyView(myPresenter);
  }

  @Bean
  @ViewScope
  public MyPresenter myPresenter() {
     return new myPresenter();
  }
}

Workaround 2: swap order of beans in test. The order of the beans would not matter if there were no bug.

class MyConfigTest {
  // myView depends on myPresenter, so make sure the myPresenter is declared first
  // Otherwise test freezes on bean creation
  @Autowired
  MyPresenter myPresenter;

  @Autowired
  MyView myView;
...

Comment From: sbrannen

Potentially related to #25618.

@nguyenquoc, out of curiosity, what version of the JDK are you using?

Comment From: sbrannen

Works fine with spring-framework 5.2.7, fails with spring-framework 5.2.9. May be a bug in spring-test rather than spring-framework. May be hard to reproduce, hoping that this limited information is sufficient to lead to a fix.

I tried to reproduce this with the following all-in-one test class.

@SpringJUnitConfig
@TestExecutionListeners(listeners = MyConfigTests.MyConfigTestExecutionListener.class, mergeMode = MergeMode.MERGE_WITH_DEFAULTS)
class MyConfigTests {

    @Autowired
    MyView myView;

    @Autowired
    MyPresenter myPresenter;

    @Test
    void testSomething() {
        // empty test, never runs because Bean creation freezes
    }

    static class MyConfigTestExecutionListener extends AbstractTestExecutionListener {

        @Override
        public int getOrder() {
            return Ordered.HIGHEST_PRECEDENCE;
        }

        @Override
        public void prepareTestInstance(TestContext testContext) throws Exception {
            if (testContext.getApplicationContext() instanceof GenericApplicationContext) {
                GenericApplicationContext context = (GenericApplicationContext) testContext.getApplicationContext();
                ConfigurableListableBeanFactory beanFactory = context.getBeanFactory();
                beanFactory.registerScope("thread", new SimpleThreadScope());
            }
        }
    }

    @Configuration
    static class MyConfig {

        @Bean
        @Scope("thread")
        MyView myView(MyPresenter myPresenter) {
            return new MyView(myPresenter);
        }

        @Bean
        @Scope("thread")
        MyPresenter myPresenter() {
            return new MyPresenter();
        }

    }

    static class MyView {

        MyView(MyPresenter myPresenter) {
        }
    }

    static class MyPresenter {
    }

}

This passes against 5.2.7, 5.2.8, 5.2.9, and master using Java 8. It fails on 5.2.7 and 5.2.8 using Java 11+, but that failure is different and was fixed in #25618.

@nguyenquoc, can you please run the above test class in your environment and let me know if it passes for you?

Comment From: nguyenquoc

Using JDK:

openjdk version "1.8.0_265" OpenJDK Runtime Environment (Zulu 8.48.0.53-CA-macosx) (build 1.8.0_265-b11) OpenJDK 64-Bit Server VM (Zulu 8.48.0.53-CA-macosx) (build 25.265-b11, mixed mode)

Reproduced on Windows and Linux also.

Comment From: sbrannen

Using JDK:

openjdk version "1.8.0_265" OpenJDK Runtime Environment (Zulu 8.48.0.53-CA-macosx) (build 1.8.0_265-b11) OpenJDK 64-Bit Server VM (Zulu 8.48.0.53-CA-macosx) (build 25.265-b11, mixed mode)

Thanks for the feedback!

Reproduced on Windows and Linux also.

Were you able to reproduce it using the all-in-one test case I posted in https://github.com/spring-projects/spring-framework/issues/25801#issuecomment-697253945?

Comment From: nguyenquoc

The all-in-one test case runs fine in my environment. Apologies, I haven't had a chance to create.a minimal test case to demonstrate the bug yet partially because I have a workaround.

Comment From: sbrannen

The all-in-one test case runs fine in my environment.

Thanks for confirming.

Apologies, I haven't had a chance to create.a minimal test case to demonstrate the bug yet partially because I have a workaround.

No problem.

It appears that the bug may only arise in more complex scenarios which may be difficult to reproduce in a reliable test case.

If you find a way to reliably reproduce the bug in the coming week, that would be great!

If not, we might just roll back the changes introduced in #25038 and #25618, since this would effectively be the second reported regression for these code paths.

Comment From: nguyenquoc

I updated the original post with a standalone test case, thanks for providing the all-in-one sample.

Amazingly, the name of the bean in the configuration class seems to matter.

Comment From: sbrannen

I updated the original post with a standalone test case, thanks for providing the all-in-one sample.

Thanks. And... you're welcome.

Amazingly, the name of the bean in the configuration class seems to matter.

OK. That seems truly bizarre. We'll investigate!

Comment From: sbrannen

I figured out what's going on here.

Basically, the two bean names removeNodeStatusScreen and removeNodeStatusPresenter end up in the same bucket within ConcurrentHashMap, and attempting to store the respective objects in the same bucket recursively via computeIfAbsent() will result in the process hanging on Java 8.

This bug was fixed in Java 9 (see https://bugs.openjdk.java.net/browse/JDK-8062841). When I execute the same test on Java 9 or higher, the test fails with the following root cause.

Caused by: java.lang.IllegalStateException: Recursive update
    at java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1774) ~[?:?]
    at org.springframework.context.support.SimpleThreadScope.get(SimpleThreadScope.java:70) ~[main/:?]

Note that changing the initial capacity of the ConcurrentHashMap in SimpleThreadScope to 12 or more allows the test to pass as well, since there is then no longer an attempt to enter the same bucket recursively.

Changing the names of the two beans has a similar effect, but we cannot rely on user-defined bean names to magically work, and we cannot pick a magic initial capacity that seems to work.

In light of the above findings, we do in fact consider this a regression, and we will revert the corresponding changes introduced in #25038 and #25618.

---

p.s. for those interested...

With the default initial capacity for ConcurrentHashMap, the strings removeNodeStatusScreen and removeNodeStatusPresenter both end up in bucket 15 in the ConcurrentHashMap.table node array.

With an initial capacity of 12 for ConcurrentHashMap, the strings removeNodeStatusScreen and removeNodeStatusPresenter end up in buckets 15 and 31, respectively, in the ConcurrentHashMap.table node array.

Comment From: nguyenquoc

Thanks very much @sbrannen, appreciate the thorough investigation!