From 71783c5d86205c5e7820eaca02338c55ae33ea13 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Tue, 23 Dec 2014 17:36:07 +0100 Subject: [PATCH] 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 --- ...ttpComponentsClientHttpRequestFactory.java | 43 +++++++------- ...pComponentsHttpInvokerRequestExecutor.java | 56 ++++++++++++------- ...mponentsClientHttpRequestFactoryTests.java | 56 ++++++++++++++++--- ...onentsHttpInvokerRequestExecutorTests.java | 46 +++++++++++++++ 4 files changed, 152 insertions(+), 49 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/client/HttpComponentsClientHttpRequestFactory.java b/spring-web/src/main/java/org/springframework/http/client/HttpComponentsClientHttpRequestFactory.java index 059ee55f60..fbfbcb1138 100644 --- a/spring-web/src/main/java/org/springframework/http/client/HttpComponentsClientHttpRequestFactory.java +++ b/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 class HttpComponentsClientHttpRequestFactory implements ClientHttpRequest /** * Set the connection timeout for the underlying HttpClient. * A timeout value of 0 specifies an infinite timeout. + *

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 @@ public class HttpComponentsClientHttpRequestFactory implements ClientHttpRequest * 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. + *

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. + *

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 @@ public class HttpComponentsClientHttpRequestFactory implements ClientHttpRequest } } + 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. *

Default is {@code true}. When sending large amounts of data via POST or PUT, it is @@ -205,18 +217,11 @@ public class HttpComponentsClientHttpRequestFactory implements ClientHttpRequest 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); diff --git a/spring-web/src/main/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutor.java b/spring-web/src/main/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutor.java index 43dd667f17..68202e92d3 100644 --- a/spring-web/src/main/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutor.java +++ b/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 schemeRegistry = RegistryBuilder.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 class HttpComponentsHttpInvokerRequestExecutor extends AbstractHttpInvoke * @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 class HttpComponentsHttpInvokerRequestExecutor extends AbstractHttpInvoke /** * Set the connection timeout for the underlying HttpClient. * A timeout value of 0 specifies an infinite timeout. + *

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 @@ public class HttpComponentsHttpInvokerRequestExecutor extends AbstractHttpInvoke * 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. + *

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. + *

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 @@ public class HttpComponentsHttpInvokerRequestExecutor extends AbstractHttpInvoke } } + private RequestConfig.Builder cloneRequestConfig() { + return this.requestConfig != null ? RequestConfig.copy(this.requestConfig) : RequestConfig.custom(); + } + /** * Execute the given request through the HttpClient. *

This method implements the basic processing workflow: @@ -251,16 +274,7 @@ public class HttpComponentsHttpInvokerRequestExecutor extends AbstractHttpInvoke * @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); } /** diff --git a/spring-web/src/test/java/org/springframework/http/client/HttpComponentsClientHttpRequestFactoryTests.java b/spring-web/src/test/java/org/springframework/http/client/HttpComponentsClientHttpRequestFactoryTests.java index 8d0dae0e82..12efc4bc43 100644 --- a/spring-web/src/test/java/org/springframework/http/client/HttpComponentsClientHttpRequestFactoryTests.java +++ b/spring-web/src/test/java/org/springframework/http/client/HttpComponentsClientHttpRequestFactoryTests.java @@ -16,10 +16,6 @@ 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; @@ -27,12 +23,13 @@ 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 class HttpComponentsClientHttpRequestFactoryTests extends AbstractHttpReq @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 class HttpComponentsClientHttpRequestFactoryTests extends AbstractHttpReq 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"); diff --git a/spring-web/src/test/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutorTests.java b/spring-web/src/test/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutorTests.java index dd61f75346..a0faf3c228 100644 --- a/spring-web/src/test/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutorTests.java +++ b/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 class HttpComponentsHttpInvokerRequestExecutorTests { 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();