Add a per-bean-name lock around destroy/reset/re-populate/re-initialize steps in rebind

docs/modules/ROOT/pages/spring-cloud-commons/application-context-services.adoc

@@ -182,6 +182,10 @@ The value is a comma-separated list of fully qualified class names or package pr
 spring.cloud.refresh.never-reset-nested-types=com.example.MyClient,com.acme.sdk.
 ----
 
+NOTE: Re-binding mutates the `@ConfigurationProperties` bean's fields in place, destroying and re-initializing the same instance rather than swapping it out for a new one.
+Concurrent rebinds of the same bean are serialized internally, but this does not make the bean safe to read from other threads while a rebind is in progress: a concurrent reader can observe transient, partially-updated state (for example, a property briefly reset to its class-level default before the new value is applied).
+If your application needs a consistent view of a bean's properties across a refresh, use `@RefreshScope` instead, which serializes reads against refreshes for beans in that scope.
+
 Re-binding `@ConfigurationProperties` does not cover another large class of use cases, where you need more control over the refresh and where you need a change to be atomic over the whole `ApplicationContext`.
 To address those concerns, we have `@RefreshScope`.
 

spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java

@@ -26,6 +26,9 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -61,6 +64,17 @@
  * re-initialized, the changes are available immediately to any component that is using
  * the <code>@ConfigurationProperties</code> bean.
  *
+ * <p>
+ * Rebinding a given bean is serialized against other concurrent rebinds of that same
+ * bean, so two overlapping refreshes (for example a manual {@link #rebind(String)} call
+ * racing with an {@link EnvironmentChangeEvent}-triggered {@link #rebind()}) cannot
+ * interleave their destroy/reset/re-initialize steps. That does not, however, make the
+ * bean safe to read concurrently from other threads while a rebind is in progress: the
+ * bean is mutated in place, field by field, so a concurrent reader can observe transient
+ * intermediate state. Beans that need a consistent view across a refresh should use
+ * {@link RefreshScope} instead, which serializes reads against refreshes via a per-bean
+ * {@link java.util.concurrent.locks.ReadWriteLock}.
+ *
  * @author Dave Syer
  * @author Yanming Zhou
  * @see RefreshScope for a deeper and optionally more focused refresh of bean components.
@@ -79,6 +93,8 @@ public class ConfigurationPropertiesRebinder
 
 	private Map<String, Exception> errors = new ConcurrentHashMap<>();
 
+	private final ConcurrentMap<String, Lock> rebindLocks = new ConcurrentHashMap<>();
+
 	private final Set<String> neverResetNestedTypes;
 
 	public ConfigurationPropertiesRebinder(ConfigurationPropertiesBeans beans) {
@@ -151,51 +167,62 @@ public boolean rebind(Class type) {
 	}
 
 	private boolean rebind(String name, ApplicationContext appContext) {
+		// Serialize concurrent rebinds of the *same* bean (for example a manual
+		// rebind(name) racing with an EnvironmentChangeEvent-triggered rebind()) so
+		// their destroy/reset/re-initialize steps cannot interleave on the live bean.
+		// This does not protect concurrent readers of the bean; see the class Javadoc.
+		Lock lock = this.rebindLocks.computeIfAbsent(name, key -> new ReentrantLock());
+		lock.lock();
 		try {
-			Object bean = appContext.getBean(name);
-			if (bean != null) {
-				Class<?> targetClass = AopUtils.getTargetClass(bean);
-				// TODO: determine a more general approach to fix this.
-				// see
-				// https://github.com/spring-cloud/spring-cloud-commons/issues/571
-				if (getNeverRefreshable().contains(targetClass.getName()) || getNeverRefreshable().contains(name)) {
-					return false; // ignore
-				}
-				if (AopUtils.isAopProxy(bean) && bean instanceof Advised advised) {
-					Object target = ProxyUtils.getTargetObject(bean);
-					if (target != bean && !targetClass.isInterface()
-							&& !Modifier.isAbstract(targetClass.getModifiers())) {
-						Object freshBean = appContext.getAutowireCapableBeanFactory().createBean(targetClass);
-						Object freshTarget = AopUtils.isAopProxy(freshBean) ? ProxyUtils.getTargetObject(freshBean)
-								: freshBean;
-						advised.setTargetSource(new SingletonTargetSource(freshTarget));
-						appContext.getAutowireCapableBeanFactory().destroyBean(target);
+			try {
+				Object bean = appContext.getBean(name);
+				if (bean != null) {
+					Class<?> targetClass = AopUtils.getTargetClass(bean);
+					// TODO: determine a more general approach to fix this.
+					// see
+					// https://github.com/spring-cloud/spring-cloud-commons/issues/571
+					if (getNeverRefreshable().contains(targetClass.getName()) || getNeverRefreshable().contains(name)) {
+						return false; // ignore
+					}
+					if (AopUtils.isAopProxy(bean) && bean instanceof Advised advised) {
+						Object target = ProxyUtils.getTargetObject(bean);
+						if (target != bean && !targetClass.isInterface()
+								&& !Modifier.isAbstract(targetClass.getModifiers())) {
+							Object freshBean = appContext.getAutowireCapableBeanFactory().createBean(targetClass);
+							Object freshTarget = AopUtils.isAopProxy(freshBean) ? ProxyUtils.getTargetObject(freshBean)
+									: freshBean;
+							advised.setTargetSource(new SingletonTargetSource(freshTarget));
+							appContext.getAutowireCapableBeanFactory().destroyBean(target);
+						}
+						else {
+							appContext.getAutowireCapableBeanFactory().destroyBean(target);
+							resetBeanToDefaults(target);
+							appContext.getAutowireCapableBeanFactory().autowireBean(target);
+							appContext.getAutowireCapableBeanFactory().initializeBean(target, name);
+						}
 					}
 					else {
-						appContext.getAutowireCapableBeanFactory().destroyBean(target);
-						resetBeanToDefaults(target);
-						appContext.getAutowireCapableBeanFactory().autowireBean(target);
-						appContext.getAutowireCapableBeanFactory().initializeBean(target, name);
+						appContext.getAutowireCapableBeanFactory().destroyBean(bean);
+						resetBeanToDefaults(bean);
+						appContext.getAutowireCapableBeanFactory().autowireBean(bean);
+						appContext.getAutowireCapableBeanFactory().initializeBean(bean, name);
 					}
+					return true;
 				}
-				else {
-					appContext.getAutowireCapableBeanFactory().destroyBean(bean);
-					resetBeanToDefaults(bean);
-					appContext.getAutowireCapableBeanFactory().autowireBean(bean);
-					appContext.getAutowireCapableBeanFactory().initializeBean(bean, name);
-				}
-				return true;
 			}
+			catch (RuntimeException e) {
+				this.errors.put(name, e);
+				throw e;
+			}
+			catch (Exception e) {
+				this.errors.put(name, e);
+				throw new IllegalStateException("Cannot rebind to " + name, e);
+			}
+			return false;
 		}
-		catch (RuntimeException e) {
-			this.errors.put(name, e);
-			throw e;
-		}
-		catch (Exception e) {
-			this.errors.put(name, e);
-			throw new IllegalStateException("Cannot rebind to " + name, e);
+		finally {
+			lock.unlock();
 		}
-		return false;
 	}
 
 	/**

spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderConcurrentRebindIntegrationTests.java

@@ -0,0 +1,149 @@
+/*
+ * Copyright 2012-present 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
+ *
+ *      https://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.properties;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.junit.jupiter.api.Test;
+
+import org.springframework.beans.factory.InitializingBean;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.autoconfigure.context.PropertyPlaceholderAutoConfiguration;
+import org.springframework.boot.context.properties.ConfigurationProperties;
+import org.springframework.boot.context.properties.EnableConfigurationProperties;
+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.cloud.autoconfigure.ConfigurationPropertiesRebinderAutoConfiguration;
+import org.springframework.cloud.autoconfigure.RefreshAutoConfiguration;
+import org.springframework.cloud.context.properties.ConfigurationPropertiesRebinderConcurrentRebindIntegrationTests.TestConfiguration;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Import;
+import org.springframework.test.annotation.DirtiesContext;
+
+import static org.assertj.core.api.BDDAssertions.then;
+
+/**
+ * Verifies that concurrent {@link ConfigurationPropertiesRebinder#rebind(String)} calls
+ * for the <em>same</em> bean are serialized, so their destroy/reset/re-initialize steps
+ * cannot interleave on the live bean. This is a narrower, safely-fixable slice of the
+ * concurrency concerns raised in gh-1709: it does not make the bean safe to read
+ * concurrently while a rebind is in progress (that would require either proxying every
+ * {@code @ConfigurationProperties} bean, as
+ * {@link org.springframework.cloud.context.config.annotation.RefreshScope} already does,
+ * or a breaking change to how such properties are consumed), but it does close a real,
+ * previously entirely-unguarded hole: two threads rebinding the same bean at once (for
+ * example a manual refresh racing with a config-watch-triggered one).
+ *
+ * @author Ryan Baxter
+ */
+@SpringBootTest(classes = TestConfiguration.class)
+public class ConfigurationPropertiesRebinderConcurrentRebindIntegrationTests {
+
+	@Autowired
+	private ConfigurationPropertiesRebinder rebinder;
+
+	@Test
+	@DirtiesContext
+	public void concurrentRebindsOfSameBeanDoNotInterleave() throws Exception {
+		TestProperties.active.set(0);
+		TestProperties.maxActive.set(0);
+		int threadCount = 8;
+		ExecutorService pool = Executors.newFixedThreadPool(threadCount);
+		try {
+			List<Future<?>> futures = new ArrayList<>();
+			for (int i = 0; i < threadCount; i++) {
+				futures.add(pool.submit(() -> this.rebinder.rebind("testProperties")));
+			}
+			for (Future<?> future : futures) {
+				future.get(10, TimeUnit.SECONDS);
+			}
+		}
+		finally {
+			pool.shutdown();
+		}
+		// If rebinds of the same bean were allowed to interleave, more than one thread
+		// would be inside the destroy/re-initialize window at the same time.
+		then(TestProperties.maxActive.get()).isEqualTo(1);
+	}
+
+	@Configuration(proxyBeanMethods = false)
+	@EnableConfigurationProperties
+	@Import({ RefreshConfiguration.RebinderConfiguration.class, PropertyPlaceholderAutoConfiguration.class })
+	protected static class TestConfiguration {
+
+		@Bean
+		protected TestProperties testProperties() {
+			return new TestProperties();
+		}
+
+	}
+
+	// Hack out a protected inner class for testing
+	protected static class RefreshConfiguration extends RefreshAutoConfiguration {
+
+		@Configuration(proxyBeanMethods = false)
+		protected static class RebinderConfiguration extends ConfigurationPropertiesRebinderAutoConfiguration {
+
+			public RebinderConfiguration(ApplicationContext context) {
+				super(context);
+			}
+
+		}
+
+	}
+
+	@ConfigurationProperties("test")
+	protected static class TestProperties implements InitializingBean {
+
+		private static final AtomicInteger active = new AtomicInteger();
+
+		private static final AtomicInteger maxActive = new AtomicInteger();
+
+		private String message = "initial";
+
+		public String getMessage() {
+			return this.message;
+		}
+
+		public void setMessage(String message) {
+			this.message = message;
+		}
+
+		@Override
+		public void afterPropertiesSet() throws Exception {
+			int current = this.active.incrementAndGet();
+			this.maxActive.accumulateAndGet(current, Math::max);
+			try {
+				// Widen the window so overlapping, unserialized rebinds would reliably
+				// collide here rather than depending on unlucky scheduling.
+				Thread.sleep(20);
+			}
+			finally {
+				this.active.decrementAndGet();
+			}
+		}
+
+	}
+
+}