From 69a8cece5fd35480d549948cabe3006562eab896 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Tue, 3 Mar 2020 17:54:58 -0500 Subject: [PATCH] Ensures HikariDataSource is never re-bound. Removes HikariDataSource from extra-refreshable and adds it to a new property never-refreshable. ConfigurationPropertiesRebinder now checks that property and does not rebind beans whose class is in never-refreshable. fixes gh-687 --- .../main/asciidoc/spring-cloud-commons.adoc | 6 +-- .../RefreshAutoConfiguration.java | 4 +- .../ConfigurationPropertiesRebinder.java | 14 +++++ ...itional-spring-configuration-metadata.json | 6 +++ .../RefreshAutoConfigurationTests.java | 52 ++++++++++++++++--- 5 files changed, 68 insertions(+), 14 deletions(-) diff --git a/docs/src/main/asciidoc/spring-cloud-commons.adoc b/docs/src/main/asciidoc/spring-cloud-commons.adoc index f89a3a75..6a122223 100644 --- a/docs/src/main/asciidoc/spring-cloud-commons.adoc +++ b/docs/src/main/asciidoc/spring-cloud-commons.adoc @@ -184,9 +184,9 @@ Then, the next time something borrows a connection from the pool, it gets one wi Sometimes, it might even be mandatory to apply the `@RefreshScope` annotation on some beans that can be only initialized once. If a bean is "`immutable`", you have to either annotate the bean with `@RefreshScope` or specify the classname under the property key: `spring.cloud.refresh.extra-refreshable`. -IMPORTANT: If you create a `DataSource` bean yourself and the implementation is a `HikariDataSource`, return the -most specific type, in this case `HikariDataSource`. Otherwise, you need to set -`spring.cloud.refresh.extra-refreshable=javax.sql.DataSource`. +WARNING: If you hava a `DataSource` bean that is a `HikariDataSource`, it can not be +refreshed. It is the default value for `spring.cloud.refresh.never-refreshable`. Choose a +different `DataSource` implementation if you need it to be refreshed. Refresh scope beans are lazy proxies that initialize when they are used (that is, when a method is called), and the scope acts as a cache of initialized values. To force a bean to re-initialize on the next method call, you must invalidate its cache entry. diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/RefreshAutoConfiguration.java b/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/RefreshAutoConfiguration.java index c815bf48..11910675 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/RefreshAutoConfiguration.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/RefreshAutoConfiguration.java @@ -16,7 +16,6 @@ package org.springframework.cloud.autoconfigure; -import java.util.Arrays; import java.util.HashSet; import java.util.Set; @@ -140,8 +139,7 @@ public class RefreshAutoConfiguration { * Class names for beans to post process into refresh scope. Useful when you don't * control the bean definition (e.g. it came from auto-configuration). */ - private Set refreshables = new HashSet<>( - Arrays.asList("com.zaxxer.hikari.HikariDataSource")); + private Set refreshables = new HashSet<>(); public Set getRefreshable() { return this.refreshables; 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 823ce2e9..20c1ac26 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 @@ -35,6 +35,7 @@ import org.springframework.jmx.export.annotation.ManagedAttribute; import org.springframework.jmx.export.annotation.ManagedOperation; import org.springframework.jmx.export.annotation.ManagedResource; import org.springframework.stereotype.Component; +import org.springframework.util.StringUtils; /** * Listens for {@link EnvironmentChangeEvent} and rebinds beans that were bound to the @@ -96,6 +97,11 @@ public class ConfigurationPropertiesRebinder bean = ProxyUtils.getTargetObject(bean); } if (bean != null) { + // TODO: determine a more general approach to fix this. + // see https://github.com/spring-cloud/spring-cloud-commons/issues/571 + if (getNeverRefreshable().contains(bean.getClass().getName())) { + return false; // ignore + } this.applicationContext.getAutowireCapableBeanFactory() .destroyBean(bean); this.applicationContext.getAutowireCapableBeanFactory() @@ -115,6 +121,14 @@ public class ConfigurationPropertiesRebinder return false; } + @ManagedAttribute + public Set getNeverRefreshable() { + String neverRefresh = this.applicationContext.getEnvironment().getProperty( + "spring.cloud.refresh.never-refreshable", + "com.zaxxer.hikari.HikariDataSource"); + return StringUtils.commaDelimitedListToSet(neverRefresh); + } + @ManagedAttribute public Set getBeanNames() { return new HashSet<>(this.beans.getBeanNames()); diff --git a/spring-cloud-context/src/main/resources/META-INF/additional-spring-configuration-metadata.json b/spring-cloud-context/src/main/resources/META-INF/additional-spring-configuration-metadata.json index 484e74fa..aeffd7e9 100644 --- a/spring-cloud-context/src/main/resources/META-INF/additional-spring-configuration-metadata.json +++ b/spring-cloud-context/src/main/resources/META-INF/additional-spring-configuration-metadata.json @@ -41,6 +41,12 @@ "type": "java.util.Set", "description": "Additional class names for beans to post process into refresh scope.", "defaultValue": true + }, + { + "name": "spring.cloud.refresh.never-refreshable", + "type": "java.lang.String", + "description": "Comma separated list of class names for beans to never be refreshed or rebound.", + "defaultValue": true } ] } diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/autoconfigure/RefreshAutoConfigurationTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/autoconfigure/RefreshAutoConfigurationTests.java index 17459422..b8a37db5 100644 --- a/spring-cloud-context/src/test/java/org/springframework/cloud/autoconfigure/RefreshAutoConfigurationTests.java +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/autoconfigure/RefreshAutoConfigurationTests.java @@ -16,6 +16,8 @@ package org.springframework.cloud.autoconfigure; +import java.util.concurrent.atomic.AtomicInteger; + import org.junit.Rule; import org.junit.Test; @@ -30,6 +32,7 @@ import org.springframework.cloud.context.refresh.ContextRefresher; import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.annotation.Configuration; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.BDDAssertions.then; /** @@ -68,8 +71,9 @@ public class RefreshAutoConfigurationTests { public void refreshables() { try (ConfigurableApplicationContext context = getApplicationContext( WebApplicationType.NONE, Config.class, "config.foo=bar", - "spring.cloud.refresh.refreshable:" + ConfigProps.class.getName())) { - context.getBean(ConfigProps.class); + "spring.cloud.refresh.refreshable:" + + SealedConfigProps.class.getName())) { + context.getBean(SealedConfigProps.class); context.getBean(ContextRefresher.class).refresh(); } } @@ -77,23 +81,37 @@ public class RefreshAutoConfigurationTests { @Test public void extraRefreshables() { try (ConfigurableApplicationContext context = getApplicationContext( - WebApplicationType.NONE, Config.class, "config.foo=bar", + WebApplicationType.NONE, Config.class, "sealedconfig.foo=bar", "spring.cloud.refresh.extra-refreshable:" - + ConfigProps.class.getName())) { - context.getBean(ConfigProps.class); + + SealedConfigProps.class.getName())) { + context.getBean(SealedConfigProps.class); context.getBean(ContextRefresher.class).refresh(); } } + @Test + public void neverRefreshable() { + try (ConfigurableApplicationContext context = getApplicationContext( + WebApplicationType.NONE, Config.class, "countingconfig.foo=bar", + "spring.cloud.refresh.never-refreshable:" + + CountingConfigProps.class.getName())) { + CountingConfigProps configProps = context.getBean(CountingConfigProps.class); + context.getBean(ContextRefresher.class).refresh(); + assertThat(configProps.count) + .as("config props was rebound when it should not have been") + .hasValue(1); + } + } + @Configuration(proxyBeanMethods = false) @EnableAutoConfiguration(exclude = DataSourceAutoConfiguration.class) - @EnableConfigurationProperties(ConfigProps.class) + @EnableConfigurationProperties({ SealedConfigProps.class, CountingConfigProps.class }) static class Config { } - @ConfigurationProperties("config") - static class ConfigProps { + @ConfigurationProperties("sealedconfig") + static class SealedConfigProps { private String foo; @@ -113,4 +131,22 @@ public class RefreshAutoConfigurationTests { } + @ConfigurationProperties("countingconfig") + static class CountingConfigProps { + + private final AtomicInteger count = new AtomicInteger(); + + private String foo; + + public String getFoo() { + return this.foo; + } + + public void setFoo(String foo) { + count.incrementAndGet(); + this.foo = foo; + } + + } + }