Thread-safety of calling @Bean methods from returned anonymous inner classes

Question

Is the following test method thread-safe, assuming that this is being called from multiple threads? Sometimes the println statement displays "null".

The idea is to return a map that will create beans on demand based on a supplied identifier. Note that this is just a simple example to illustrate a similar real life scenario in which one bean did not have its dependencies satisfied (e.g., value.x being null) using the same approach. For bonus points, is there another (better) way to achieve the same effect?

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;

import javax.annotation.Resource;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Scope;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
import org.springframework.test.context.support.AnnotationConfigContextLoader;

import com.oanda.bi.rm.test.AnnotationConfigTest.Config;

@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration(classes = { Config.class }, loader = AnnotationConfigContextLoader.class)
public class AnnotationConfigTest {

    @Resource
    Map<String, Value> map;

    @Test
    public void test() throws InterruptedException {
        ExecutorService service = Executors.newFixedThreadPool( 10 );

        for ( int i = 0; i < 10; i++ ) {
            service.execute( new Runnable() {
                @Override
                public void run() {
                    // Is this thread-safe?
                    Value value = map.get( "value" );

                    // Sometimes null!
                    System.out.println( value.x );
                }
            } );
        }

        service.shutdown();
        service.awaitTermination( 1, TimeUnit.MINUTES );
    }

    public static class Value {
        @Resource
        protected Integer x;
    }

    @Configuration
    public static class Config {

        @Bean
        public Integer x() {
            return 1;
        }

        @Bean
        @Scope("prototype")
        public Value value() {
            return new Value();
        }

        @Bean
        @SuppressWarnings("serial")
        public Map<String, Value> map() {
            // Return a Spring agnostic "bean factory" map
            return Collections.unmodifiableMap( new HashMap<String, Value>() {
                @Override
                public Value get( Object obj ) {
                    String key = (String) obj;

                    if ( key.equals( "value" ) ) {
                        // Create new bean on demand
                        return value();
                    }

                    // Assume other similar branches here...

                    return null;
                }
            } );
        }

    }
}

Update

Given the insightful feedback from Biju Kunjummen, I've tried a different approach that uses the application context directly, but it still fails will nulls. This time I'm using a Function abstraction instead of a Map which seems more appropriate:

import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;

import javax.annotation.Resource;

import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.ApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Scope;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
import org.springframework.test.context.support.AnnotationConfigContextLoader;

import com.google.common.base.Function;
import com.oanda.bi.rm.test.AnnotationFunctionConfigTest.Config;

/**
 * Unit test that tries to perform the same pattern as {#link ResourceConfig} and ensure
 * thread safety.
 * 
 * @see http://stackoverflow.com/questions/12700239/thread-safety-of-calling-bean-methods-from-returned-annonymous-inner-classes/12700284#comment17146235_12700284
 * @see http://forum.springsource.org/showthread.php?130731-Thread-safety-of-calling-Bean-methods-from-returned-annonymous-inner-classes&p=426403#post426403
 * @author btiernay
 */
@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration(classes = { Config.class }, loader = AnnotationConfigContextLoader.class)
public class AnnotationFunctionConfigTest {

    @Resource
    Function<String, Value> function;

    @Test
    public void test() throws InterruptedException {
        final int threads = 10;
        ExecutorService service = Executors.newFixedThreadPool( threads );

        for ( int i = 0; i < threads; i++ ) {
            service.execute( new Runnable() {
                @Override
                public void run() {
                    Value value = function.apply( "value" );
                    Assert.assertNotNull( value.x );
                }
            } );
        }

        service.shutdown();
        service.awaitTermination( 1, TimeUnit.MINUTES );
    }

    public static class Value {
        @Resource
        protected Integer x;
    }

    @Configuration
    public static class Config {

        @Bean
        public Integer x() {
            return 1;
        }

        @Bean
        @Scope("prototype")
        public Value value() {
            return new Value();
        }

        @Bean
        public Function<String, Value> function() {
            // Return a Spring agnostic "bean factory" function
            return new Function<String, Value>() {
                @Autowired
                private ApplicationContext applicationContext;

                @Override
                public Value apply( String key ) {
                    if ( key.equals( "value" ) ) {
                        // Create new bean on demand
                        return applicationContext.getBean( key, Value.class );
                    }

                    // Assume other similar branches here...

                    return null;
                }
            };
        }

    }
}

Anyone care to comment why this still appears to be unsafe?

Update

This looks like it might be a Spring bug. I submitted a jira ticket:

https://jira.springsource.org/browse/SPR-9852

Answer 1

I wouldn't recommend the way you have implemented it:

1. Like Jordan said, the map is not really required there, you are not using it as a hashmap at all, instead just using it to call .value() method.

2. Spring @Configuration mechanism is being bypassed, internally Spring creates a CGLIB proxy for @Configuration classes and using this knows which dependencies need to be injected where and to create instances which know how to manage scopes, by bypassing it you are essentially not using Spring to manage your bean instances anymore.

The following does something similar to what you have implemented but I think is simpler and works consistently every time - this is using by using the application context to get a prototype bean and hiding this behind a custom factory this way:

Updated Implementation

import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.beans.BeansException;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.ApplicationContext;
import org.springframework.context.ApplicationContextAware;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Scope;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;


@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration
public class AnnotationConfigTest {


    @Autowired PrototypeBeanFactory prototypeFactory;

    @Test
    public void test() throws InterruptedException {
        ExecutorService service = Executors.newFixedThreadPool( 10 );

        for ( int i = 0; i < 10; i++ ) {
            service.execute( new Runnable() {
                @Override
                public void run() {
                    Value value = prototypeFactory.getBean("value", Value.class);
                    System.out.println( "value1.x = "  + value.getX() );
                }
            } );
        }

        service.shutdown();
        service.awaitTermination( 1, TimeUnit.MINUTES );
    }

    public static class Value {
        @Autowired
        private Integer x;

        public Integer getX() {
            return x;
        }

        public void setX(Integer x) {
            this.x = x;
        }
    }



    @Configuration
    public static class Config {

        @Bean
        public Integer x() {
            return 1;
        }

        @Bean
        @Scope(value="prototype")
        public Value value() {
            return new Value();
        }

        @Bean
        public PrototypeBeanFactory prototypeFactory(){
            return new PrototypeBeanFactory();
        }

    }


    public static class PrototypeBeanFactory implements ApplicationContextAware{
        private ApplicationContext applicationContext;
        @Override
        public void setApplicationContext(ApplicationContext applicationContext) throws BeansException {
            this.applicationContext = applicationContext;
        }

        public<T> T getBean(String name, Class<T> clazz){
            return this.applicationContext.getBean(name, clazz);
        }

    }
}

Answer 2

All you are doing is returning an immutable map with only a get method so really the only way this wouldn't be thread-safe is if you were modifying the value of the passed in obj. A better solution might be to use ConcurrentHashMap instead of Collections.unmodifiableMap, but that would only really be helpful if you are planning on adding values to the map later.