Polishing.

Related: #1164.

Author: gregturn

spring-ws-core/src/main/java/org/springframework/ws/transport/http/HttpComponents5ClientFactory.java

@@ -18,8 +18,8 @@
 
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.time.Duration;
 import java.util.Map;
-import java.util.concurrent.TimeUnit;
 
 import org.apache.hc.client5.http.HttpRoute;
 import org.apache.hc.client5.http.auth.AuthScope;
@@ -31,25 +31,28 @@
 import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager;
 import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManagerBuilder;
 import org.apache.hc.core5.http.HttpHost;
-
+import org.apache.hc.core5.util.Timeout;
 import org.springframework.beans.factory.FactoryBean;
 
 /**
- * {@code FactoryBean} to set up a <a href="http://hc.apache.org/httpcomponents-client">Apache
- * CloseableHttpClient</a>
+ * {@link FactoryBean} to set up a {@link CloseableHttpClient} using HttpComponents HttpClient 5.
  *
+ * @see http://hc.apache.org/httpcomponents-client
  * @author Lars Uffmann
  * @since 4.0.5
  */
 public class HttpComponents5ClientFactory implements FactoryBean<CloseableHttpClient> {
-	private static final int DEFAULT_CONNECTION_TIMEOUT_MILLISECONDS = (60 * 1000);
 
-	private int connectionTimeout = DEFAULT_CONNECTION_TIMEOUT_MILLISECONDS;
-	private static final int DEFAULT_READ_TIMEOUT_MILLISECONDS = (60 * 1000);
+	private static final Duration DEFAULT_CONNECTION_TIMEOUT = Duration.ofSeconds(60);
+
+	private static final Duration DEFAULT_READ_TIMEOUT = Duration.ofSeconds(60);
+
+	private Duration connectionTimeout = DEFAULT_CONNECTION_TIMEOUT;
 
-	private int readTimeout = DEFAULT_READ_TIMEOUT_MILLISECONDS;
+	private Duration readTimeout = DEFAULT_READ_TIMEOUT;
 
 	private int maxTotalConnections = -1;
+
 	private AuthScope authScope = null;
 
 	private Credentials credentials = null;
@@ -86,24 +89,28 @@ public void setAuthScope(AuthScope authScope) {
 	/**
 	 * Sets the timeout until a connection is established. A value of 0 means <em>never</em> timeout.
 	 *
-	 * @param timeout the timeout value in milliseconds
+	 * @param timeout the timeout value
 	 */
-	public void setConnectionTimeout(int timeout) {
-		if (timeout < 0) {
+	public void setConnectionTimeout(Duration timeout) {
+
+		if (timeout.isNegative()) {
 			throw new IllegalArgumentException("timeout must be a non-negative value");
 		}
+
 		this.connectionTimeout = timeout;
 	}
 
 	/**
 	 * Set the socket read timeout for the underlying HttpClient. A value of 0 means <em>never</em> timeout.
 	 *
-	 * @param timeout the timeout value in milliseconds
+	 * @param timeout the timeout value
 	 */
-	public void setReadTimeout(int timeout) {
-		if (timeout < 0) {
+	public void setReadTimeout(Duration timeout) {
+
+		if (timeout.isNegative()) {
 			throw new IllegalArgumentException("timeout must be a non-negative value");
 		}
+
 		this.readTimeout = timeout;
 	}
 
@@ -114,9 +121,11 @@ public void setReadTimeout(int timeout) {
 	 * @see PoolingHttpClientConnectionManager...
 	 */
 	public void setMaxTotalConnections(int maxTotalConnections) {
+
 		if (maxTotalConnections <= 0) {
 			throw new IllegalArgumentException("maxTotalConnections must be a positive value");
 		}
+
 		this.maxTotalConnections = maxTotalConnections;
 	}
 
@@ -142,14 +151,14 @@ public void setMaxConnectionsPerHost(Map<String, String> maxConnectionsPerHost)
 	void applyMaxConnectionsPerHost(PoolingHttpClientConnectionManager connectionManager) throws URISyntaxException {
 
 		for (Map.Entry<String, String> entry : maxConnectionsPerHost.entrySet()) {
+
 			URI uri = new URI(entry.getKey());
 			HttpHost host = new HttpHost(uri.getScheme(), uri.getHost(), getPort(uri));
 			final HttpRoute route;
 
 			if (uri.getScheme().equals("https")) {
 				route = new HttpRoute(host, null, true);
-			}
-			else {
+			} else {
 				route = new HttpRoute(host);
 			}
 			int max = Integer.parseInt(entry.getValue());
@@ -158,14 +167,17 @@ void applyMaxConnectionsPerHost(PoolingHttpClientConnectionManager connectionMan
 	}
 
 	static int getPort(URI uri) {
+
 		if (uri.getPort() == -1) {
+
 			if ("https".equalsIgnoreCase(uri.getScheme())) {
 				return 443;
 			}
 			if ("http".equalsIgnoreCase(uri.getScheme())) {
 				return 80;
 			}
 		}
+
 		return uri.getPort();
 	}
 
@@ -176,34 +188,38 @@ public boolean isSingleton() {
 
 	@Override
 	public CloseableHttpClient getObject() throws Exception {
-		PoolingHttpClientConnectionManagerBuilder connectionManagerBuilder = PoolingHttpClientConnectionManagerBuilder.create();
+
+		PoolingHttpClientConnectionManagerBuilder connectionManagerBuilder = PoolingHttpClientConnectionManagerBuilder
+				.create();
+
 		if (this.maxTotalConnections != -1) {
 			connectionManagerBuilder.setMaxConnTotal(this.maxTotalConnections);
 		}
 
-		if (null != this.connectionManagerBuilderCustomizer) {
+		if (this.connectionManagerBuilderCustomizer != null) {
 			this.connectionManagerBuilderCustomizer.customize(connectionManagerBuilder);
 		}
 
 		this.connectionManager = connectionManagerBuilder.build();
 
 		applyMaxConnectionsPerHost(connectionManager);
 
-		RequestConfig.Builder requestConfigBuilder = RequestConfig.custom()
-				.setConnectTimeout(connectionTimeout, TimeUnit.MILLISECONDS)
-				.setResponseTimeout(readTimeout, TimeUnit.MILLISECONDS);
+		RequestConfig.Builder requestConfigBuilder = RequestConfig.custom() //
+				.setConnectionRequestTimeout(Timeout.of(connectionTimeout)) //
+				.setResponseTimeout(Timeout.of(readTimeout));
 
-		HttpClientBuilder httpClientBuilder = HttpClientBuilder.create()
-				.setDefaultRequestConfig(requestConfigBuilder.build())
+		HttpClientBuilder httpClientBuilder = HttpClientBuilder.create() //
+				.setDefaultRequestConfig(requestConfigBuilder.build()) //
 				.setConnectionManager(connectionManager);
 
-		if (null != credentials && null != authScope) {
+		if (credentials != null && authScope != null) {
+
 			BasicCredentialsProvider basicCredentialsProvider = new BasicCredentialsProvider();
 			basicCredentialsProvider.setCredentials(authScope, credentials);
 			httpClientBuilder.setDefaultCredentialsProvider(basicCredentialsProvider);
 		}
 
-		if (null != this.clientBuilderCustomizer) {
+		if (this.clientBuilderCustomizer != null) {
 			clientBuilderCustomizer.customize(httpClientBuilder);
 		}
 
@@ -223,7 +239,8 @@ public void setClientBuilderCustomizer(HttpClientBuilderCustomizer clientBuilder
 		this.clientBuilderCustomizer = clientBuilderCustomizer;
 	}
 
-	public void setConnectionManagerBuilderCustomizer(PoolingHttpClientConnectionManagerBuilderCustomizer connectionManagerBuilderCustomizer) {
+	public void setConnectionManagerBuilderCustomizer(
+			PoolingHttpClientConnectionManagerBuilderCustomizer connectionManagerBuilderCustomizer) {
 		this.connectionManagerBuilderCustomizer = connectionManagerBuilderCustomizer;
 	}
 

spring-ws-core/src/main/java/org/springframework/ws/transport/http/HttpComponents5Connection.java

@@ -28,20 +28,19 @@
 import org.apache.hc.client5.http.classic.HttpClient;
 import org.apache.hc.client5.http.classic.methods.HttpPost;
 import org.apache.hc.core5.http.ClassicHttpResponse;
-import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.HttpEntity;
 import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.NameValuePair;
 import org.apache.hc.core5.http.io.entity.ByteArrayEntity;
 import org.apache.hc.core5.http.io.entity.EntityUtils;
 import org.apache.hc.core5.http.protocol.HttpContext;
-
 import org.springframework.util.Assert;
 import org.springframework.ws.WebServiceMessage;
 import org.springframework.ws.transport.WebServiceConnection;
 
 /**
- * Implementation of {@link WebServiceConnection} that is based on Apache HttpClient. Exposes a {@link org.apache.hc.client5.http.classic.methods.HttpPost} and
- * {@link org.apache.hc.core5.http.HttpResponse
+ * Implementation of {@link WebServiceConnection} that is based on Apache HttpClient 5. Exposes a {@link HttpPost} and
+ * {@link HttpResponse}.
  *
  * @author Alan Stewart
  * @author Barry Pitman
@@ -63,8 +62,10 @@ public class HttpComponents5Connection extends AbstractHttpSenderConnection {
 	private ByteArrayOutputStream requestBuffer;
 
 	protected HttpComponents5Connection(HttpClient httpClient, HttpPost httpPost, HttpContext httpContext) {
+
 		Assert.notNull(httpClient, "httpClient must not be null");
 		Assert.notNull(httpPost, "httpPost must not be null");
+
 		this.httpClient = httpClient;
 		this.httpPost = httpPost;
 		this.httpContext = httpContext;
@@ -80,8 +81,9 @@ public HttpResponse getHttpResponse() {
 
 	@Override
 	public void onClose() throws IOException {
-		//XXX:
+
 		if (httpResponse instanceof ClassicHttpResponse response) {
+
 			if (response.getEntity() != null) {
 				EntityUtils.consume(response.getEntity());
 			}
@@ -117,9 +119,10 @@ protected OutputStream getRequestOutputStream() throws IOException {
 
 	@Override
 	protected void onSendAfterWrite(WebServiceMessage message) throws IOException {
-		//XXX
+
 		httpPost.setEntity(new ByteArrayEntity(requestBuffer.toByteArray(), null));
 		requestBuffer = null;
+
 		if (httpContext != null) {
 			httpResponse = httpClient.execute(httpPost, httpContext);
 		} else {
@@ -143,8 +146,9 @@ protected String getResponseMessage() throws IOException {
 
 	@Override
 	protected long getResponseContentLength() throws IOException {
-		//XXX:
+
 		if (httpResponse instanceof ClassicHttpResponse response) {
+
 			HttpEntity entity = response.getEntity();
 			if (entity != null) {
 				return entity.getContentLength();
@@ -155,32 +159,31 @@ protected long getResponseContentLength() throws IOException {
 
 	@Override
 	protected InputStream getRawResponseInputStream() throws IOException {
+
 		if (httpResponse instanceof ClassicHttpResponse response) {
+
 			HttpEntity entity = response.getEntity();
 			if (entity != null) {
 				return entity.getContent();
 			}
 		}
+
 		throw new IllegalStateException("Response has no enclosing response entity, cannot create input stream");
 	}
 
 	@Override
 	public Iterator<String> getResponseHeaderNames() throws IOException {
-		Header[] headers = httpResponse.getHeaders();
-		String[] names = new String[headers.length];
-		for (int i = 0; i < headers.length; i++) {
-			names[i] = headers[i].getName();
-		}
-		return Arrays.asList(names).iterator();
+
+		return Arrays.stream(httpResponse.getHeaders()) //
+				.map(NameValuePair::getName) //
+				.iterator();
 	}
 
 	@Override
 	public Iterator<String> getResponseHeaders(String name) throws IOException {
-		Header[] headers = httpResponse.getHeaders(name);
-		String[] values = new String[headers.length];
-		for (int i = 0; i < headers.length; i++) {
-			values[i] = headers[i].getValue();
-		}
-		return Arrays.asList(values).iterator();
+
+		return Arrays.stream(httpResponse.getHeaders(name)) //
+				.map(NameValuePair::getValue) //
+				.iterator();
 	}
 }

spring-ws-core/src/main/java/org/springframework/ws/transport/http/HttpComponents5MessageSender.java

@@ -18,6 +18,7 @@
 
 import java.io.IOException;
 import java.net.URI;
+import java.time.Duration;
 import java.util.Map;
 
 import org.apache.hc.client5.http.auth.AuthScope;
@@ -33,7 +34,6 @@
 import org.apache.hc.core5.http.HttpRequest;
 import org.apache.hc.core5.http.HttpRequestInterceptor;
 import org.apache.hc.core5.http.protocol.HttpContext;
-
 import org.springframework.beans.factory.DisposableBean;
 import org.springframework.beans.factory.InitializingBean;
 import org.springframework.util.Assert;
@@ -57,7 +57,9 @@
  */
 public class HttpComponents5MessageSender extends AbstractHttpWebServiceMessageSender
 		implements InitializingBean, DisposableBean {
+
 	private static final String HTTP_CLIENT_ALREADY_SET = "httpClient already set";
+
 	private HttpClient httpClient;
 
 	private HttpComponents5ClientFactory clientFactory;
@@ -67,22 +69,23 @@ public class HttpComponents5MessageSender extends AbstractHttpWebServiceMessageS
 	 * {@link PoolingHttpClientConnectionManager}.
 	 */
 	public HttpComponents5MessageSender() {
+
 		this.clientFactory = new HttpComponents5ClientFactory();
-		this.clientFactory.setClientBuilderCustomizer(httpClientBuilder ->
-				httpClientBuilder.addRequestInterceptorFirst(new RemoveSoapHeadersInterceptor()));
+		this.clientFactory.setClientBuilderCustomizer(
+				httpClientBuilder -> httpClientBuilder.addRequestInterceptorFirst(new RemoveSoapHeadersInterceptor()));
 	}
 
 	/**
-	 * Create a new instance of the {@code HttpClientMessageSender} with the given {@link HttpClient} instance.
+	 * Create a new instance of the {@link HttpComponents5MessageSender} with the given {@link HttpClient} instance.
 	 * <p>
 	 * This constructor does not change the given {@code HttpClient} in any way. As such, it does not set timeouts, nor
-	 * does it
-	 * {@linkplain HttpClientBuilder#addRequestInterceptorFirst(HttpRequestInterceptor)
-	 * add} the {@link RemoveSoapHeadersInterceptor}.
+	 * does it {@linkplain HttpClientBuilder#addRequestInterceptorFirst(HttpRequestInterceptor) add} the
+	 * {@link RemoveSoapHeadersInterceptor}.
 	 *
 	 * @param httpClient the HttpClient instance to use for this sender
 	 */
 	public HttpComponents5MessageSender(HttpClient httpClient) {
+
 		Assert.notNull(httpClient, "httpClient must not be null");
 		this.httpClient = httpClient;
 	}
@@ -91,19 +94,23 @@ public HttpComponents5MessageSender(HttpClient httpClient) {
 	 * @see HttpComponents5ClientFactory#setAuthScope(AuthScope)
 	 */
 	public void setAuthScope(AuthScope authScope) {
-		if (null != getHttpClient()) {
+
+		if (getHttpClient() != null) {
 			throw new IllegalStateException(HTTP_CLIENT_ALREADY_SET);
 		}
+
 		this.clientFactory.setAuthScope(authScope);
 	}
 
 	/**
 	 * @see HttpComponents5ClientFactory#setCredentials(Credentials)
 	 */
 	public void setCredentials(Credentials credentials) {
-		if (null != getHttpClient()) {
+
+		if (getHttpClient() != null) {
 			throw new IllegalStateException(HTTP_CLIENT_ALREADY_SET);
 		}
+
 		this.clientFactory.setCredentials(credentials);
 	}
 
@@ -122,42 +129,50 @@ public void setHttpClient(HttpClient httpClient) {
 	}
 
 	/**
-	 * @see HttpComponents5ClientFactory#setConnectionTimeout(int) 
+	 * @see HttpComponents5ClientFactory#setConnectionTimeout(Duration)
 	 */
-	public void setConnectionTimeout(int timeout) {
-		if (null != getHttpClient()) {
+	public void setConnectionTimeout(Duration timeout) {
+
+		if (getHttpClient() != null) {
 			throw new IllegalStateException(HTTP_CLIENT_ALREADY_SET);
 		}
+
 		this.clientFactory.setConnectionTimeout(timeout);
 	}
 
 	/**
-	 * @see HttpComponents5ClientFactory#setReadTimeout(int)
+	 * @see HttpComponents5ClientFactory#setReadTimeout(Duration)
 	 */
-	public void setReadTimeout(int timeout) {
-		if (null != getHttpClient()) {
+	public void setReadTimeout(Duration timeout) {
+
+		if (getHttpClient() != null) {
 			throw new IllegalStateException(HTTP_CLIENT_ALREADY_SET);
 		}
+
 		this.clientFactory.setReadTimeout(timeout);
 	}
 
 	/**
 	 * @see HttpComponents5ClientFactory#setMaxTotalConnections(int)
 	 */
 	public void setMaxTotalConnections(int maxTotalConnections) {
-		if (null != getHttpClient()) {
+
+		if (getHttpClient() != null) {
 			throw new IllegalStateException(HTTP_CLIENT_ALREADY_SET);
 		}
+
 		this.clientFactory.setMaxTotalConnections(maxTotalConnections);
 	}
 
 	/**
 	 * @see HttpComponents5ClientFactory#setMaxConnectionsPerHost(Map)
 	 */
 	public void setMaxConnectionsPerHost(Map<String, String> maxConnectionsPerHost) {
-		if (null != getHttpClient()) {
+
+		if (getHttpClient() != null) {
 			throw new IllegalStateException(HTTP_CLIENT_ALREADY_SET);
 		}
+
 		this.clientFactory.setMaxConnectionsPerHost(maxConnectionsPerHost);
 	}
 
@@ -168,16 +183,20 @@ public void afterPropertiesSet() throws Exception {
 
 	@Override
 	public WebServiceConnection createConnection(URI uri) throws IOException {
+
 		HttpPost httpPost = new HttpPost(uri);
+
 		if (isAcceptGzipEncoding()) {
 			httpPost.addHeader(HttpTransportConstants.HEADER_ACCEPT_ENCODING, HttpTransportConstants.CONTENT_ENCODING_GZIP);
 		}
+
 		HttpContext httpContext = createContext(uri);
+
 		return new HttpComponents5Connection(getHttpClient(), httpPost, httpContext);
 	}
 
 	/**
-	 * Template method that allows for creation of a {@link HttpContext} for the given uri. Default implementation returns
+	 * Template method that allows for creation of an {@link HttpContext} for the given uri. Default implementation returns
 	 * {@code null}.
 	 *
 	 * @param uri the URI to create the context for
@@ -189,7 +208,8 @@ protected HttpContext createContext(URI uri) {
 
 	@Override
 	public void destroy() throws Exception {
-		if (getHttpClient() instanceof CloseableHttpClient client) {
+
+		if (getHttpClient()instanceof CloseableHttpClient client) {
 			client.close();
 		}
 	}
@@ -200,11 +220,15 @@ public void destroy() throws Exception {
 	 * these headers themselves, and HttpClient throws an exception if they have been set.
 	 */
 	public static class RemoveSoapHeadersInterceptor implements HttpRequestInterceptor {
+
 		@Override
-		public void process(HttpRequest request, EntityDetails entityDetails, HttpContext httpContext) throws HttpException, IOException {
+		public void process(HttpRequest request, EntityDetails entityDetails, HttpContext httpContext)
+				throws HttpException, IOException {
+
 			if (request.containsHeader(HttpHeaders.TRANSFER_ENCODING)) {
 				request.removeHeaders(HttpHeaders.TRANSFER_ENCODING);
 			}
+
 			if (request.containsHeader(HttpHeaders.CONTENT_LENGTH)) {
 				request.removeHeaders(HttpHeaders.CONTENT_LENGTH);
 			}

spring-ws-core/src/test/java/org/springframework/ws/transport/http/HttpComponents5MessageSenderIntegrationTest.java

@@ -16,15 +16,19 @@
 
 package org.springframework.ws.transport.http;
 
-import java.io.IOException;
-import java.net.URI;
-import java.util.HashMap;
-import java.util.Map;
+import static org.assertj.core.api.AssertionsForClassTypes.*;
+import static org.springframework.ws.transport.http.HttpComponents5ClientFactory.*;
 
 import jakarta.servlet.http.HttpServlet;
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 import jakarta.xml.soap.MessageFactory;
+
+import java.io.IOException;
+import java.net.URI;
+import java.util.HashMap;
+import java.util.Map;
+
 import org.apache.hc.client5.http.HttpRoute;
 import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
 import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager;
@@ -34,15 +38,12 @@
 import org.eclipse.jetty.server.ServerConnector;
 import org.eclipse.jetty.servlet.ServletContextHandler;
 import org.junit.jupiter.api.Test;
-
 import org.springframework.context.support.StaticApplicationContext;
 import org.springframework.util.FileCopyUtils;
 import org.springframework.ws.soap.saaj.SaajSoapMessage;
 import org.springframework.ws.soap.saaj.SaajSoapMessageFactory;
 import org.springframework.ws.transport.WebServiceConnection;
 import org.springframework.ws.transport.support.FreePortScanner;
-import static org.assertj.core.api.AssertionsForClassTypes.*;
-import static org.springframework.ws.transport.http.HttpComponents5ClientFactory.*;
 
 class HttpComponents5MessageSenderIntegrationTest
 		extends AbstractHttpWebServiceMessageSenderIntegrationTestCase<HttpComponents5MessageSender> {
@@ -52,7 +53,7 @@ protected HttpComponents5MessageSender createMessageSender() {
 		return new HttpComponents5MessageSender();
 	}
 
-	@Test
+	@Test // GH-1164
 	void testMaxConnections() throws Exception {
 
 		final String url1 = "https://www.example.com";
@@ -103,7 +104,7 @@ void testMaxConnections() throws Exception {
 		assertThat(poolingHttpClientConnectionManager.getMaxPerRoute(route3)).isEqualTo(10);
 	}
 
-	@Test
+	@Test // GH-1164
 	void testContextClose() throws Exception {
 
 		MessageFactory messageFactory = MessageFactory.newInstance();
@@ -138,6 +139,7 @@ void testContextClose() throws Exception {
 
 			appContext.close();
 		} finally {
+
 			if (connection != null) {
 				try {
 					connection.close();