From 19d991a67b5819460ef34a6c65895659581bcf1b Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Tue, 13 Sep 2022 15:53:01 +0200 Subject: [PATCH] Revisit resource cleanup in ClientHttpRequestFactory Prior to this commit, resource management around `ClientHttpRequestFactory` and `RestTemplate` was unclear. Some factories implementation were implementing a `DisposableBean` and other contracts were not managing request factory resources. In the meantime, neither `ClientHttpRequestFactory` nor `RestTemplate` are typically meant to be contributed as beans to the application context. Most often, they're instantiated within beans and their lifecycle should be managed by those. This commit makes all `ClientHttpRequestFactory` `Closeable` and ensures that all existing implementations have a similar behavior between `dispose()` and `close()`. Since `RestTemplate` (actually `HttpAccessor`) can instantiate factories on its own, they also now extend `Closeable` to properly close those resources, if not externally managed. Closes gh-29010 --- .../http/client/ClientHttpRequestFactory.java | 9 +++-- ...ttpComponentsClientHttpRequestFactory.java | 8 +++-- .../OkHttp3ClientHttpRequestFactory.java | 8 +++-- .../http/client/support/HttpAccessor.java | 18 ++++++++-- .../AbstractHttpRequestFactoryTests.java | 13 +++---- ...mponentsClientHttpRequestFactoryTests.java | 36 ++++++++++--------- 6 files changed, 58 insertions(+), 34 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/client/ClientHttpRequestFactory.java b/spring-web/src/main/java/org/springframework/http/client/ClientHttpRequestFactory.java index 2c433fb6fee..f2ea5497927 100644 --- a/spring-web/src/main/java/org/springframework/http/client/ClientHttpRequestFactory.java +++ b/spring-web/src/main/java/org/springframework/http/client/ClientHttpRequestFactory.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2022 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. @@ -16,6 +16,7 @@ package org.springframework.http.client; +import java.io.Closeable; import java.io.IOException; import java.net.URI; @@ -29,7 +30,7 @@ import org.springframework.http.HttpMethod; * @since 3.0 */ @FunctionalInterface -public interface ClientHttpRequestFactory { +public interface ClientHttpRequestFactory extends Closeable { /** * Create a new {@link ClientHttpRequest} for the specified URI and HTTP method. @@ -42,4 +43,8 @@ public interface ClientHttpRequestFactory { */ ClientHttpRequest createRequest(URI uri, HttpMethod httpMethod) throws IOException; + @Override + default void close() throws IOException { + + } } 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 dbf763ac3b8..5e9773cffc7 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2022 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. @@ -329,13 +329,17 @@ public class HttpComponentsClientHttpRequestFactory implements ClientHttpRequest */ @Override public void destroy() throws Exception { + close(); + } + + @Override + public void close() throws IOException { HttpClient httpClient = getHttpClient(); if (httpClient instanceof Closeable) { ((Closeable) httpClient).close(); } } - /** * An alternative to {@link org.apache.http.client.methods.HttpDelete} that * extends {@link org.apache.http.client.methods.HttpEntityEnclosingRequestBase} diff --git a/spring-web/src/main/java/org/springframework/http/client/OkHttp3ClientHttpRequestFactory.java b/spring-web/src/main/java/org/springframework/http/client/OkHttp3ClientHttpRequestFactory.java index e81bbe61a4c..d321652af97 100644 --- a/spring-web/src/main/java/org/springframework/http/client/OkHttp3ClientHttpRequestFactory.java +++ b/spring-web/src/main/java/org/springframework/http/client/OkHttp3ClientHttpRequestFactory.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2022 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. @@ -107,6 +107,11 @@ public class OkHttp3ClientHttpRequestFactory implements ClientHttpRequestFactory @Override public void destroy() throws IOException { + close(); + } + + @Override + public void close() throws IOException { if (this.defaultClient) { // Clean up the client if we created it in the constructor Cache cache = this.client.cache(); @@ -118,7 +123,6 @@ public class OkHttp3ClientHttpRequestFactory implements ClientHttpRequestFactory } } - static Request buildRequest(HttpHeaders headers, byte[] content, URI uri, HttpMethod method) throws MalformedURLException { diff --git a/spring-web/src/main/java/org/springframework/http/client/support/HttpAccessor.java b/spring-web/src/main/java/org/springframework/http/client/support/HttpAccessor.java index e2d59b63470..648c808fd5a 100644 --- a/spring-web/src/main/java/org/springframework/http/client/support/HttpAccessor.java +++ b/spring-web/src/main/java/org/springframework/http/client/support/HttpAccessor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2022 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. @@ -16,6 +16,7 @@ package org.springframework.http.client.support; +import java.io.Closeable; import java.io.IOException; import java.net.URI; import java.util.ArrayList; @@ -48,7 +49,7 @@ import org.springframework.util.Assert; * @see ClientHttpRequestFactory * @see org.springframework.web.client.RestTemplate */ -public abstract class HttpAccessor { +public abstract class HttpAccessor implements Closeable { /** Logger available to subclasses. */ protected final Log logger = HttpLogging.forLogName(getClass()); @@ -66,7 +67,7 @@ public abstract class HttpAccessor { * Configure the Apache HttpComponents or OkHttp request factory to enable PATCH. * @see #createRequest(URI, HttpMethod) * @see SimpleClientHttpRequestFactory - * @see org.springframework.http.client.HttpComponentsAsyncClientHttpRequestFactory + * @see org.springframework.http.client.HttpComponentsClientHttpRequestFactory * @see org.springframework.http.client.OkHttp3ClientHttpRequestFactory */ public void setRequestFactory(ClientHttpRequestFactory requestFactory) { @@ -133,4 +134,15 @@ public abstract class HttpAccessor { this.clientHttpRequestInitializers.forEach(initializer -> initializer.initialize(request)); } + /** + * Close the underlying {@link ClientHttpRequestFactory}. + *

This should not be called if the factory has been {@link #setRequestFactory(ClientHttpRequestFactory) set} + * and is externally managed. + * @throws IOException if the request factory cannot be closed properly + */ + @Override + public void close() throws IOException { + this.requestFactory.close(); + } + } diff --git a/spring-web/src/test/java/org/springframework/http/client/AbstractHttpRequestFactoryTests.java b/spring-web/src/test/java/org/springframework/http/client/AbstractHttpRequestFactoryTests.java index 427f21f6190..652efe58b06 100644 --- a/spring-web/src/test/java/org/springframework/http/client/AbstractHttpRequestFactoryTests.java +++ b/spring-web/src/test/java/org/springframework/http/client/AbstractHttpRequestFactoryTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2022 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. @@ -25,7 +25,6 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.springframework.beans.factory.DisposableBean; import org.springframework.beans.factory.InitializingBean; import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; @@ -47,17 +46,15 @@ abstract class AbstractHttpRequestFactoryTests extends AbstractMockWebServerTest @BeforeEach final void createFactory() throws Exception { - factory = createRequestFactory(); - if (factory instanceof InitializingBean) { - ((InitializingBean) factory).afterPropertiesSet(); + this.factory = createRequestFactory(); + if (this.factory instanceof InitializingBean) { + ((InitializingBean) this.factory).afterPropertiesSet(); } } @AfterEach final void destroyFactory() throws Exception { - if (factory instanceof DisposableBean) { - ((DisposableBean) factory).destroy(); - } + this.factory.close(); } 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 1df324c8dc4..d05969fab67 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2022 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. @@ -55,22 +55,24 @@ public class HttpComponentsClientHttpRequestFactoryTests extends AbstractHttpReq @Test public void assertCustomConfig() throws Exception { HttpClient httpClient = HttpClientBuilder.create().build(); - HttpComponentsClientHttpRequestFactory hrf = new HttpComponentsClientHttpRequestFactory(httpClient); - hrf.setConnectTimeout(1234); - hrf.setConnectionRequestTimeout(4321); - hrf.setReadTimeout(4567); - - URI uri = new URI(baseUrl + "/status/ok"); - HttpComponentsClientHttpRequest request = (HttpComponentsClientHttpRequest) - hrf.createRequest(uri, HttpMethod.GET); - - Object config = request.getHttpContext().getAttribute(HttpClientContext.REQUEST_CONFIG); - assertThat(config).as("Request config should be set").isNotNull(); - assertThat(config).as("Wrong request config type " + config.getClass().getName()).isInstanceOf(RequestConfig.class); - RequestConfig requestConfig = (RequestConfig) config; - assertThat(requestConfig.getConnectTimeout()).as("Wrong custom connection timeout").isEqualTo(1234); - assertThat(requestConfig.getConnectionRequestTimeout()).as("Wrong custom connection request timeout").isEqualTo(4321); - assertThat(requestConfig.getSocketTimeout()).as("Wrong custom socket timeout").isEqualTo(4567); + HttpComponentsClientHttpRequest request; + try (HttpComponentsClientHttpRequestFactory hrf = new HttpComponentsClientHttpRequestFactory(httpClient)) { + hrf.setConnectTimeout(1234); + hrf.setConnectionRequestTimeout(4321); + hrf.setReadTimeout(4567); + + URI uri = new URI(baseUrl + "/status/ok"); + request = (HttpComponentsClientHttpRequest) + hrf.createRequest(uri, HttpMethod.GET); + + Object config = request.getHttpContext().getAttribute(HttpClientContext.REQUEST_CONFIG); + assertThat(config).as("Request config should be set").isNotNull(); + assertThat(config).as("Wrong request config type " + config.getClass().getName()).isInstanceOf(RequestConfig.class); + RequestConfig requestConfig = (RequestConfig) config; + assertThat(requestConfig.getConnectTimeout()).as("Wrong custom connection timeout").isEqualTo(1234); + assertThat(requestConfig.getConnectionRequestTimeout()).as("Wrong custom connection request timeout").isEqualTo(4321); + assertThat(requestConfig.getSocketTimeout()).as("Wrong custom socket timeout").isEqualTo(4567); + } } @Test