Improve Zuul Hystrix Timeouts (#2645)

docs/src/main/asciidoc/spring-cloud-netflix.adoc

@@ -2115,11 +2115,44 @@ class MyFallbackProvider implements FallbackProvider {
 
 === Zuul Timeouts
 
-If you want to configure the socket timeouts and read timeouts for requests proxied through
-Zuul there are two options based on your configuration.
+==== Service Discovery Configuration
 
-If Zuul is using service discovery than you need to configure these timeouts via Ribbon properties,
-`ribbon.ReadTimeout` and `ribbon.SocketTimeout`.
+If Zuul is using service discovery there are two timeouts you need to be concerned
+with, the Hystrix timeout (since all routes are wrapped in Hystrix commands by default)
+and the Ribbon timeout.  The Hystrix timeout needs to take into account the Ribbon
+read and connect timeout PLUS the total number of retries that will happen for that
+service.  By default Spring Cloud Zuul will do its best to calculate the Hystrix timeout
+for you *UNLESS* you specify the Hystrix timeout explicitly.
+
+The Hystrix timeout is calculated using the following formula:
+```
+(ribbon.ConnectTimeout + ribbon.ReadTimeout) * (ribbon.MaxAutoRetries + 1) * (ribbon.MaxAutoRetriesNextServer + 1)
+```
+
+As an example, if you set the following properties in your application properties
+
+.application.yml
+----
+ribbon:
+  ReadTimeout:100
+  ConnectTimeout:500
+  MaxAutoRetries:1
+  MaxAutoRetriesNextServer:1
+----
+Then the Hystrix timeout (for all routes in this case) will be set to `2400ms.`
+
+NOTE: You can configure the Hystrix timeout for individual routes using `service.ribbon.*` properties.
+
+NOTE: If you choose to not configure the above properties than the default values will be used therefore the
+default Hystrix timeout will be set to `4000ms`.
+
+If you set `hystrix.command.commandKey.execution.isolation.thread.timeoutInMilliseconds`, where
+`commandKey` is the route id, or set `hystrix.command.default.execution.isolation.thread.timeoutInMilliseconds`
+than these values will be used for the Hystrix timeout regardless of what you have set for the `ribbon.*` properties.
+If you set either of these properties **YOU** are responsible for making sure it takes
+into account the Ribbon connect and read timeouts as well as any retries that may happen.
+
+==== URL Configuration
 
 If you have configured Zuul routes by specifying URLs than you will need to use
 `zuul.host.connect-timeout-millis` and `zuul.host.socket-timeout-millis`.

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

@@ -29,6 +29,7 @@ import org.springframework.cloud.netflix.zuul.filters.route.FallbackProvider;
 import org.springframework.http.client.ClientHttpResponse;
 import com.netflix.client.AbstractLoadBalancerAwareClient;
 import com.netflix.client.ClientRequest;
+import com.netflix.client.config.DefaultClientConfigImpl;
 import com.netflix.client.config.IClientConfig;
 import com.netflix.client.config.IClientConfigKey;
 import com.netflix.client.http.HttpResponse;
@@ -88,31 +89,55 @@ public abstract class AbstractRibbonCommand<LBC extends AbstractLoadBalancerAwar
 	}
 
 	protected static HystrixCommandProperties.Setter createSetter(IClientConfig config, String commandKey, ZuulProperties zuulProperties) {
+		int hystrixTimeout = getHystrixTimeout(config, commandKey);
+		return HystrixCommandProperties.Setter().withExecutionIsolationStrategy(
+				zuulProperties.getRibbonIsolationStrategy()).withExecutionTimeoutInMilliseconds(hystrixTimeout);
+	}
+
+	protected static int getHystrixTimeout(IClientConfig config, String commandKey) {
+		int ribbonTimeout = getRibbonTimeout(config, commandKey);
 		DynamicPropertyFactory dynamicPropertyFactory = DynamicPropertyFactory.getInstance();
 		int defaultHystrixTimeout = dynamicPropertyFactory.getIntProperty("hystrix.command.default.execution.isolation.thread.timeoutInMilliseconds",
-				0).get();
+			0).get();
 		int commandHystrixTimeout = dynamicPropertyFactory.getIntProperty("hystrix.command." + commandKey + ".execution.isolation.thread.timeoutInMilliseconds",
-				0).get();
-		int ribbonReadTimeout = config == null ? RibbonClientConfiguration.DEFAULT_READ_TIMEOUT :
-				config.get(IClientConfigKey.Keys.ReadTimeout, RibbonClientConfiguration.DEFAULT_READ_TIMEOUT).intValue();
-		int ribbonConnectTimeout = config == null ? RibbonClientConfiguration.DEFAULT_CONNECT_TIMEOUT :
-				config.get(IClientConfigKey.Keys.ConnectTimeout, RibbonClientConfiguration.DEFAULT_CONNECT_TIMEOUT).intValue();
-		int ribbonTimeout = ribbonConnectTimeout + ribbonReadTimeout;
+			0).get();
 		int hystrixTimeout;
 		if(commandHystrixTimeout > 0) {
 			hystrixTimeout = commandHystrixTimeout;
 		}
-		else if( defaultHystrixTimeout > 0) {
+		else if(defaultHystrixTimeout > 0) {
 			hystrixTimeout = defaultHystrixTimeout;
 		} else {
 			hystrixTimeout = ribbonTimeout;
 		}
 		if(hystrixTimeout < ribbonTimeout) {
 			LOGGER.warn("The Hystrix timeout of " + hystrixTimeout + "ms for the command " + commandKey +
-					" is set lower than the combination of the Ribbon read and connect timeout, " + ribbonTimeout + "ms.");
+				" is set lower than the combination of the Ribbon read and connect timeout, " + ribbonTimeout + "ms.");
 		}
-		return HystrixCommandProperties.Setter().withExecutionIsolationStrategy(
-				zuulProperties.getRibbonIsolationStrategy()).withExecutionTimeoutInMilliseconds(hystrixTimeout);
+		return hystrixTimeout;
+	}
+
+	protected static int getRibbonTimeout(IClientConfig config, String commandKey) {
+		int ribbonTimeout;
+		if (config == null) {
+			ribbonTimeout = RibbonClientConfiguration.DEFAULT_READ_TIMEOUT + RibbonClientConfiguration.DEFAULT_CONNECT_TIMEOUT;
+		} else {
+			int ribbonReadTimeout = getTimeout(config, commandKey, "ReadTimeout",
+				IClientConfigKey.Keys.ReadTimeout, RibbonClientConfiguration.DEFAULT_READ_TIMEOUT);
+			int ribbonConnectTimeout = getTimeout(config, commandKey, "ConnectTimeout",
+				IClientConfigKey.Keys.ConnectTimeout, RibbonClientConfiguration.DEFAULT_CONNECT_TIMEOUT);
+			int maxAutoRetries = getTimeout(config, commandKey, "MaxAutoRetries",
+				IClientConfigKey.Keys.MaxAutoRetries, DefaultClientConfigImpl.DEFAULT_MAX_AUTO_RETRIES);
+			int maxAutoRetriesNextServer = getTimeout(config, commandKey, "MaxAutoRetriesNextServer",
+				IClientConfigKey.Keys.MaxAutoRetriesNextServer, DefaultClientConfigImpl.DEFAULT_MAX_AUTO_RETRIES_NEXT_SERVER);
+			ribbonTimeout = (ribbonReadTimeout + ribbonConnectTimeout) * (maxAutoRetries + 1) * (maxAutoRetriesNextServer + 1);
+		}
+		return ribbonTimeout;
+	}
+
+	private static int getTimeout(IClientConfig config, String commandKey, String property, IClientConfigKey<Integer> configKey, int defaultValue) {
+		DynamicPropertyFactory dynamicPropertyFactory = DynamicPropertyFactory.getInstance();
+		return dynamicPropertyFactory.getIntProperty(commandKey + "." + config.getNameSpace() + "." + property, config.get(configKey, defaultValue)).get();
 	}
 
 	@Deprecated