Synchronize access to the ObjectFactory in GenericScope

Otherwise multiple instances of a scoped target can be created
if several threads need one at the same time. This isn't always
going to be a problem, but it breaks the singleton semantics, and
might lead to surprising results and/or performance issues if bean
creation is expensive.

spring-cloud-context/src/main/java/org/springframework/cloud/context/scope/GenericScope.java

@@ -154,8 +154,8 @@ public class GenericScope implements Scope, BeanFactoryPostProcessor, Disposable
 		if (this.lifecycle == null) {
 			this.lifecycle = new StandardBeanLifecycleDecorator(this.proxyTargetClass);
 		}
-		BeanLifecycleWrapper value = this.cache.put(name, new BeanLifecycleWrapper(name,
-				objectFactory, this.lifecycle));
+		BeanLifecycleWrapper value = this.cache.put(name,
+				new BeanLifecycleWrapper(name, objectFactory, this.lifecycle));
 		return value.getBean();
 	}
 
@@ -198,7 +198,8 @@ public class GenericScope implements Scope, BeanFactoryPostProcessor, Disposable
 				return parser.parseExpression(input);
 			}
 			catch (ParseException e) {
-				throw new IllegalArgumentException("Cannot parse expression: " + input, e);
+				throw new IllegalArgumentException("Cannot parse expression: " + input,
+						e);
 			}
 
 		}
@@ -240,8 +241,9 @@ public class GenericScope implements Scope, BeanFactoryPostProcessor, Disposable
 
 		}
 		else {
-			logger.warn("BeanFactory was not a DefaultListableBeanFactory, so RefreshScope beans "
-					+ "cannot be serialized reliably and passed to a remote JVM.");
+			logger.warn(
+					"BeanFactory was not a DefaultListableBeanFactory, so RefreshScope beans "
+							+ "cannot be serialized reliably and passed to a remote JVM.");
 		}
 
 	}
@@ -323,8 +325,12 @@ public class GenericScope implements Scope, BeanFactoryPostProcessor, Disposable
 		@SuppressWarnings("unchecked")
 		public Object getBean() {
 			if (this.bean == null) {
-				this.bean = this.lifecycle.decorateBean(this.objectFactory.getObject(),
-						this.context);
+				synchronized (this.name) {
+					if (this.bean == null) {
+						this.bean = this.lifecycle.decorateBean(
+								this.objectFactory.getObject(), this.context);
+					}
+				}
 			}
 			return this.bean;
 		}

spring-cloud-context/src/test/java/org/springframework/cloud/context/scope/refresh/RefreshScopeConcurrencyTests.java

@@ -15,10 +15,6 @@
  */
 package org.springframework.cloud.context.scope.refresh;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertTrue;
-
 import java.util.concurrent.Callable;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutorService;
@@ -37,19 +33,23 @@ import org.springframework.boot.autoconfigure.PropertyPlaceholderAutoConfigurati
 import org.springframework.boot.context.properties.ConfigurationProperties;
 import org.springframework.boot.context.properties.EnableConfigurationProperties;
 import org.springframework.boot.test.SpringApplicationConfiguration;
+import org.springframework.cloud.autoconfigure.RefreshAutoConfiguration;
+import org.springframework.cloud.context.config.annotation.RefreshScope;
+import org.springframework.cloud.context.scope.refresh.RefreshScopeConcurrencyTests.TestConfiguration;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Import;
 import org.springframework.jmx.export.annotation.ManagedAttribute;
 import org.springframework.jmx.export.annotation.ManagedResource;
-import org.springframework.cloud.autoconfigure.RefreshAutoConfiguration;
-import org.springframework.cloud.context.config.annotation.RefreshScope;
-import org.springframework.cloud.context.scope.refresh.RefreshScopeConcurrencyTests.TestConfiguration;
 import org.springframework.test.annotation.DirtiesContext;
 import org.springframework.test.annotation.Repeat;
 import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
 
-@SpringApplicationConfiguration(classes=TestConfiguration.class)
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+@SpringApplicationConfiguration(classes = TestConfiguration.class)
 @RunWith(SpringJUnit4ClassRunner.class)
 public class RefreshScopeConcurrencyTests {
 
@@ -71,16 +71,18 @@ public class RefreshScopeConcurrencyTests {
 	@DirtiesContext
 	public void testConcurrentRefresh() throws Exception {
 
-		assertEquals("Hello scope!", service.getMessage());
-		properties.setMessage("Foo");
-		properties.setDelay(500);
+		assertEquals("Hello scope!", this.service.getMessage());
+		this.properties.setMessage("Foo");
+		this.properties.setDelay(500);
 		final CountDownLatch latch = new CountDownLatch(1);
-		Future<String> result = executor.submit(new Callable<String>() {
+		Future<String> result = this.executor.submit(new Callable<String>() {
+			@Override
 			public String call() throws Exception {
 				logger.debug("Background started.");
 				try {
-					return service.getMessage();
-				} finally {
+					return RefreshScopeConcurrencyTests.this.service.getMessage();
+				}
+				finally {
 					latch.countDown();
 					logger.debug("Background done.");
 				}
@@ -88,11 +90,11 @@ public class RefreshScopeConcurrencyTests {
 		});
 		assertTrue(latch.await(1500, TimeUnit.MILLISECONDS));
 		logger.info("Refreshing");
-		scope.refreshAll();
-		assertEquals("Foo", service.getMessage());
+		this.scope.refreshAll();
+		assertEquals("Foo", this.service.getMessage());
 		/*
-		 * This is the most important assertion: we don't want a null value because that means the bean was destroyed
-		 * and not re-initialized before we accessed it.
+		 * This is the most important assertion: we don't want a null value because that
+		 * means the bean was destroyed and not re-initialized before we accessed it.
 		 */
 		assertNotNull(result.get());
 		assertEquals("Hello scope!", result.get());
@@ -104,7 +106,8 @@ public class RefreshScopeConcurrencyTests {
 
 	}
 
-	public static class ExampleService implements Service, InitializingBean, DisposableBean {
+	public static class ExampleService
+			implements Service, InitializingBean, DisposableBean {
 
 		private static Log logger = LogFactory.getLog(ExampleService.class);
 
@@ -115,13 +118,15 @@ public class RefreshScopeConcurrencyTests {
 			this.delay = delay;
 		}
 
+		@Override
 		public void afterPropertiesSet() throws Exception {
-			logger.debug("Initializing message: " + message);
+			logger.debug("Initializing message: " + this.message);
 		}
 
+		@Override
 		public void destroy() throws Exception {
-			logger.debug("Destroying message: " + message);
-			message = null;
+			logger.debug("Destroying message: " + this.message);
+			this.message = null;
 		}
 
 		public void setMessage(String message) {
@@ -129,36 +134,39 @@ public class RefreshScopeConcurrencyTests {
 			this.message = message;
 		}
 
+		@Override
 		public String getMessage() {
-			logger.debug("Getting message: " + message);
+			logger.debug("Getting message: " + this.message);
 			try {
-				Thread.sleep(delay);
-			} catch (InterruptedException e) {
+				Thread.sleep(this.delay);
+			}
+			catch (InterruptedException e) {
 				Thread.currentThread().interrupt();
 			}
-			logger.info("Returning message: " + message);
-			return message;
+			logger.info("Returning message: " + this.message);
+			return this.message;
 		}
 
 	}
-	
+
 	@Configuration
 	@EnableConfigurationProperties(TestProperties.class)
-	@Import({RefreshAutoConfiguration.class, PropertyPlaceholderAutoConfiguration.class})
+	@Import({ RefreshAutoConfiguration.class,
+			PropertyPlaceholderAutoConfiguration.class })
 	protected static class TestConfiguration {
-		
+
 		@Autowired
 		private TestProperties properties;
-		
+
 		@Bean
 		@RefreshScope
 		public ExampleService service() {
 			ExampleService service = new ExampleService();
-			service.setMessage(properties.getMessage());
-			service.setDelay(properties.getDelay());
+			service.setMessage(this.properties.getMessage());
+			service.setDelay(this.properties.getDelay());
 			return service;
 		}
-		
+
 	}
 
 	@ConfigurationProperties
@@ -166,20 +174,24 @@ public class RefreshScopeConcurrencyTests {
 	protected static class TestProperties {
 		private String message;
 		private int delay;
+
 		@ManagedAttribute
 		public String getMessage() {
-			return message;
+			return this.message;
 		}
+
 		public void setMessage(String message) {
 			this.message = message;
 		}
+
 		@ManagedAttribute
 		public int getDelay() {
-			return delay;
+			return this.delay;
 		}
+
 		public void setDelay(int delay) {
 			this.delay = delay;
 		}
 	}
-	
+
 }

spring-cloud-context/src/test/java/org/springframework/cloud/context/scope/refresh/RefreshScopeScaleTests.java

@@ -0,0 +1,195 @@
+/*
+ * Copyright 2006-2007 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.cloud.context.scope.refresh;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.beans.factory.InitializingBean;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.autoconfigure.PropertyPlaceholderAutoConfiguration;
+import org.springframework.boot.context.properties.ConfigurationProperties;
+import org.springframework.boot.context.properties.EnableConfigurationProperties;
+import org.springframework.boot.test.SpringApplicationConfiguration;
+import org.springframework.cloud.autoconfigure.RefreshAutoConfiguration;
+import org.springframework.cloud.context.config.annotation.RefreshScope;
+import org.springframework.cloud.context.scope.refresh.RefreshScopeScaleTests.TestConfiguration;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Import;
+import org.springframework.jmx.export.annotation.ManagedAttribute;
+import org.springframework.jmx.export.annotation.ManagedResource;
+import org.springframework.test.annotation.DirtiesContext;
+import org.springframework.test.annotation.Repeat;
+import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+@SpringApplicationConfiguration(classes = TestConfiguration.class)
+@RunWith(SpringJUnit4ClassRunner.class)
+public class RefreshScopeScaleTests {
+
+	private static Log logger = LogFactory.getLog(RefreshScopeScaleTests.class);
+
+	private ExecutorService executor = Executors.newFixedThreadPool(8);
+
+	@Autowired
+	private ExampleService service;
+
+	@Autowired
+	private TestProperties properties;
+
+	@Test
+	@Repeat(10)
+	@DirtiesContext
+	public void testConcurrentRefresh() throws Exception {
+
+		// overload the thread pool and try to force Spring to create too many instances
+		int n = 80;
+		ExampleService.count = 0;
+		this.properties.setMessage("Foo");
+		this.properties.setDelay(500);
+		final CountDownLatch latch = new CountDownLatch(n);
+		Future<String> result = null;
+		for (int i = 0; i < n; i++) {
+			result = this.executor.submit(new Callable<String>() {
+				@Override
+				public String call() throws Exception {
+					logger.debug("Background started.");
+					try {
+						return RefreshScopeScaleTests.this.service.getMessage();
+					}
+					finally {
+						latch.countDown();
+						logger.debug("Background done.");
+					}
+				}
+			});
+		}
+		assertTrue(latch.await(15000, TimeUnit.MILLISECONDS));
+		assertEquals("Foo", this.service.getMessage());
+		assertNotNull(result.get());
+		assertEquals("Foo", result.get());
+		assertEquals(1, ExampleService.count);
+	}
+
+	public static interface Service {
+
+		String getMessage();
+
+	}
+
+	public static class ExampleService
+			implements Service, InitializingBean, DisposableBean {
+
+		private static Log logger = LogFactory.getLog(ExampleService.class);
+
+		private String message = null;
+		private volatile long delay = 0;
+		private static volatile int count;
+
+		public void setDelay(long delay) {
+			this.delay = delay;
+		}
+
+		@Override
+		public void afterPropertiesSet() throws Exception {
+			ExampleService.count++;
+			try {
+				Thread.sleep(this.delay);
+			}
+			catch (InterruptedException e) {
+				Thread.currentThread().interrupt();
+			}
+			logger.debug("Initializing message: " + this.message);
+		}
+
+		@Override
+		public void destroy() throws Exception {
+			logger.debug("Destroying message: " + this.message);
+			this.message = null;
+		}
+
+		public void setMessage(String message) {
+			logger.debug("Setting message: " + message);
+			this.message = message;
+		}
+
+		@Override
+		public String getMessage() {
+			logger.info("Returning message: " + this.message);
+			return this.message;
+		}
+
+	}
+
+	@Configuration
+	@EnableConfigurationProperties(TestProperties.class)
+	@Import({ RefreshAutoConfiguration.class,
+			PropertyPlaceholderAutoConfiguration.class })
+	protected static class TestConfiguration {
+
+		@Autowired
+		private TestProperties properties;
+
+		@Bean
+		@RefreshScope
+		public ExampleService service() {
+			ExampleService service = new ExampleService();
+			service.setMessage(this.properties.getMessage());
+			service.setDelay(this.properties.getDelay());
+			return service;
+		}
+
+	}
+
+	@ConfigurationProperties
+	@ManagedResource
+	protected static class TestProperties {
+		private String message;
+		private int delay;
+
+		@ManagedAttribute
+		public String getMessage() {
+			return this.message;
+		}
+
+		public void setMessage(String message) {
+			this.message = message;
+		}
+
+		@ManagedAttribute
+		public int getDelay() {
+			return this.delay;
+		}
+
+		public void setDelay(int delay) {
+			this.delay = delay;
+		}
+	}
+
+}