Merge pull request #2261 from ryanjbaxter/timeout-values

spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfiguration.java

@@ -35,6 +35,7 @@ import org.springframework.context.annotation.Import;
 
 import com.netflix.client.DefaultLoadBalancerRetryHandler;
 import com.netflix.client.RetryHandler;
+import com.netflix.client.config.CommonClientConfigKey;
 import com.netflix.client.config.DefaultClientConfigImpl;
 import com.netflix.client.config.IClientConfig;
 import com.netflix.loadbalancer.ConfigurationBasedServerList;
@@ -68,6 +69,9 @@ import static org.springframework.cloud.netflix.ribbon.RibbonUtils.updateToHttps
 @Import({HttpClientConfiguration.class, OkHttpRibbonConfiguration.class, RestClientRibbonConfiguration.class, HttpClientRibbonConfiguration.class})
 public class RibbonClientConfiguration {
 
+	public static final int DEFAULT_CONNECT_TIMEOUT = 1000;
+	public static final int DEFAULT_READ_TIMEOUT = 1000;
+
 	@Value("${ribbon.client.name}")
 	private String name = "client";
 
@@ -82,6 +86,8 @@ public class RibbonClientConfiguration {
 	public IClientConfig ribbonClientConfig() {
 		DefaultClientConfigImpl config = new DefaultClientConfigImpl();
 		config.loadProperties(this.name);
+		config.set(CommonClientConfigKey.ConnectTimeout, DEFAULT_CONNECT_TIMEOUT);
+		config.set(CommonClientConfigKey.ReadTimeout, DEFAULT_READ_TIMEOUT);
 		return config;
 	}
 

spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/support/AbstractLoadBalancingClient.java

@@ -18,6 +18,7 @@
 package org.springframework.cloud.netflix.ribbon.support;
 
 import org.springframework.cloud.netflix.ribbon.DefaultServerIntrospector;
+import org.springframework.cloud.netflix.ribbon.RibbonClientConfiguration;
 import org.springframework.cloud.netflix.ribbon.ServerIntrospector;
 
 import com.netflix.client.AbstractLoadBalancerAwareClient;
@@ -92,10 +93,10 @@ public abstract class AbstractLoadBalancingClient<S extends ContextAwareRequest,
 		super.initWithNiwsConfig(clientConfig);
 		this.connectTimeout = clientConfig.getPropertyAsInteger(
 				CommonClientConfigKey.ConnectTimeout,
-				DefaultClientConfigImpl.DEFAULT_CONNECT_TIMEOUT);
+				RibbonClientConfiguration.DEFAULT_CONNECT_TIMEOUT);
 		this.readTimeout = clientConfig.getPropertyAsInteger(
 				CommonClientConfigKey.ReadTimeout,
-				DefaultClientConfigImpl.DEFAULT_READ_TIMEOUT);
+				RibbonClientConfiguration.DEFAULT_READ_TIMEOUT);
 		this.secure = clientConfig.getPropertyAsBoolean(CommonClientConfigKey.IsSecure,
 				false);
 		this.followRedirects = clientConfig.getPropertyAsBoolean(

spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommand.java

@@ -17,6 +17,7 @@
 
 package org.springframework.cloud.netflix.zuul.filters.route.support;
 
+import org.springframework.cloud.netflix.ribbon.RibbonClientConfiguration;
 import org.springframework.cloud.netflix.ribbon.RibbonHttpResponse;
 import org.springframework.cloud.netflix.zuul.filters.ZuulProperties;
 import org.springframework.cloud.netflix.zuul.filters.route.RibbonCommand;
@@ -83,7 +84,8 @@ public abstract class AbstractRibbonCommand<LBC extends AbstractLoadBalancerAwar
 								.andCommandKey(HystrixCommandKey.Factory.asKey(commandKey));
 
 		final HystrixCommandProperties.Setter setter = HystrixCommandProperties.Setter()
-				.withExecutionIsolationStrategy(zuulProperties.getRibbonIsolationStrategy());
+				.withExecutionIsolationStrategy(zuulProperties.getRibbonIsolationStrategy()).withExecutionTimeoutInMilliseconds(
+						RibbonClientConfiguration.DEFAULT_CONNECT_TIMEOUT + RibbonClientConfiguration.DEFAULT_READ_TIMEOUT);
 		if (zuulProperties.getRibbonIsolationStrategy() == ExecutionIsolationStrategy.SEMAPHORE){
 			final String name = ZuulConstants.ZUUL_EUREKA + commandKey + ".semaphore.maxSemaphores";
 			// we want to default to semaphore-isolation since this wraps

spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClientTests.java

@@ -114,6 +114,13 @@ public class RibbonLoadBalancingHttpClientTests {
 	}
 
 	@Test
+	public void testDefaultTimeouts() throws Exception {
+		RequestConfig result = getBuiltRequestConfig(UseDefaults.class, null);
+		assertThat(result.getConnectTimeout(), is(1000));
+		assertThat(result.getSocketTimeout(), is (1000));
+	}
+
+	@Test
 	public void testConnections() throws Exception {
 		SpringClientFactory factory = new SpringClientFactory();
 		factory.setApplicationContext(new AnnotationConfigApplicationContext(

spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/okhttp/OkHttpLoadBalancingClientTests.java

@@ -94,6 +94,13 @@ public class OkHttpLoadBalancingClientTests {
 	}
 
 	@Test
+	public void testDefaultTimeouts() throws Exception {
+		OkHttpClient result = getHttpClient(UseDefaults.class, null);
+		assertThat(result.readTimeoutMillis(), is(1000));
+		assertThat(result.connectTimeoutMillis(), is(1000));
+	}
+
+	@Test
 	public void testTimeoutsOverride() throws Exception {
 		DefaultClientConfigImpl override = new DefaultClientConfigImpl();
 		override.set(CommonClientConfigKey.ConnectTimeout, 60);

spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFactoryTest.java

+package org.springframework.cloud.netflix.zuul.filters.route.apache;
+
+import java.util.HashSet;
+import org.junit.Before;
+import org.junit.Test;
+import org.springframework.cloud.netflix.ribbon.SpringClientFactory;
+import org.springframework.cloud.netflix.ribbon.apache.RibbonLoadBalancingHttpClient;
+import org.springframework.cloud.netflix.zuul.filters.ZuulProperties;
+import org.springframework.cloud.netflix.zuul.filters.route.RibbonCommandContext;
+import org.springframework.cloud.netflix.zuul.filters.route.ZuulFallbackProvider;
+
+import com.netflix.client.config.DefaultClientConfigImpl;
+import com.netflix.client.config.IClientConfig;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+
+/**
+ * @author Ryan Baxter
+ */
+public class HttpClientRibbonCommandFactoryTest {
+
+	SpringClientFactory springClientFactory;
+	ZuulProperties zuulProperties;
+	HttpClientRibbonCommandFactory ribbonCommandFactory;
+
+	@Before
+	public void setup(){
+		this.springClientFactory = mock(SpringClientFactory.class);
+		this.zuulProperties = new ZuulProperties();
+		RibbonLoadBalancingHttpClient loadBalancingHttpClient = mock(RibbonLoadBalancingHttpClient.class);
+		IClientConfig clientConfig = new DefaultClientConfigImpl();
+		doReturn(loadBalancingHttpClient).when(this.springClientFactory).getClient(anyString(),
+				eq(RibbonLoadBalancingHttpClient.class));
+		doReturn(clientConfig).when(this.springClientFactory).getClientConfig(anyString());
+		this.ribbonCommandFactory = new HttpClientRibbonCommandFactory(springClientFactory, zuulProperties, new HashSet<ZuulFallbackProvider>());
+	}
+
+	@Test
+	public void testHystrixTimeoutValue() throws Exception {
+		RibbonCommandContext context = mock(RibbonCommandContext.class);
+		doReturn("service").when(context).getServiceId();
+		HttpClientRibbonCommand ribbonCommand = this.ribbonCommandFactory.create(context);
+		assertEquals(2000, ribbonCommand.getProperties().executionTimeoutInMilliseconds().get().intValue());
+	}
+
+}
\ No newline at end of file

spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/okhttp/OkHttpRibbonCommandFactoryTest.java

+package org.springframework.cloud.netflix.zuul.filters.route.okhttp;
+
+import java.util.HashSet;
+import org.junit.Before;
+import org.junit.Test;
+import org.springframework.cloud.netflix.ribbon.SpringClientFactory;
+import org.springframework.cloud.netflix.ribbon.okhttp.OkHttpLoadBalancingClient;
+import org.springframework.cloud.netflix.zuul.filters.ZuulProperties;
+import org.springframework.cloud.netflix.zuul.filters.route.RibbonCommandContext;
+import org.springframework.cloud.netflix.zuul.filters.route.ZuulFallbackProvider;
+
+import com.netflix.client.config.DefaultClientConfigImpl;
+import com.netflix.client.config.IClientConfig;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+
+/**
+ * @author Ryan Baxter
+ */
+public class OkHttpRibbonCommandFactoryTest {
+
+	SpringClientFactory springClientFactory;
+	ZuulProperties zuulProperties;
+	OkHttpRibbonCommandFactory commandFactory;
+
+	@Before
+	public void setup() {
+		this.springClientFactory = mock(SpringClientFactory.class);
+		this.zuulProperties = new ZuulProperties();
+		OkHttpLoadBalancingClient loadBalancingHttpClient = mock(OkHttpLoadBalancingClient.class);
+		IClientConfig clientConfig = new DefaultClientConfigImpl();
+		doReturn(loadBalancingHttpClient).when(this.springClientFactory).getClient(anyString(),
+				eq(OkHttpLoadBalancingClient.class));
+		doReturn(clientConfig).when(this.springClientFactory).getClientConfig(anyString());
+		commandFactory = new OkHttpRibbonCommandFactory(springClientFactory, zuulProperties, new HashSet<ZuulFallbackProvider>());
+	}
+
+	@Test
+	public void testHystrixTimeoutValue() throws Exception {
+		RibbonCommandContext context = mock(RibbonCommandContext.class);
+		doReturn("service").when(context).getServiceId();
+		OkHttpRibbonCommand ribbonCommand = this.commandFactory.create(context);
+		assertEquals(2000, ribbonCommand.getProperties().executionTimeoutInMilliseconds().get().intValue());
+	}
+
+}
\ No newline at end of file