From 773888fcadcc019a7c07870408fdf9370134f264 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Tue, 3 Mar 2020 16:04:07 -0500 Subject: [PATCH 1/4] Updates javadoc to mention WebClient. fixes gh-645 --- .../cloud/client/loadbalancer/LoadBalanced.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalanced.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalanced.java index 151a15d7..10c997c3 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalanced.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalanced.java @@ -26,7 +26,8 @@ import java.lang.annotation.Target; import org.springframework.beans.factory.annotation.Qualifier; /** - * Annotation to mark a RestTemplate bean to be configured to use a LoadBalancerClient. + * Annotation to mark a RestTemplate or WebClient bean to be configured to use a + * LoadBalancerClient. * @author Spencer Gibb */ @Target({ ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD }) From 0f78c5b4adf30d8f3f317e3f77c464572ee08628 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Tue, 3 Mar 2020 17:51:22 -0500 Subject: [PATCH 2/4] Pass SimpleDiscoveryProperties as parameter. 18a4227c4ef1bdf86c3103e64bce75847a53073e turned proxying off which broke this. fixes gh-687 --- .../simple/SimpleDiscoveryClientAutoConfiguration.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/discovery/simple/SimpleDiscoveryClientAutoConfiguration.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/discovery/simple/SimpleDiscoveryClientAutoConfiguration.java index a513ddeb..7f54c202 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/discovery/simple/SimpleDiscoveryClientAutoConfiguration.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/discovery/simple/SimpleDiscoveryClientAutoConfiguration.java @@ -68,8 +68,8 @@ public class SimpleDiscoveryClientAutoConfiguration @Bean @Order - public DiscoveryClient simpleDiscoveryClient() { - return new SimpleDiscoveryClient(simpleDiscoveryProperties()); + public DiscoveryClient simpleDiscoveryClient(SimpleDiscoveryProperties properties) { + return new SimpleDiscoveryClient(properties); } private int findPort() { From 5ea26e13a30d1d442365a932e4f9aa5c6f791fb4 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Tue, 3 Mar 2020 17:52:52 -0500 Subject: [PATCH 3/4] formatting --- ...ealthCheckServiceInstanceListSupplier.java | 28 ++--- ...CheckServiceInstanceListSupplierTests.java | 108 +++++++----------- 2 files changed, 50 insertions(+), 86 deletions(-) diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplier.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplier.java index 86efcaae..31bd8c5a 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplier.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplier.java @@ -71,11 +71,9 @@ public class HealthCheckServiceInstanceListSupplier this.webClient = webClient; this.aliveInstancesReplay = Flux.defer(delegate) .delaySubscription(Duration.ofMillis(this.healthCheck.getInitialDelay())) - .switchMap(serviceInstances -> healthCheckFlux(serviceInstances) - .map(alive -> Collections.unmodifiableList(new ArrayList<>(alive))) - ) - .replay(1) - .refCount(1); + .switchMap(serviceInstances -> healthCheckFlux(serviceInstances).map( + alive -> Collections.unmodifiableList(new ArrayList<>(alive)))) + .replay(1).refCount(1); } @Override @@ -87,7 +85,8 @@ public class HealthCheckServiceInstanceListSupplier this.healthCheckDisposable = aliveInstancesReplay.subscribe(); } - protected Flux> healthCheckFlux(List instances) { + protected Flux> healthCheckFlux( + List instances) { return Flux.defer(() -> { List> checks = new ArrayList<>(instances.size()); for (ServiceInstance instance : instances) { @@ -98,8 +97,7 @@ public class HealthCheckServiceInstanceListSupplier instance.getServiceId(), instance.getUri()), error); } return Mono.empty(); - }) - .timeout(this.healthCheck.getInterval(), Mono.defer(() -> { + }).timeout(this.healthCheck.getInterval(), Mono.defer(() -> { if (LOG.isDebugEnabled()) { LOG.debug(String.format( "The instance for service %s: %s did not respond for %s during health check", @@ -107,8 +105,7 @@ public class HealthCheckServiceInstanceListSupplier this.healthCheck.getInterval())); } return Mono.empty(); - })) - .handle((isHealthy, sink) -> { + })).handle((isHealthy, sink) -> { if (isHealthy) { sink.next(instance); } @@ -120,10 +117,8 @@ public class HealthCheckServiceInstanceListSupplier return Flux.merge(checks).map(alive -> { result.add(alive); return result; - }) - .defaultIfEmpty(result); - }) - .repeatWhen(restart -> restart.delayElements(this.healthCheck.getInterval())); + }).defaultIfEmpty(result); + }).repeatWhen(restart -> restart.delayElements(this.healthCheck.getInterval())); } @Override @@ -145,9 +140,8 @@ public class HealthCheckServiceInstanceListSupplier .uri(UriComponentsBuilder.fromUri(serviceInstance.getUri()) .path(healthCheckPath).build().toUri()) .exchange() - .flatMap(clientResponse -> clientResponse.releaseBody() - .thenReturn(HttpStatus.OK.value() == clientResponse.rawStatusCode()) - ); + .flatMap(clientResponse -> clientResponse.releaseBody().thenReturn( + HttpStatus.OK.value() == clientResponse.rawStatusCode())); } @Override diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplierTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplierTests.java index 7d85ccb1..82320d59 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplierTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplierTests.java @@ -162,13 +162,10 @@ class HealthCheckServiceInstanceListSupplierTests { }; return listSupplier.get(); - }) - .expectSubscription() + }).expectSubscription() .expectNoEvent(Duration.ofMillis(healthCheck.getInitialDelay())) - .expectNext(Lists.list(si1)) - .expectNoEvent(healthCheck.getInterval()) - .thenCancel() - .verify(VERIFY_TIMEOUT); + .expectNext(Lists.list(si1)).expectNoEvent(healthCheck.getInterval()) + .thenCancel().verify(VERIFY_TIMEOUT); } @Test @@ -199,13 +196,10 @@ class HealthCheckServiceInstanceListSupplierTests { }; return listSupplier.get(); - }) - .expectSubscription() + }).expectSubscription() .expectNoEvent(Duration.ofMillis(healthCheck.getInitialDelay())) - .expectNext(Lists.list(si1)) - .expectNext(Lists.list(si1, si2)) - .expectNoEvent(healthCheck.getInterval()) - .thenCancel() + .expectNext(Lists.list(si1)).expectNext(Lists.list(si1, si2)) + .expectNoEvent(healthCheck.getInterval()).thenCancel() .verify(VERIFY_TIMEOUT); } @@ -238,13 +232,10 @@ class HealthCheckServiceInstanceListSupplierTests { }; return listSupplier.get(); - }) - .expectSubscription() + }).expectSubscription() .expectNoEvent(Duration.ofMillis(healthCheck.getInitialDelay())) - .expectNext(Lists.list(si1)) - .expectNoEvent(healthCheck.getInterval()) - .thenCancel() - .verify(VERIFY_TIMEOUT); + .expectNext(Lists.list(si1)).expectNoEvent(healthCheck.getInterval()) + .thenCancel().verify(VERIFY_TIMEOUT); } @Test @@ -274,13 +265,10 @@ class HealthCheckServiceInstanceListSupplierTests { }; return listSupplier.get(); - }) - .expectSubscription() + }).expectSubscription() .expectNoEvent(Duration.ofMillis(healthCheck.getInitialDelay())) - .expectNext(Lists.list()) - .expectNoEvent(healthCheck.getInterval()) - .thenCancel() - .verify(VERIFY_TIMEOUT); + .expectNext(Lists.list()).expectNoEvent(healthCheck.getInterval()) + .thenCancel().verify(VERIFY_TIMEOUT); } @Test @@ -305,13 +293,10 @@ class HealthCheckServiceInstanceListSupplierTests { listSupplier.afterPropertiesSet(); return listSupplier.get(); - }) - .expectSubscription() + }).expectSubscription() .expectNoEvent(Duration.ofMillis(healthCheck.getInitialDelay())) - .expectNext(Lists.list(si1)) - .expectNoEvent(healthCheck.getInterval()) - .thenCancel() - .verify(VERIFY_TIMEOUT); + .expectNext(Lists.list(si1)).expectNoEvent(healthCheck.getInterval()) + .thenCancel().verify(VERIFY_TIMEOUT); } @Test @@ -343,15 +328,11 @@ class HealthCheckServiceInstanceListSupplierTests { }; return listSupplier.get(); - }) - .expectSubscription() + }).expectSubscription() .expectNoEvent(Duration.ofMillis(healthCheck.getInitialDelay())) - .expectNext(Lists.list()) - .expectNoEvent(healthCheck.getInterval()) - .expectNext(Lists.list(si1)) - .expectNoEvent(healthCheck.getInterval()) - .expectNext(Lists.list(si1)) - .thenCancel().verify(VERIFY_TIMEOUT); + .expectNext(Lists.list()).expectNoEvent(healthCheck.getInterval()) + .expectNext(Lists.list(si1)).expectNoEvent(healthCheck.getInterval()) + .expectNext(Lists.list(si1)).thenCancel().verify(VERIFY_TIMEOUT); } @Test @@ -379,15 +360,11 @@ class HealthCheckServiceInstanceListSupplierTests { }; return listSupplier.get(); - }) - .expectSubscription() + }).expectSubscription() .expectNoEvent(Duration.ofMillis(healthCheck.getInitialDelay())) - .expectNoEvent(healthCheck.getInterval()) - .expectNext(Lists.list()) - .expectNoEvent(healthCheck.getInterval()) - .expectNext(Lists.list(si1)) - .expectNoEvent(healthCheck.getInterval()) - .expectNext(Lists.list(si1)) + .expectNoEvent(healthCheck.getInterval()).expectNext(Lists.list()) + .expectNoEvent(healthCheck.getInterval()).expectNext(Lists.list(si1)) + .expectNoEvent(healthCheck.getInterval()).expectNext(Lists.list(si1)) .thenCancel().verify(VERIFY_TIMEOUT); } @@ -417,18 +394,13 @@ class HealthCheckServiceInstanceListSupplierTests { }; return listSupplier.get(); - }) - .expectSubscription() + }).expectSubscription() .expectNoEvent(Duration.ofMillis(healthCheck.getInitialDelay())) .expectNext(Lists.list(si1)) .thenAwait(healthCheck.getInterval().dividedBy(2)) - .expectNext(Lists.list(si1)) - .expectNext(Lists.list(si1, si2)) - .expectNoEvent(healthCheck.getInterval()) - .expectNext(Lists.list(si1)) - .expectNext(Lists.list(si1, si2)) - .thenCancel() - .verify(VERIFY_TIMEOUT); + .expectNext(Lists.list(si1)).expectNext(Lists.list(si1, si2)) + .expectNoEvent(healthCheck.getInterval()).expectNext(Lists.list(si1)) + .expectNext(Lists.list(si1, si2)).thenCancel().verify(VERIFY_TIMEOUT); } @Test @@ -453,20 +425,19 @@ class HealthCheckServiceInstanceListSupplierTests { } @Override - protected Flux> healthCheckFlux(List instances) { - return super.healthCheckFlux(instances).doOnNext(it -> emitCounter.incrementAndGet()); + protected Flux> healthCheckFlux( + List instances) { + return super.healthCheckFlux(instances) + .doOnNext(it -> emitCounter.incrementAndGet()); } }; listSupplier.afterPropertiesSet(); return listSupplier.get().take(1).concatWith(listSupplier.get().take(1)); - }) - .expectSubscription() + }).expectSubscription() .expectNoEvent(Duration.ofMillis(healthCheck.getInitialDelay())) - .expectNext(Lists.list(si1)) - .expectNext(Lists.list(si1)) - .thenCancel() + .expectNext(Lists.list(si1)).expectNext(Lists.list(si1)).thenCancel() .verify(VERIFY_TIMEOUT); Assertions.assertThat(emitCounter).hasValue(1); @@ -479,20 +450,19 @@ class HealthCheckServiceInstanceListSupplierTests { ServiceInstanceListSupplier delegate = Mockito .mock(ServiceInstanceListSupplier.class); - Mockito.when(delegate.get()) - .thenReturn(Flux.>never() - .log("test") - .doOnCancel(instancesCanceled::incrementAndGet)); + Mockito.when(delegate.get()).thenReturn(Flux.>never() + .log("test").doOnCancel(instancesCanceled::incrementAndGet)); - listSupplier = new HealthCheckServiceInstanceListSupplier(delegate, healthCheck, webClient); + listSupplier = new HealthCheckServiceInstanceListSupplier(delegate, healthCheck, + webClient); listSupplier.afterPropertiesSet(); Assertions.assertThat(instancesCanceled).hasValue(0); listSupplier.destroy(); - Awaitility.await() - .pollDelay(Duration.ofMillis(100)).atMost(VERIFY_TIMEOUT).untilAsserted( + Awaitility.await().pollDelay(Duration.ofMillis(100)).atMost(VERIFY_TIMEOUT) + .untilAsserted( () -> Assertions.assertThat(instancesCanceled).hasValue(1)); } From 69a8cece5fd35480d549948cabe3006562eab896 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Tue, 3 Mar 2020 17:54:58 -0500 Subject: [PATCH 4/4] 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; + } + + } + }