From 866eb4718209aafc5cf9bf02f530239a7a3a5978 Mon Sep 17 00:00:00 2001 From: Sharang Gupta Date: Wed, 21 Oct 2026 23:04:06 +0100 Subject: [PATCH 1/5] Bind a fresh instance before resetting a rebound bean Rebinding reset the live bean to its class-level defaults before binding it again, so concurrent readers could observe a nested object as null or a scalar as its unbound default between the two steps. A fresh instance is now bound from the Environment first, through the ConfigurationPropertiesBindingPostProcessor so that no other post-processor or lifecycle callback runs for it, and its values are then copied into the live bean. Readers never see unbound state, removed properties still revert to their defaults, and a failed binding leaves the live bean untouched. See gh-1727 Signed-off-by: Sharang Gupta --- .../ConfigurationPropertiesRebinder.java | 31 +++- ...ebinderTransientStateIntegrationTests.java | 147 ++++++++++++++++++ 2 files changed, 173 insertions(+), 5 deletions(-) create mode 100644 spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java 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 eb6f03704..b7fff8bd6 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 @@ -43,6 +43,7 @@ import org.springframework.beans.BeansException; import org.springframework.beans.factory.BeanFactoryUtils; import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.boot.context.properties.ConfigurationPropertiesBindingPostProcessor; import org.springframework.cloud.autoconfigure.RefreshAutoConfiguration.RefreshProperties; import org.springframework.cloud.context.config.annotation.RefreshScope; import org.springframework.cloud.context.environment.EnvironmentChangeEvent; @@ -208,14 +209,14 @@ private boolean rebind(String name, ApplicationContext appContext) { } else { appContext.getAutowireCapableBeanFactory().destroyBean(target); - resetBeanToDefaults(target); + resetBeanToDefaults(target, name, appContext); appContext.getAutowireCapableBeanFactory().autowireBean(target); appContext.getAutowireCapableBeanFactory().initializeBean(target, name); } } else { appContext.getAutowireCapableBeanFactory().destroyBean(bean); - resetBeanToDefaults(bean); + resetBeanToDefaults(bean, name, appContext); appContext.getAutowireCapableBeanFactory().autowireBean(bean); appContext.getAutowireCapableBeanFactory().initializeBean(bean, name); } @@ -238,10 +239,14 @@ private boolean rebind(String name, ApplicationContext appContext) { } /** - * Reset bean properties to their class-level defaults so that removed properties do - * not retain stale values after rebinding. + * Reset bean properties to the values a freshly bound instance of the bean's class + * holds, so that removed properties do not retain stale values after rebinding while + * concurrent readers never observe the bean's unbound class-level defaults (see + * gh-1727). The fresh instance is bound from the {@link Environment} without being + * initialized as a bean, so no lifecycle callbacks run for it; if binding it fails, + * the live bean is left untouched and the failure surfaces as the rebind error. */ - private void resetBeanToDefaults(Object bean) { + private void resetBeanToDefaults(Object bean, String name, ApplicationContext appContext) { Class targetClass = AopUtils.getTargetClass(bean); if (!hasDefaultConstructor(targetClass)) { // Beans whose no-arg constructor (if any) is not their only constructor @@ -265,9 +270,25 @@ private void resetBeanToDefaults(Object bean) { + " for reset; skipping property reset", ex); return; } + bind(freshInstance, name, appContext); resetProperties(bean, freshInstance, Collections.newSetFromMap(new IdentityHashMap<>()), 0); } + /** + * Bind the given instance from the {@link Environment} as if it were the bean with + * the given name, without running any other bean post-processor or lifecycle + * callback. Without Spring Boot's binding post-processor the instance keeps its + * class-level defaults. + */ + private void bind(Object instance, String name, ApplicationContext appContext) { + ConfigurationPropertiesBindingPostProcessor binder = appContext + .getBeanProvider(ConfigurationPropertiesBindingPostProcessor.class) + .getIfAvailable(); + if (binder != null) { + binder.postProcessBeforeInitialization(instance, name); + } + } + /** * Whether the given type's only declared constructor is a no-argument one, * which is what {@link BeanUtils#instantiateClass(Class)} needs to build a defaults diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java new file mode 100644 index 000000000..74383cef7 --- /dev/null +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java @@ -0,0 +1,147 @@ +/* + * 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 org.junit.jupiter.api.Test; + +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.ConfigurationPropertiesRebinderTransientStateIntegrationTests.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; + +/** + * A rebind must not expose the unbound class-level defaults of a bean to concurrent + * readers: a nested object populated only by binding must never be set to {@code null}, + * and a scalar must never pass through its class-level default on the way to its bound + * value (see gh-1727). + */ +@SpringBootTest(classes = TestConfiguration.class, properties = { "test.timeout.seconds=30", "test.workers=4" }) +public class ConfigurationPropertiesRebinderTransientStateIntegrationTests { + + @Autowired + private TestProperties properties; + + @Autowired + private ConfigurationPropertiesRebinder rebinder; + + @Test + @DirtiesContext + public void rebindDoesNotNullNestedObjectPopulatedByBinding() { + then(this.properties.getTimeout().getSeconds()).isEqualTo(30); + + this.rebinder.rebind(); + + then(this.properties.timeoutAssignments).doesNotContainNull(); + then(this.properties.getTimeout().getSeconds()).isEqualTo(30); + } + + @Test + @DirtiesContext + public void rebindDoesNotSetScalarToClassLevelDefault() { + then(this.properties.getWorkers()).isEqualTo(4); + + this.rebinder.rebind(); + + then(this.properties.workerAssignments).containsOnly(4); + } + + @Configuration(proxyBeanMethods = false) + @EnableConfigurationProperties(RefreshAutoConfiguration.RefreshProperties.class) + @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 { + + final List timeoutAssignments = new ArrayList<>(); + + final List workerAssignments = new ArrayList<>(); + + private Timeout timeout; + + private int workers; + + public Timeout getTimeout() { + return this.timeout; + } + + public void setTimeout(Timeout timeout) { + this.timeoutAssignments.add(timeout); + this.timeout = timeout; + } + + public int getWorkers() { + return this.workers; + } + + public void setWorkers(int workers) { + this.workerAssignments.add(workers); + this.workers = workers; + } + + } + + protected static class Timeout { + + private int seconds; + + public int getSeconds() { + return this.seconds; + } + + public void setSeconds(int seconds) { + this.seconds = seconds; + } + + } + +} From 7dae26e8f1304c6c30e4ed74653faedb979a8275 Mon Sep 17 00:00:00 2001 From: Sharang Gupta Date: Wed, 21 Oct 2026 23:32:47 +0100 Subject: [PATCH 2/5] Bind the fresh instance before destroying the live bean Binding the fresh instance after destroying the live bean left the bean destroyed and never re-initialized when the binding failed. The fresh instance is now created, autowired and bound first, so a failing binding leaves the live bean as it was, and the rebinder's vocabulary no longer talks about class-level defaults where it copies the freshly bound state. See gh-1727 Signed-off-by: Sharang Gupta --- .../ConfigurationPropertiesRebinder.java | 81 +++++++++++-------- ...ebinderTransientStateIntegrationTests.java | 42 +++++++++- 2 files changed, 86 insertions(+), 37 deletions(-) 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 b7fff8bd6..6f45ef728 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 @@ -32,6 +32,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.jspecify.annotations.Nullable; import org.springframework.aop.framework.Advised; import org.springframework.aop.scope.ScopedProxyUtils; @@ -208,15 +209,17 @@ private boolean rebind(String name, ApplicationContext appContext) { appContext.getAutowireCapableBeanFactory().destroyBean(target); } else { + Object freshInstance = bindFreshInstance(target, name, appContext); appContext.getAutowireCapableBeanFactory().destroyBean(target); - resetBeanToDefaults(target, name, appContext); + copyFreshInstance(target, freshInstance); appContext.getAutowireCapableBeanFactory().autowireBean(target); appContext.getAutowireCapableBeanFactory().initializeBean(target, name); } } else { + Object freshInstance = bindFreshInstance(bean, name, appContext); appContext.getAutowireCapableBeanFactory().destroyBean(bean); - resetBeanToDefaults(bean, name, appContext); + copyFreshInstance(bean, freshInstance); appContext.getAutowireCapableBeanFactory().autowireBean(bean); appContext.getAutowireCapableBeanFactory().initializeBean(bean, name); } @@ -239,14 +242,14 @@ private boolean rebind(String name, ApplicationContext appContext) { } /** - * Reset bean properties to the values a freshly bound instance of the bean's class - * holds, so that removed properties do not retain stale values after rebinding while - * concurrent readers never observe the bean's unbound class-level defaults (see - * gh-1727). The fresh instance is bound from the {@link Environment} without being - * initialized as a bean, so no lifecycle callbacks run for it; if binding it fails, - * the live bean is left untouched and the failure surfaces as the rebind error. + * Create a fresh instance of the bean's class, autowire it and bind it from the + * {@link Environment}, so that its state can replace the live bean's state once the + * live bean has been destroyed. This happens before the live bean is touched: a + * binding failure surfaces as the rebind error and leaves the live bean as it was. + * Returns {@code null} when no trustworthy fresh instance can be created; the live + * bean is then re-bound in place and removed properties keep their stale values. */ - private void resetBeanToDefaults(Object bean, String name, ApplicationContext appContext) { + private @Nullable Object bindFreshInstance(Object bean, String name, ApplicationContext appContext) { Class targetClass = AopUtils.getTargetClass(bean); if (!hasDefaultConstructor(targetClass)) { // Beans whose no-arg constructor (if any) is not their only constructor @@ -259,7 +262,7 @@ private void resetBeanToDefaults(Object bean, String name, ApplicationContext ap logger.debug("No default constructor for " + targetClass.getName() + "; skipping property reset before rebinding"); } - return; + return null; } Object freshInstance; try { @@ -268,17 +271,29 @@ private void resetBeanToDefaults(Object bean, String name, ApplicationContext ap catch (Exception ex) { logger.warn("Cannot create default instance of " + targetClass.getName() + " for reset; skipping property reset", ex); - return; + return null; } + appContext.getAutowireCapableBeanFactory().autowireBean(freshInstance); bind(freshInstance, name, appContext); - resetProperties(bean, freshInstance, Collections.newSetFromMap(new IdentityHashMap<>()), 0); + return freshInstance; + } + + /** + * Replace the properties of the live bean with those of the freshly bound instance, + * so that removed properties revert to their defaults while concurrent readers never + * observe the bean's unbound class-level defaults (see gh-1727). + */ + private void copyFreshInstance(Object bean, @Nullable Object freshInstance) { + if (freshInstance != null) { + resetProperties(bean, freshInstance, Collections.newSetFromMap(new IdentityHashMap<>()), 0); + } } /** * Bind the given instance from the {@link Environment} as if it were the bean with - * the given name, without running any other bean post-processor or lifecycle - * callback. Without Spring Boot's binding post-processor the instance keeps its - * class-level defaults. + * the given name, through Spring Boot's binding post-processor only, so that no other + * post-processor or lifecycle callback runs for it. Without that post-processor the + * instance keeps its class-level defaults. */ private void bind(Object instance, String name, ApplicationContext appContext) { ConfigurationPropertiesBindingPostProcessor binder = appContext @@ -311,7 +326,7 @@ private boolean hasDefaultConstructor(Class type) { return constructors.length == 1 && constructors[0].getParameterCount() == 0; } - private void resetProperties(Object bean, Object defaults, Set visited, int depth) { + private void resetProperties(Object bean, Object freshInstance, Set visited, int depth) { // Guard against cyclic object graphs and pathologically deep ones so that // recursion // always terminates; see MAX_RESET_DEPTH. @@ -319,7 +334,7 @@ private void resetProperties(Object bean, Object defaults, Set visited, return; } BeanWrapper target = new BeanWrapperImpl(bean); - BeanWrapper defaultsWrapper = new BeanWrapperImpl(defaults); + BeanWrapper freshWrapper = new BeanWrapperImpl(freshInstance); for (PropertyDescriptor pd : target.getPropertyDescriptors()) { String propertyName = pd.getName(); if ("class".equals(propertyName)) { @@ -328,16 +343,16 @@ private void resetProperties(Object bean, Object defaults, Set visited, try { // A property whose declared type the user excluded is left alone whatever // its accessors look like: a read-only property is not descended into and - // a writable one is not overwritten with the default instance's value. + // a writable one is not overwritten with the fresh instance's value. if (isNeverReset(pd.getPropertyType())) { continue; } - if (target.isWritableProperty(propertyName) && defaultsWrapper.isReadableProperty(propertyName)) { + if (target.isWritableProperty(propertyName) && freshWrapper.isReadableProperty(propertyName)) { if (target.isReadableProperty(propertyName) && isNeverReset(target.getPropertyValue(propertyName))) { continue; } - Object defaultValue = defaultsWrapper.getPropertyValue(propertyName); + Object freshValue = freshWrapper.getPropertyValue(propertyName); Object currentValue = target.isReadableProperty(propertyName) ? target.getPropertyValue(propertyName) : null; // Skip the setter call entirely when there's nothing to change: this @@ -345,23 +360,23 @@ && isNeverReset(target.getPropertyValue(propertyName))) { // again with the same value, and setters that reject null are only // ever // invoked when the property actually needs to become null. - if (!ObjectUtils.nullSafeEquals(currentValue, defaultValue)) { + if (!ObjectUtils.nullSafeEquals(currentValue, freshValue)) { try { - target.setPropertyValue(propertyName, defaultValue); + target.setPropertyValue(propertyName, freshValue); } catch (Exception ex) { warnCannotReset(bean, propertyName, ex); } } } - else if (target.isReadableProperty(propertyName) && defaultsWrapper.isReadableProperty(propertyName)) { + else if (target.isReadableProperty(propertyName) && freshWrapper.isReadableProperty(propertyName)) { Object value = target.getPropertyValue(propertyName); - Object defaultValue = defaultsWrapper.getPropertyValue(propertyName); + Object freshValue = freshWrapper.getPropertyValue(propertyName); if (value instanceof Collection collection) { try { collection.clear(); - if (defaultValue instanceof Collection defaultCollection) { - collection.addAll(defaultCollection); + if (freshValue instanceof Collection freshCollection) { + collection.addAll(freshCollection); } } catch (UnsupportedOperationException ex) { @@ -371,17 +386,17 @@ else if (target.isReadableProperty(propertyName) && defaultsWrapper.isReadablePr else if (value instanceof Map map) { try { map.clear(); - if (defaultValue instanceof Map defaultMap) { - map.putAll(defaultMap); + if (freshValue instanceof Map freshMap) { + map.putAll(freshMap); } } catch (UnsupportedOperationException ex) { warnCannotReset(bean, propertyName, ex); } } - else if (value != null && defaultValue != null && isResettableNestedType(value.getClass()) + else if (value != null && freshValue != null && isResettableNestedType(value.getClass()) && isStableInstance(target, propertyName, value)) { - resetProperties(value, defaultValue, visited, depth + 1); + resetProperties(value, freshValue, visited, depth + 1); } } } @@ -412,9 +427,9 @@ private boolean isStableInstance(BeanWrapper target, String propertyName, Object /** * Log, at most once per bean type and property, that a property could not be reset to - * its default value and may therefore retain a stale value after this and future - * refreshes. The full exception is only logged at DEBUG, on every occurrence, to - * avoid spamming the log at the default level while still making the failure + * its freshly bound value and may therefore retain a stale value after this and + * future refreshes. The full exception is only logged at DEBUG, on every occurrence, + * to avoid spamming the log at the default level while still making the failure * discoverable. */ private void warnCannotReset(Object bean, String propertyName, Exception ex) { diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java index 74383cef7..396522e3f 100644 --- a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java @@ -18,12 +18,14 @@ import java.util.ArrayList; import java.util.List; +import java.util.Map; import org.junit.jupiter.api.Test; 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.ConfigurationPropertiesBindException; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.cloud.autoconfigure.ConfigurationPropertiesRebinderAutoConfiguration; @@ -33,15 +35,20 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; +import org.springframework.core.env.ConfigurableEnvironment; +import org.springframework.core.env.PropertySource; import org.springframework.test.annotation.DirtiesContext; import static org.assertj.core.api.BDDAssertions.then; +import static org.assertj.core.api.BDDAssertions.thenThrownBy; /** - * A rebind must not expose the unbound class-level defaults of a bean to concurrent - * readers: a nested object populated only by binding must never be set to {@code null}, - * and a scalar must never pass through its class-level default on the way to its bound - * value (see gh-1727). + * A rebind must never pass the live bean through its unbound class-level defaults, which + * is what a concurrent reader would observe (see gh-1727): a nested object populated only + * by binding must never be set to {@code null}, a scalar must never be set to its + * class-level default on the way to its bound value, and a rebind whose binding fails + * must leave the bean as it was. The tests check the values the bean's setters receive + * during a rebind. */ @SpringBootTest(classes = TestConfiguration.class, properties = { "test.timeout.seconds=30", "test.workers=4" }) public class ConfigurationPropertiesRebinderTransientStateIntegrationTests { @@ -52,6 +59,9 @@ public class ConfigurationPropertiesRebinderTransientStateIntegrationTests { @Autowired private ConfigurationPropertiesRebinder rebinder; + @Autowired + private ConfigurableEnvironment environment; + @Test @DirtiesContext public void rebindDoesNotNullNestedObjectPopulatedByBinding() { @@ -73,6 +83,30 @@ public void rebindDoesNotSetScalarToClassLevelDefault() { then(this.properties.workerAssignments).containsOnly(4); } + @Test + @DirtiesContext + public void failedRebindLeavesTheBeanUntouched() { + then(this.properties.getWorkers()).isEqualTo(4); + findTestProperties().put("test.workers", "not-a-number"); + + thenThrownBy(() -> this.rebinder.rebind()).isInstanceOf(ConfigurationPropertiesBindException.class); + + then(this.properties.workerAssignments).containsOnly(4); + then(this.properties.getWorkers()).isEqualTo(4); + then(this.properties.getTimeout().getSeconds()).isEqualTo(30); + } + + private Map findTestProperties() { + for (PropertySource source : this.environment.getPropertySources()) { + if (source.getName().toLowerCase().contains("test")) { + @SuppressWarnings("unchecked") + Map map = (Map) source.getSource(); + return map; + } + } + throw new IllegalStateException("Could not find test property source"); + } + @Configuration(proxyBeanMethods = false) @EnableConfigurationProperties(RefreshAutoConfiguration.RefreshProperties.class) @Import({ RefreshConfiguration.RebinderConfiguration.class, PropertyPlaceholderAutoConfiguration.class }) From 31094dc28a49491c461389449e77b93ad0df7665 Mon Sep 17 00:00:00 2001 From: Sharang Gupta Date: Thu, 22 Oct 2026 16:24:19 +0100 Subject: [PATCH 3/5] Keep getter-only collections populated during a rebind Following review: a getter-only collection or map is brought to the contents of the fresh instance without being emptied first (equal contents are left alone, new elements are added before the previous ones are removed), the reset is skipped when no ConfigurationPropertiesBindingPostProcessor is available instead of copying class-level defaults onto the live bean, and the autowiring of the fresh instance is explained in place. Signed-off-by: Sharang Gupta --- .../ConfigurationPropertiesRebinder.java | 102 ++++++++++++------ ...ebinderTransientStateIntegrationTests.java | 66 +++++++++++- 2 files changed, 135 insertions(+), 33 deletions(-) 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 6f45ef728..92c1e43c9 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 @@ -23,6 +23,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.IdentityHashMap; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -73,8 +74,12 @@ * 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 + * bean is mutated in place, field by field, so a concurrent reader can observe a mix of + * previous and new values. What it does not observe is unbound state: a fresh instance is + * bound first and its values are then copied into the live bean, so a scalar never passes + * through its class-level default, a nested object never through {@code null}, and a + * getter-only collection or map is brought to its new contents without being emptied + * first. 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}. * @@ -250,6 +255,18 @@ private boolean rebind(String name, ApplicationContext appContext) { * bean is then re-bound in place and removed properties keep their stale values. */ private @Nullable Object bindFreshInstance(Object bean, String name, ApplicationContext appContext) { + ConfigurationPropertiesBindingPostProcessor binder = appContext + .getBeanProvider(ConfigurationPropertiesBindingPostProcessor.class) + .getIfAvailable(); + if (binder == null) { + // Without the binder the fresh instance would only carry its class-level + // defaults, and copying those onto the live bean would discard its values. + if (logger.isDebugEnabled()) { + logger.debug("No ConfigurationPropertiesBindingPostProcessor available; skipping property reset " + + "before rebinding"); + } + return null; + } Class targetClass = AopUtils.getTargetClass(bean); if (!hasDefaultConstructor(targetClass)) { // Beans whose no-arg constructor (if any) is not their only constructor @@ -273,8 +290,10 @@ private boolean rebind(String name, ApplicationContext appContext) { + " for reset; skipping property reset", ex); return null; } + // Autowired so that an injected dependency is copied over as the same instance + // rather than nulled and re-injected on the live bean afterwards. appContext.getAutowireCapableBeanFactory().autowireBean(freshInstance); - bind(freshInstance, name, appContext); + binder.postProcessBeforeInitialization(freshInstance, name); return freshInstance; } @@ -295,15 +314,6 @@ private void copyFreshInstance(Object bean, @Nullable Object freshInstance) { * post-processor or lifecycle callback runs for it. Without that post-processor the * instance keeps its class-level defaults. */ - private void bind(Object instance, String name, ApplicationContext appContext) { - ConfigurationPropertiesBindingPostProcessor binder = appContext - .getBeanProvider(ConfigurationPropertiesBindingPostProcessor.class) - .getIfAvailable(); - if (binder != null) { - binder.postProcessBeforeInitialization(instance, name); - } - } - /** * Whether the given type's only declared constructor is a no-argument one, * which is what {@link BeanUtils#instantiateClass(Class)} needs to build a defaults @@ -373,26 +383,10 @@ else if (target.isReadableProperty(propertyName) && freshWrapper.isReadablePrope Object value = target.getPropertyValue(propertyName); Object freshValue = freshWrapper.getPropertyValue(propertyName); if (value instanceof Collection collection) { - try { - collection.clear(); - if (freshValue instanceof Collection freshCollection) { - collection.addAll(freshCollection); - } - } - catch (UnsupportedOperationException ex) { - warnCannotReset(bean, propertyName, ex); - } + replaceContents(bean, propertyName, collection, freshValue); } else if (value instanceof Map map) { - try { - map.clear(); - if (freshValue instanceof Map freshMap) { - map.putAll(freshMap); - } - } - catch (UnsupportedOperationException ex) { - warnCannotReset(bean, propertyName, ex); - } + replaceContents(bean, propertyName, map, freshValue); } else if (value != null && freshValue != null && isResettableNestedType(value.getClass()) && isStableInstance(target, propertyName, value)) { @@ -416,6 +410,54 @@ && isStableInstance(target, propertyName, value)) { * walk a throw-away object graph - one that, for record-style/fluent accessors, may * not even terminate (see gh-1750). */ + /** + * Bring a getter-only collection to the contents of the fresh instance without ever + * leaving it empty: nothing is touched when the contents are already equal, and + * otherwise the new elements are added before the previous ones are removed. + */ + @SuppressWarnings({ "unchecked", "rawtypes" }) + private void replaceContents(Object bean, String propertyName, Collection collection, @Nullable Object freshValue) { + Collection fresh = (freshValue instanceof Collection freshCollection) ? freshCollection + : Collections.emptyList(); + if (collection.equals(fresh)) { + return; + } + try { + if (collection instanceof List list) { + int previousSize = list.size(); + list.addAll(fresh); + list.subList(0, previousSize).clear(); + } + else { + collection.retainAll(fresh); + collection.addAll(fresh); + } + } + catch (UnsupportedOperationException ex) { + warnCannotReset(bean, propertyName, ex); + } + } + + /** + * Bring a getter-only map to the contents of the fresh instance without ever leaving + * it empty: nothing is touched when the contents are already equal, and otherwise + * only the removed keys are dropped before the fresh entries are put. + */ + @SuppressWarnings({ "unchecked", "rawtypes" }) + private void replaceContents(Object bean, String propertyName, Map map, @Nullable Object freshValue) { + Map fresh = (freshValue instanceof Map freshMap) ? freshMap : Collections.emptyMap(); + if (map.equals(fresh)) { + return; + } + try { + map.keySet().retainAll(fresh.keySet()); + map.putAll(fresh); + } + catch (UnsupportedOperationException ex) { + warnCannotReset(bean, propertyName, ex); + } + } + private boolean isStableInstance(BeanWrapper target, String propertyName, Object value) { try { return target.getPropertyValue(propertyName) == value; diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java index 396522e3f..97b380395 100644 --- a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java @@ -17,6 +17,7 @@ package org.springframework.cloud.context.properties; import java.util.ArrayList; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -39,6 +40,7 @@ import org.springframework.core.env.PropertySource; import org.springframework.test.annotation.DirtiesContext; +import static org.assertj.core.api.Assertions.entry; import static org.assertj.core.api.BDDAssertions.then; import static org.assertj.core.api.BDDAssertions.thenThrownBy; @@ -47,10 +49,12 @@ * is what a concurrent reader would observe (see gh-1727): a nested object populated only * by binding must never be set to {@code null}, a scalar must never be set to its * class-level default on the way to its bound value, and a rebind whose binding fails - * must leave the bean as it was. The tests check the values the bean's setters receive - * during a rebind. + * must leave the bean as it was, and a getter-only collection or map must never be + * emptied on the way to its new contents. The tests check the values the bean's setters + * receive during a rebind and the operations its collections see. */ -@SpringBootTest(classes = TestConfiguration.class, properties = { "test.timeout.seconds=30", "test.workers=4" }) +@SpringBootTest(classes = TestConfiguration.class, properties = { "test.timeout.seconds=30", "test.workers=4", + "test.tags[0]=a", "test.tags[1]=b", "test.labels.one=1", "test.labels.two=2" }) public class ConfigurationPropertiesRebinderTransientStateIntegrationTests { @Autowired @@ -96,6 +100,37 @@ public void failedRebindLeavesTheBeanUntouched() { then(this.properties.getTimeout().getSeconds()).isEqualTo(30); } + @Test + @DirtiesContext + public void rebindBringsGetterOnlyMapToNewContentsWithoutEmptyingIt() { + then(this.properties.getLabels()).containsOnly(entry("one", "1"), entry("two", "2")); + findTestProperties().remove("test.labels.two"); + findTestProperties().put("test.labels.three", "3"); + this.rebinder.rebind(); + then(this.properties.getLabels()).containsOnly(entry("one", "1"), entry("three", "3")); + then(this.properties.getLabels().clears).isZero(); + } + + @Test + @DirtiesContext + public void rebindBringsGetterOnlyListToNewContents() { + then(this.properties.getTags()).containsExactly("a", "b"); + findTestProperties().put("test.tags[0]", "c"); + findTestProperties().remove("test.tags[1]"); + this.rebinder.rebind(); + then(this.properties.getTags()).containsExactly("c"); + } + + @Test + @DirtiesContext + public void rebindEmptiesRemovedGetterOnlyList() { + then(this.properties.getTags()).containsExactly("a", "b"); + findTestProperties().remove("test.tags[0]"); + findTestProperties().remove("test.tags[1]"); + this.rebinder.rebind(); + then(this.properties.getTags()).isEmpty(); + } + private Map findTestProperties() { for (PropertySource source : this.environment.getPropertySources()) { if (source.getName().toLowerCase().contains("test")) { @@ -140,6 +175,10 @@ protected static class TestProperties { final List workerAssignments = new ArrayList<>(); + private final List tags = new ArrayList<>(); + + private final TrackingMap labels = new TrackingMap(); + private Timeout timeout; private int workers; @@ -162,6 +201,27 @@ public void setWorkers(int workers) { this.workers = workers; } + public List getTags() { + return this.tags; + } + + public TrackingMap getLabels() { + return this.labels; + } + + } + + @SuppressWarnings("serial") + protected static class TrackingMap extends LinkedHashMap { + + int clears; + + @Override + public void clear() { + this.clears++; + super.clear(); + } + } protected static class Timeout { From 037d96edd98a64ac2a8f19c8b4c4d28a219eb529 Mon Sep 17 00:00:00 2001 From: Sharang Gupta Date: Thu, 22 Oct 2026 20:43:10 +0100 Subject: [PATCH 4/5] Re-initialize a rebound bean without binding it in place again Following review: the live bean's state now comes from the bound fresh instance only. After the copy the bean is destroyed, re-autowired and re-initialized through its post-processors, InitializingBean and init methods, but not through ConfigurationPropertiesBindingPostProcessor, whose in-place bind cleared and refilled every getter-only collection on every rebind. A list is updated element by element so that a reader iterating over it is not disturbed unless its size changes. The fresh instance is no longer autowired; the copy skips injected setters and fields instead, so a dependency is neither nulled nor re-registered through a throwaway instance. Tests cover an unchanged, a changed, a shortened and a removed list (no clear() after startup), a reader thread iterating during fifty rebinds, and an @Autowired setter kept across a rebind. Signed-off-by: Sharang Gupta --- .../ConfigurationPropertiesRebinder.java | 212 +++++++++++++++--- ...ebinderTransientStateIntegrationTests.java | 107 ++++++++- 2 files changed, 273 insertions(+), 46 deletions(-) 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 92c1e43c9..f91e58cae 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 @@ -17,12 +17,16 @@ package org.springframework.cloud.context.properties; import java.beans.PropertyDescriptor; +import java.lang.annotation.Annotation; import java.lang.reflect.Constructor; +import java.lang.reflect.Field; +import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.IdentityHashMap; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; @@ -44,6 +48,12 @@ import org.springframework.beans.BeanWrapperImpl; import org.springframework.beans.BeansException; import org.springframework.beans.factory.BeanFactoryUtils; +import org.springframework.beans.factory.InitializingBean; +import org.springframework.beans.factory.NoSuchBeanDefinitionException; +import org.springframework.beans.factory.config.AutowireCapableBeanFactory; +import org.springframework.beans.factory.config.BeanPostProcessor; +import org.springframework.beans.factory.support.AbstractBeanDefinition; +import org.springframework.beans.factory.support.AbstractBeanFactory; import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.boot.context.properties.ConfigurationPropertiesBindingPostProcessor; import org.springframework.cloud.autoconfigure.RefreshAutoConfiguration.RefreshProperties; @@ -59,6 +69,7 @@ import org.springframework.jmx.export.annotation.ManagedResource; import org.springframework.stereotype.Component; import org.springframework.util.ObjectUtils; +import org.springframework.util.ReflectionUtils; import org.springframework.util.StringUtils; /** @@ -79,7 +90,8 @@ * bound first and its values are then copied into the live bean, so a scalar never passes * through its class-level default, a nested object never through {@code null}, and a * getter-only collection or map is brought to its new contents without being emptied - * first. Beans that need a consistent view across a refresh should use + * first (the bean is destroyed, re-autowired and re-initialized afterwards, but not bound + * in place a second time). 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}. * @@ -116,6 +128,11 @@ public class ConfigurationPropertiesRebinder */ private static final int MAX_RESET_DEPTH = 25; + private static final Set INJECTION_ANNOTATIONS = Set.of( + "org.springframework.beans.factory.annotation.Autowired", + "org.springframework.beans.factory.annotation.Value", "jakarta.inject.Inject", "javax.inject.Inject", + "jakarta.annotation.Resource", "javax.annotation.Resource"); + public ConfigurationPropertiesRebinder(ConfigurationPropertiesBeans beans) { this(beans, Collections.emptySet()); } @@ -214,19 +231,11 @@ private boolean rebind(String name, ApplicationContext appContext) { appContext.getAutowireCapableBeanFactory().destroyBean(target); } else { - Object freshInstance = bindFreshInstance(target, name, appContext); - appContext.getAutowireCapableBeanFactory().destroyBean(target); - copyFreshInstance(target, freshInstance); - appContext.getAutowireCapableBeanFactory().autowireBean(target); - appContext.getAutowireCapableBeanFactory().initializeBean(target, name); + rebindInPlace(target, name, appContext); } } else { - Object freshInstance = bindFreshInstance(bean, name, appContext); - appContext.getAutowireCapableBeanFactory().destroyBean(bean); - copyFreshInstance(bean, freshInstance); - appContext.getAutowireCapableBeanFactory().autowireBean(bean); - appContext.getAutowireCapableBeanFactory().initializeBean(bean, name); + rebindInPlace(bean, name, appContext); } return true; } @@ -254,6 +263,80 @@ private boolean rebind(String name, ApplicationContext appContext) { * Returns {@code null} when no trustworthy fresh instance can be created; the live * bean is then re-bound in place and removed properties keep their stale values. */ + /** + * Rebind the live bean in place. Its new state is bound on a fresh instance first and + * then copied over, so that readers never observe unbound state; the bean is then + * destroyed, re-autowired and re-initialized, without being bound in place a second + * time. A bean that cannot be reset from a fresh instance is destroyed and + * re-initialized as before, which binds it in place. + */ + private void rebindInPlace(Object bean, String name, ApplicationContext appContext) throws Exception { + Object freshInstance = bindFreshInstance(bean, name, appContext); + AutowireCapableBeanFactory beanFactory = appContext.getAutowireCapableBeanFactory(); + beanFactory.destroyBean(bean); + if (freshInstance == null) { + beanFactory.autowireBean(bean); + beanFactory.initializeBean(bean, name); + return; + } + copyFreshInstance(bean, freshInstance); + beanFactory.autowireBean(bean); + initializeWithoutBinding(bean, name, beanFactory); + } + + /** + * Run the initialization callbacks of the live bean again, as + * {@link AutowireCapableBeanFactory#initializeBean} would, but without + * {@link ConfigurationPropertiesBindingPostProcessor}: the bean already holds the + * state bound on the fresh instance, and binding it in place would clear and refill + * its getter-only collections (Spring Boot's {@code CollectionBinder} merges into an + * existing collection that way), which is exactly the kind of transient state this + * class avoids. + */ + private void initializeWithoutBinding(Object bean, String name, AutowireCapableBeanFactory beanFactory) + throws Exception { + if (!(beanFactory instanceof AbstractBeanFactory abstractBeanFactory)) { + beanFactory.initializeBean(bean, name); + return; + } + for (BeanPostProcessor processor : abstractBeanFactory.getBeanPostProcessors()) { + if (!(processor instanceof ConfigurationPropertiesBindingPostProcessor) + && processor.postProcessBeforeInitialization(bean, name) == null) { + return; + } + } + if (bean instanceof InitializingBean initializingBean) { + initializingBean.afterPropertiesSet(); + } + invokeInitMethods(bean, name, abstractBeanFactory); + for (BeanPostProcessor processor : abstractBeanFactory.getBeanPostProcessors()) { + if (processor.postProcessAfterInitialization(bean, name) == null) { + return; + } + } + } + + private void invokeInitMethods(Object bean, String name, AbstractBeanFactory beanFactory) { + String[] initMethodNames; + try { + initMethodNames = (beanFactory.getMergedBeanDefinition(name) instanceof AbstractBeanDefinition definition) + ? definition.getInitMethodNames() : null; + } + catch (NoSuchBeanDefinitionException ex) { + return; + } + if (initMethodNames == null) { + return; + } + for (String initMethodName : initMethodNames) { + Method initMethod = ReflectionUtils.findMethod(AopUtils.getTargetClass(bean), initMethodName); + if (initMethod != null) { + ReflectionUtils.makeAccessible(initMethod); + ReflectionUtils.invokeMethod(initMethod, bean); + } + } + } + private @Nullable Object bindFreshInstance(Object bean, String name, ApplicationContext appContext) { ConfigurationPropertiesBindingPostProcessor binder = appContext .getBeanProvider(ConfigurationPropertiesBindingPostProcessor.class) @@ -290,9 +373,6 @@ private boolean rebind(String name, ApplicationContext appContext) { + " for reset; skipping property reset", ex); return null; } - // Autowired so that an injected dependency is copied over as the same instance - // rather than nulled and re-injected on the live bean afterwards. - appContext.getAutowireCapableBeanFactory().autowireBean(freshInstance); binder.postProcessBeforeInitialization(freshInstance, name); return freshInstance; } @@ -308,12 +388,6 @@ private void copyFreshInstance(Object bean, @Nullable Object freshInstance) { } } - /** - * Bind the given instance from the {@link Environment} as if it were the bean with - * the given name, through Spring Boot's binding post-processor only, so that no other - * post-processor or lifecycle callback runs for it. Without that post-processor the - * instance keeps its class-level defaults. - */ /** * Whether the given type's only declared constructor is a no-argument one, * which is what {@link BeanUtils#instantiateClass(Class)} needs to build a defaults @@ -357,14 +431,20 @@ private void resetProperties(Object bean, Object freshInstance, Set visi if (isNeverReset(pd.getPropertyType())) { continue; } - if (target.isWritableProperty(propertyName) && freshWrapper.isReadableProperty(propertyName)) { - if (target.isReadableProperty(propertyName) - && isNeverReset(target.getPropertyValue(propertyName))) { + if (target.isWritableProperty(propertyName) + && (freshWrapper.isReadableProperty(propertyName) || hasField(bean, propertyName))) { + // A setter that is injected (for example @Autowired or @Value) is not + // a + // bound property: the fresh instance is not autowired, so copying it + // would null the dependency before it is re-injected. + if (isInjected(bean, pd) || (target.isReadableProperty(propertyName) + && isNeverReset(target.getPropertyValue(propertyName)))) { continue; } - Object freshValue = freshWrapper.getPropertyValue(propertyName); - Object currentValue = target.isReadableProperty(propertyName) - ? target.getPropertyValue(propertyName) : null; + // A write-only property (a setter without a getter, which the binder + // still binds) is read from its backing field on both instances. + Object freshValue = readProperty(freshWrapper, freshInstance, propertyName); + Object currentValue = readProperty(target, bean, propertyName); // Skip the setter call entirely when there's nothing to change: this // avoids needlessly invoking setters that don't tolerate being called // again with the same value, and setters that reject null are only @@ -410,10 +490,21 @@ && isStableInstance(target, propertyName, value)) { * walk a throw-away object graph - one that, for record-style/fluent accessors, may * not even terminate (see gh-1750). */ + private boolean isStableInstance(BeanWrapper target, String propertyName, Object value) { + try { + return target.getPropertyValue(propertyName) == value; + } + catch (Exception ex) { + return false; + } + } + /** * Bring a getter-only collection to the contents of the fresh instance without ever - * leaving it empty: nothing is touched when the contents are already equal, and - * otherwise the new elements are added before the previous ones are removed. + * leaving it empty: nothing is touched when the contents are already equal; a list is + * updated element by element, so a reader iterating over it is not disturbed unless + * its size changes; any other collection drops only the removed elements before the + * new ones are added. */ @SuppressWarnings({ "unchecked", "rawtypes" }) private void replaceContents(Object bean, String propertyName, Collection collection, @Nullable Object freshValue) { @@ -424,9 +515,7 @@ private void replaceContents(Object bean, String propertyName, Collection collec } try { if (collection instanceof List list) { - int previousSize = list.size(); - list.addAll(fresh); - list.subList(0, previousSize).clear(); + replaceElements(list, fresh); } else { collection.retainAll(fresh); @@ -438,6 +527,27 @@ private void replaceContents(Object bean, String propertyName, Collection collec } } + @SuppressWarnings({ "unchecked", "rawtypes" }) + private static void replaceElements(List list, Collection fresh) { + int index = 0; + Iterator iterator = fresh.iterator(); + while (iterator.hasNext() && index < list.size()) { + Object element = iterator.next(); + if (!ObjectUtils.nullSafeEquals(list.get(index), element)) { + list.set(index, element); + } + index++; + } + if (iterator.hasNext()) { + while (iterator.hasNext()) { + list.add(iterator.next()); + } + } + else if (index < list.size()) { + list.subList(index, list.size()).clear(); + } + } + /** * Bring a getter-only map to the contents of the fresh instance without ever leaving * it empty: nothing is touched when the contents are already equal, and otherwise @@ -458,13 +568,43 @@ private void replaceContents(Object bean, String propertyName, Map map, @Nullabl } } - private boolean isStableInstance(BeanWrapper target, String propertyName, Object value) { - try { - return target.getPropertyValue(propertyName) == value; + private static boolean hasField(Object bean, String propertyName) { + return ReflectionUtils.findField(AopUtils.getTargetClass(bean), propertyName) != null; + } + + private static @Nullable Object readProperty(BeanWrapper wrapper, Object instance, String propertyName) { + if (wrapper.isReadableProperty(propertyName)) { + return wrapper.getPropertyValue(propertyName); } - catch (Exception ex) { - return false; + Field field = ReflectionUtils.findField(AopUtils.getTargetClass(instance), propertyName); + if (field == null) { + return null; } + ReflectionUtils.makeAccessible(field); + return ReflectionUtils.getField(field, instance); + } + + /** + * Whether the given property is a dependency injection point (an annotated setter or + * backing field, for example {@code @Autowired} or {@code @Value}) rather than a + * bound property. + */ + private boolean isInjected(Object bean, PropertyDescriptor pd) { + Method writeMethod = pd.getWriteMethod(); + if (writeMethod != null && isInjected(writeMethod.getAnnotations())) { + return true; + } + Field field = ReflectionUtils.findField(AopUtils.getTargetClass(bean), pd.getName()); + return field != null && isInjected(field.getAnnotations()); + } + + private static boolean isInjected(Annotation[] annotations) { + for (Annotation annotation : annotations) { + if (INJECTION_ANNOTATIONS.contains(annotation.annotationType().getName())) { + return true; + } + } + return false; } /** diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java index 97b380395..14a0fdb60 100644 --- a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTransientStateIntegrationTests.java @@ -16,10 +16,16 @@ package org.springframework.cloud.context.properties; +import java.time.Clock; import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +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.AtomicBoolean; import org.junit.jupiter.api.Test; @@ -113,22 +119,71 @@ public void rebindBringsGetterOnlyMapToNewContentsWithoutEmptyingIt() { @Test @DirtiesContext - public void rebindBringsGetterOnlyListToNewContents() { - then(this.properties.getTags()).containsExactly("a", "b"); + public void rebindNeverClearsGetterOnlyList() { + TrackingList tags = this.properties.getTags(); + then(tags).containsExactly("a", "b"); + int clearsAtStartup = tags.clears; + this.rebinder.rebind(); + then(tags).containsExactly("a", "b"); + then(tags.clears).as("unchanged rebind").isEqualTo(clearsAtStartup); findTestProperties().put("test.tags[0]", "c"); + this.rebinder.rebind(); + then(tags).containsExactly("c", "b"); + then(tags.clears).as("changed rebind").isEqualTo(clearsAtStartup); findTestProperties().remove("test.tags[1]"); this.rebinder.rebind(); - then(this.properties.getTags()).containsExactly("c"); + then(tags).containsExactly("c"); + then(tags.clears).as("shortened rebind").isEqualTo(clearsAtStartup); + findTestProperties().remove("test.tags[0]"); + this.rebinder.rebind(); + then(tags).isEmpty(); + then(tags.clears).as("removed rebind").isEqualTo(clearsAtStartup); } @Test @DirtiesContext - public void rebindEmptiesRemovedGetterOnlyList() { - then(this.properties.getTags()).containsExactly("a", "b"); - findTestProperties().remove("test.tags[0]"); - findTestProperties().remove("test.tags[1]"); + public void concurrentReaderNeverSeesGetterOnlyListEmptyDuringRebinds() throws Exception { + TrackingList tags = this.properties.getTags(); + then(tags).containsExactly("a", "b"); + AtomicBoolean stop = new AtomicBoolean(); + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + Future reads = executor.submit(() -> { + int count = 0; + while (!stop.get()) { + int seen = 0; + for (String tag : tags) { + then(tag).isNotNull(); + seen++; + } + then(seen).as("elements seen by a reader").isEqualTo(2); + count++; + } + return count; + }); + for (int i = 0; i < 50; i++) { + findTestProperties().put("test.tags[0]", (i % 2 == 0) ? "c" : "a"); + this.rebinder.rebind(); + } + stop.set(true); + then(reads.get(10, TimeUnit.SECONDS)).isPositive(); + } + finally { + stop.set(true); + executor.shutdownNow(); + } + then(tags).containsExactly("a", "b"); + } + + @Test + @DirtiesContext + public void injectedSetterIsNeitherCopiedFromTheFreshInstanceNorNulled() { + Clock clock = this.properties.getClock(); + then(clock).isNotNull(); this.rebinder.rebind(); - then(this.properties.getTags()).isEmpty(); + then(this.properties.getClock()).isSameAs(clock); + then(this.properties.clockAssignments).doesNotContainNull() + .allSatisfy(assigned -> then(assigned).isSameAs(clock)); } private Map findTestProperties() { @@ -152,6 +207,11 @@ protected TestProperties testProperties() { return new TestProperties(); } + @Bean + protected Clock testClock() { + return Clock.systemUTC(); + } + } // Hack out a protected inner class for testing @@ -175,7 +235,11 @@ protected static class TestProperties { final List workerAssignments = new ArrayList<>(); - private final List tags = new ArrayList<>(); + final List clockAssignments = new ArrayList<>(); + + private final TrackingList tags = new TrackingList(); + + private Clock clock; private final TrackingMap labels = new TrackingMap(); @@ -201,16 +265,39 @@ public void setWorkers(int workers) { this.workers = workers; } - public List getTags() { + public TrackingList getTags() { return this.tags; } + public Clock getClock() { + return this.clock; + } + + @Autowired + public void setClock(Clock clock) { + this.clockAssignments.add(clock); + this.clock = clock; + } + public TrackingMap getLabels() { return this.labels; } } + @SuppressWarnings("serial") + protected static class TrackingList extends ArrayList { + + int clears; + + @Override + public void clear() { + this.clears++; + super.clear(); + } + + } + @SuppressWarnings("serial") protected static class TrackingMap extends LinkedHashMap { From e32851fac434e32a75732daaa52a6ec2f82a84d9 Mon Sep 17 00:00:00 2001 From: Sharang Gupta Date: Thu, 22 Oct 2026 20:44:35 +0100 Subject: [PATCH 5/5] Move the Javadoc of bindFreshInstance back to its method Signed-off-by: Sharang Gupta --- .../ConfigurationPropertiesRebinder.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) 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 f91e58cae..42ed0aa32 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 @@ -255,14 +255,6 @@ private boolean rebind(String name, ApplicationContext appContext) { } } - /** - * Create a fresh instance of the bean's class, autowire it and bind it from the - * {@link Environment}, so that its state can replace the live bean's state once the - * live bean has been destroyed. This happens before the live bean is touched: a - * binding failure surfaces as the rebind error and leaves the live bean as it was. - * Returns {@code null} when no trustworthy fresh instance can be created; the live - * bean is then re-bound in place and removed properties keep their stale values. - */ /** * Rebind the live bean in place. Its new state is bound on a fresh instance first and * then copied over, so that readers never observe unbound state; the bean is then @@ -337,6 +329,15 @@ private void invokeInitMethods(Object bean, String name, AbstractBeanFactory bea } } + /** + * Create a fresh instance of the bean's class and bind it from the + * {@link Environment}, so that its state can replace the live bean's state once the + * live bean has been destroyed. This happens before the live bean is touched: a + * binding failure surfaces as the rebind error and leaves the live bean as it was. + * The instance is not autowired; injected properties are left alone by the copy. + * Returns {@code null} when no trustworthy fresh instance can be created; the live + * bean is then re-bound in place and removed properties keep their stale values. + */ private @Nullable Object bindFreshInstance(Object bean, String name, ApplicationContext appContext) { ConfigurationPropertiesBindingPostProcessor binder = appContext .getBeanProvider(ConfigurationPropertiesBindingPostProcessor.class)