Allow default settings of a custom HttpClient to apply

Previously the default settings of a custom HttpClient were always
ignored since a RequestConfig instance was always set no matter if
some customizations were applied or not.

This commit keeps an internal RequestConfig object instance that is
only initialized if the user applies a customization. If he does not, the
default settings of the HttpClient are used as it should.

Note that if the HttpComponents API exposed the default RequestConfig
of a given HttpClient, we would be able to merge our customizations with
the one specified by the client. Unfortunately, such API does not exist
and the "defaultSettingsOfHttpClientLostOnExecutorCustomization" test
illustrates that limitation.

Issue: SPR-12540

Author: snicoll

spring-web/src/main/java/org/springframework/http/client/HttpComponentsClientHttpRequestFactory.java

@@ -59,11 +59,7 @@ public class HttpComponentsClientHttpRequestFactory implements ClientHttpRequest
 
 	private CloseableHttpClient httpClient;
 
-	private int connectTimeout;
-
-	private int connectionRequestTimeout;
-
-	private int socketTimeout;
+	private RequestConfig requestConfig;
 
 	private boolean bufferRequestBody = true;
 
@@ -111,11 +107,15 @@ public HttpClient getHttpClient() {
 	/**
 	 * Set the connection timeout for the underlying HttpClient.
 	 * A timeout value of 0 specifies an infinite timeout.
+	 * <p>Additional properties can be configured by specifying a
+	 * {@link RequestConfig} instance on a custom {@link HttpClient}.
 	 * @param timeout the timeout value in milliseconds
+	 * @see RequestConfig#getConnectTimeout()
 	 */
 	public void setConnectTimeout(int timeout) {
 		Assert.isTrue(timeout >= 0, "Timeout must be a non-negative value");
-		this.connectTimeout = timeout;
+		this.requestConfig = cloneRequestConfig()
+				.setConnectTimeout(timeout).build();
 		setLegacyConnectionTimeout(getHttpClient(), timeout);
 	}
 
@@ -145,20 +145,28 @@ private void setLegacyConnectionTimeout(HttpClient client, int timeout) {
 	 * Set the timeout in milliseconds used when requesting a connection from the connection
 	 * manager using the underlying HttpClient.
 	 * A timeout value of 0 specifies an infinite timeout.
+	 * <p>Additional properties can be configured by specifying a
+	 * {@link RequestConfig} instance on a custom {@link HttpClient}.
 	 * @param connectionRequestTimeout the timeout value to request a connection in milliseconds
+	 * @see RequestConfig#getConnectionRequestTimeout()
 	 */
 	public void setConnectionRequestTimeout(int connectionRequestTimeout) {
-		this.connectionRequestTimeout = connectionRequestTimeout;
+		this.requestConfig = cloneRequestConfig()
+				.setConnectionRequestTimeout(connectionRequestTimeout).build();
 	}
 
 	/**
 	 * Set the socket read timeout for the underlying HttpClient.
 	 * A timeout value of 0 specifies an infinite timeout.
+	 * <p>Additional properties can be configured by specifying a
+	 * {@link RequestConfig} instance on a custom {@link HttpClient}.
 	 * @param timeout the timeout value in milliseconds
+	 * @see RequestConfig#getSocketTimeout()
 	 */
 	public void setReadTimeout(int timeout) {
 		Assert.isTrue(timeout >= 0, "Timeout must be a non-negative value");
-		this.socketTimeout = timeout;
+		this.requestConfig = cloneRequestConfig()
+				.setSocketTimeout(timeout).build();
 		setLegacySocketTimeout(getHttpClient(), timeout);
 	}
 
@@ -177,6 +185,10 @@ private void setLegacySocketTimeout(HttpClient client, int timeout) {
 		}
 	}
 
+	private RequestConfig.Builder cloneRequestConfig() {
+		return this.requestConfig != null ? RequestConfig.copy(this.requestConfig) : RequestConfig.custom();
+	}
+
 	/**
 	 * Indicates whether this request factory should buffer the request body internally.
 	 * <p>Default is {@code true}. When sending large amounts of data via POST or PUT, it is
@@ -205,18 +217,11 @@ public ClientHttpRequest createRequest(URI uri, HttpMethod httpMethod) throws IO
 				config = ((Configurable) httpRequest).getConfig();
 			}
 			if (config == null) {
-				if (this.connectTimeout > 0 || this.connectionRequestTimeout > 0 || this.socketTimeout > 0) {
-					config = RequestConfig.custom()
-							.setConnectTimeout(this.connectTimeout)
-							.setConnectionRequestTimeout(this.connectionRequestTimeout)
-							.setSocketTimeout(this.socketTimeout)
-							.build();
-				}
-				else {
-					config = RequestConfig.DEFAULT;
-				}
+				config = this.requestConfig;
+			}
+			if (config != null) {
+				context.setAttribute(HttpClientContext.REQUEST_CONFIG, config);
 			}
-			context.setAttribute(HttpClientContext.REQUEST_CONFIG, config);
 		}
 		if (this.bufferRequestBody) {
 			return new HttpComponentsClientHttpRequest(client, httpRequest, context);

spring-web/src/main/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutor.java

@@ -69,27 +69,30 @@ public class HttpComponentsHttpInvokerRequestExecutor extends AbstractHttpInvoke
 	private static final int DEFAULT_READ_TIMEOUT_MILLISECONDS = (60 * 1000);
 
 	private HttpClient httpClient;
-	private int connectionTimeout = 0;
-	private int connectionRequestTimeout = 0;
-	private int readTimeout = DEFAULT_READ_TIMEOUT_MILLISECONDS;
 
+	private RequestConfig requestConfig;
 
 	/**
 	 * Create a new instance of the HttpComponentsHttpInvokerRequestExecutor with a default
 	 * {@link HttpClient} that uses a default {@code org.apache.http.impl.conn.PoolingClientConnectionManager}.
 	 */
 	public HttpComponentsHttpInvokerRequestExecutor() {
+		this(createDefaultHttpClient(), RequestConfig.custom()
+				.setSocketTimeout(DEFAULT_READ_TIMEOUT_MILLISECONDS).build());
+	}
+
+	private static HttpClient createDefaultHttpClient() {
 		Registry<ConnectionSocketFactory> schemeRegistry = RegistryBuilder.<ConnectionSocketFactory>create()
-				.register("http", PlainConnectionSocketFactory.getSocketFactory())
-				.register("https", SSLConnectionSocketFactory.getSocketFactory())
-				.build();
+			.register("http", PlainConnectionSocketFactory.getSocketFactory())
+			.register("https", SSLConnectionSocketFactory.getSocketFactory())
+			.build();
 
 		PoolingHttpClientConnectionManager connectionManager
 				= new PoolingHttpClientConnectionManager(schemeRegistry);
 		connectionManager.setMaxTotal(DEFAULT_MAX_TOTAL_CONNECTIONS);
 		connectionManager.setDefaultMaxPerRoute(DEFAULT_MAX_CONNECTIONS_PER_ROUTE);
 
-		this.httpClient = HttpClientBuilder.create().setConnectionManager(connectionManager).build();
+		return HttpClientBuilder.create().setConnectionManager(connectionManager).build();
 	}
 
 	/**
@@ -98,9 +101,13 @@ public HttpComponentsHttpInvokerRequestExecutor() {
 	 * @param httpClient the HttpClient instance to use for this request executor
 	 */
 	public HttpComponentsHttpInvokerRequestExecutor(HttpClient httpClient) {
-		this.httpClient = httpClient;
+		this(httpClient, null);
 	}
 
+	private HttpComponentsHttpInvokerRequestExecutor(HttpClient httpClient, RequestConfig requestConfig) {
+		this.httpClient = httpClient;
+		this.requestConfig = requestConfig;
+	}
 
 	/**
 	 * Set the {@link HttpClient} instance to use for this request executor.
@@ -119,11 +126,15 @@ public HttpClient getHttpClient() {
 	/**
 	 * Set the connection timeout for the underlying HttpClient.
 	 * A timeout value of 0 specifies an infinite timeout.
+	 * <p>Additional properties can be configured by specifying a
+	 * {@link RequestConfig} instance on a custom {@link HttpClient}.
 	 * @param timeout the timeout value in milliseconds
+	 * @see RequestConfig#getConnectTimeout()
 	 */
 	public void setConnectTimeout(int timeout) {
 		Assert.isTrue(timeout >= 0, "Timeout must be a non-negative value");
-		this.connectionTimeout = timeout;
+		this.requestConfig = cloneRequestConfig()
+				.setConnectTimeout(timeout).build();
 		setLegacyConnectionTimeout(getHttpClient(), timeout);
 	}
 
@@ -153,21 +164,29 @@ private void setLegacyConnectionTimeout(HttpClient client, int timeout) {
 	 * Set the timeout in milliseconds used when requesting a connection from the connection
 	 * manager using the underlying HttpClient.
 	 * A timeout value of 0 specifies an infinite timeout.
+	 * <p>Additional properties can be configured by specifying a
+	 * {@link RequestConfig} instance on a custom {@link HttpClient}.
 	 * @param connectionRequestTimeout the timeout value to request a connection in milliseconds
+	 * @see RequestConfig#getConnectionRequestTimeout()
 	 */
 	public void setConnectionRequestTimeout(int connectionRequestTimeout) {
-		this.connectionRequestTimeout = connectionRequestTimeout;
+		this.requestConfig = cloneRequestConfig()
+				.setConnectionRequestTimeout(connectionRequestTimeout).build();
 	}
 
 	/**
 	 * Set the socket read timeout for the underlying HttpClient.
 	 * A timeout value of 0 specifies an infinite timeout.
+	 * <p>Additional properties can be configured by specifying a
+	 * {@link RequestConfig} instance on a custom {@link HttpClient}.
 	 * @param timeout the timeout value in milliseconds
 	 * @see #DEFAULT_READ_TIMEOUT_MILLISECONDS
+	 * @see RequestConfig#getSocketTimeout()
 	 */
 	public void setReadTimeout(int timeout) {
 		Assert.isTrue(timeout >= 0, "Timeout must be a non-negative value");
-		this.readTimeout = timeout;
+		this.requestConfig = cloneRequestConfig()
+				.setSocketTimeout(timeout).build();
 		setLegacySocketTimeout(getHttpClient(), timeout);
 	}
 
@@ -186,6 +205,10 @@ private void setLegacySocketTimeout(HttpClient client, int timeout) {
 		}
 	}
 
+	private RequestConfig.Builder cloneRequestConfig() {
+		return this.requestConfig != null ? RequestConfig.copy(this.requestConfig) : RequestConfig.custom();
+	}
+
 	/**
 	 * Execute the given request through the HttpClient.
 	 * <p>This method implements the basic processing workflow:
@@ -251,16 +274,7 @@ protected HttpPost createHttpPost(HttpInvokerClientConfiguration config) throws
 	 * @return the RequestConfig to use
 	 */
 	protected RequestConfig createRequestConfig(HttpInvokerClientConfiguration config) {
-		if (this.connectionTimeout > 0 || this.connectionRequestTimeout > 0 || this.readTimeout > 0) {
-			return RequestConfig.custom()
-					.setConnectTimeout(this.connectionTimeout)
-					.setConnectionRequestTimeout(this.connectionRequestTimeout)
-					.setSocketTimeout(this.readTimeout)
-					.build();
-		}
-		else {
-			return RequestConfig.DEFAULT;
-		}
+		return (this.requestConfig != null ? this.requestConfig : null);
 	}
 
 	/**

spring-web/src/test/java/org/springframework/http/client/HttpComponentsClientHttpRequestFactoryTests.java

@@ -16,23 +16,20 @@
 
 package org.springframework.http.client;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertTrue;
-
 import java.net.URI;
 
 import org.apache.http.HttpEntityEnclosingRequest;
 import org.apache.http.client.HttpClient;
 import org.apache.http.client.config.RequestConfig;
 import org.apache.http.client.methods.HttpUriRequest;
 import org.apache.http.client.protocol.HttpClientContext;
-import org.apache.http.impl.client.DefaultHttpClient;
+import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClientBuilder;
-import org.apache.http.params.CoreConnectionPNames;
 import org.junit.Test;
 import org.springframework.http.HttpMethod;
 
+import static org.junit.Assert.*;
+
 public class HttpComponentsClientHttpRequestFactoryTests extends AbstractHttpRequestFactoryTestCase {
 
 	@Override
@@ -49,13 +46,15 @@ public void httpMethods() throws Exception {
 	@SuppressWarnings("deprecation")
 	@Test
 	public void assertLegacyCustomConfig() {
-		HttpClient httpClient = new DefaultHttpClient(); // Does not support RequestConfig
+		HttpClient httpClient = new org.apache.http.impl.client.DefaultHttpClient(); // Does not support RequestConfig
 		HttpComponentsClientHttpRequestFactory hrf = new HttpComponentsClientHttpRequestFactory(httpClient);
 		hrf.setConnectTimeout(1234);
-		assertEquals(1234, httpClient.getParams().getIntParameter(CoreConnectionPNames.CONNECTION_TIMEOUT, 0));
+		assertEquals(1234, httpClient.getParams().getIntParameter(
+				org.apache.http.params.CoreConnectionPNames.CONNECTION_TIMEOUT, 0));
 
 		hrf.setReadTimeout(4567);
-		assertEquals(4567, httpClient.getParams().getIntParameter(CoreConnectionPNames.SO_TIMEOUT, 0));
+		assertEquals(4567, httpClient.getParams().getIntParameter(
+				org.apache.http.params.CoreConnectionPNames.SO_TIMEOUT, 0));
 	}
 
 	@Test
@@ -80,6 +79,45 @@ public void assertCustomConfig() throws Exception {
 		assertEquals("Wrong custom socket timeout", 4567, requestConfig.getSocketTimeout());
 	}
 
+	@Test
+	public void customHttpClientUsesItsDefault() throws Exception {
+		HttpComponentsClientHttpRequestFactory hrf =
+				new HttpComponentsClientHttpRequestFactory(HttpClientBuilder.create().build());
+
+		URI uri = new URI(baseUrl + "/status/ok");
+		HttpComponentsClientHttpRequest request = (HttpComponentsClientHttpRequest)
+				hrf.createRequest(uri, HttpMethod.GET);
+
+		assertNull("No custom config should be set with a custom HttpClient",
+				request.getHttpContext().getAttribute(HttpClientContext.REQUEST_CONFIG));
+	}
+
+	@Test
+	public void defaultSettingsOfHttpClientLostOnExecutorCustomization() throws Exception {
+		CloseableHttpClient client = HttpClientBuilder.create()
+				.setDefaultRequestConfig(RequestConfig.custom().setConnectTimeout(1234).build())
+				.build();
+		HttpComponentsClientHttpRequestFactory hrf = new HttpComponentsClientHttpRequestFactory(client);
+
+		URI uri = new URI(baseUrl + "/status/ok");
+		HttpComponentsClientHttpRequest request = (HttpComponentsClientHttpRequest)
+				hrf.createRequest(uri, HttpMethod.GET);
+
+		assertNull("No custom config should be set with a custom HttpClient",
+				request.getHttpContext().getAttribute(HttpClientContext.REQUEST_CONFIG));
+
+		hrf.setConnectionRequestTimeout(4567);
+		HttpComponentsClientHttpRequest request2 = (HttpComponentsClientHttpRequest)
+				hrf.createRequest(uri, HttpMethod.GET);
+		Object requestConfigAttribute = request2.getHttpContext().getAttribute(HttpClientContext.REQUEST_CONFIG);
+		assertNotNull(requestConfigAttribute);
+		RequestConfig requestConfig = (RequestConfig) requestConfigAttribute;
+
+		assertEquals(4567, requestConfig.getConnectionRequestTimeout());
+		// No way to access the request config of the HTTP client so no way to "merge" our customizations
+		assertEquals(-1, requestConfig.getConnectTimeout());
+	}
+
 	@Test
 	public void createHttpUriRequest() throws Exception {
 		URI uri = new URI("http://example.com");

spring-web/src/test/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutorTests.java

@@ -1,3 +1,19 @@
+/*
+ * Copyright 2002-2014 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
+ *
+ *      http://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.remoting.httpinvoker;
 
 import java.io.IOException;
@@ -63,6 +79,36 @@ public void customizeReadTimeout() throws IOException {
 		assertEquals(10000, httpPost.getConfig().getSocketTimeout());
 	}
 
+	@Test
+	public void customHttpClientUsesItsDefault() throws IOException {
+		HttpComponentsHttpInvokerRequestExecutor executor =
+				new HttpComponentsHttpInvokerRequestExecutor(HttpClientBuilder.create().build());
+
+		HttpInvokerClientConfiguration config = mockHttpInvokerClientConfiguration("http://fake-service");
+		HttpPost httpPost = executor.createHttpPost(config);
+		assertNull("No custom config should be set with a custom HttpClient", httpPost.getConfig());
+	}
+
+	@Test
+	public void defaultSettingsOfHttpClientLostOnExecutorCustomization() throws IOException {
+		CloseableHttpClient client = HttpClientBuilder.create()
+				.setDefaultRequestConfig(RequestConfig.custom().setConnectTimeout(1234).build())
+				.build();
+		HttpComponentsHttpInvokerRequestExecutor executor =
+				new HttpComponentsHttpInvokerRequestExecutor(client);
+
+		HttpInvokerClientConfiguration config = mockHttpInvokerClientConfiguration("http://fake-service");
+		HttpPost httpPost = executor.createHttpPost(config);
+		assertNull("No custom config should be set with a custom HttpClient", httpPost.getConfig());
+
+		executor.setConnectionRequestTimeout(4567);
+		HttpPost httpPost2 = executor.createHttpPost(config);
+		assertNotNull(httpPost2.getConfig());
+		assertEquals(4567, httpPost2.getConfig().getConnectionRequestTimeout());
+		// No way to access the request config of the HTTP client so no way to "merge" our customizations
+		assertEquals(-1, httpPost2.getConfig().getConnectTimeout());
+	}
+
 	@Test
 	public void ignoreFactorySettings() throws IOException {
 		CloseableHttpClient httpClient = HttpClientBuilder.create().build();