diff --git a/docs/modules/ROOT/pages/spring-cloud-commons/application-context-services.adoc b/docs/modules/ROOT/pages/spring-cloud-commons/application-context-services.adoc index 1a86e051b..4ab5bd593 100644 --- a/docs/modules/ROOT/pages/spring-cloud-commons/application-context-services.adoc +++ b/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`. diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java b/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java index ebb98cca9..d6dcadc8e 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java +++ b/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 @ConfigurationProperties bean. * + *

+ * 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 errors = new ConcurrentHashMap<>(); + private final ConcurrentMap rebindLocks = new ConcurrentHashMap<>(); + private final Set 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; } /** diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderConcurrentRebindIntegrationTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderConcurrentRebindIntegrationTests.java new file mode 100644 index 000000000..02960fc64 --- /dev/null +++ b/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 same 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> 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(); + } + } + + } + +}