Remove buffering from ClientHttpRequest implementations

This commit ensures that ClientHttpRequest implementations implement
StreamingHttpOutputMessage, so that they do not expose an OutputStream,
but store a handle capable of writing to a stream instead.

Closes gh-30557

Author: poutsma

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2023 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,11 +16,11 @@
 
 package org.springframework.http.client;
 
-import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
 
 import org.springframework.http.HttpHeaders;
+import org.springframework.util.FastByteArrayOutputStream;
 
 /**
  * Base implementation of {@link ClientHttpRequest} that buffers output
@@ -31,7 +31,7 @@
  */
 abstract class AbstractBufferingClientHttpRequest extends AbstractClientHttpRequest {
 
-	private ByteArrayOutputStream bufferedOutput = new ByteArrayOutputStream(1024);
+	private final FastByteArrayOutputStream bufferedOutput = new FastByteArrayOutputStream(1024);
 
 
 	@Override
@@ -41,12 +41,12 @@ protected OutputStream getBodyInternal(HttpHeaders headers) throws IOException {
 
 	@Override
 	protected ClientHttpResponse executeInternal(HttpHeaders headers) throws IOException {
-		byte[] bytes = this.bufferedOutput.toByteArray();
+		byte[] bytes = this.bufferedOutput.toByteArrayUnsafe();
 		if (headers.getContentLength() < 0) {
 			headers.setContentLength(bytes.length);
 		}
 		ClientHttpResponse result = executeInternal(headers, bytes);
-		this.bufferedOutput = new ByteArrayOutputStream(0);
+		this.bufferedOutput.reset();
 		return result;
 	}
 

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

@@ -0,0 +1,83 @@
+/*
+ * Copyright 2002-2023 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
+ *
+ *      https://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.http.client;
+
+import java.io.IOException;
+import java.io.OutputStream;
+
+import org.springframework.http.HttpHeaders;
+import org.springframework.http.StreamingHttpOutputMessage;
+import org.springframework.lang.Nullable;
+import org.springframework.util.Assert;
+import org.springframework.util.FastByteArrayOutputStream;
+
+/**
+ * Abstract base for {@link ClientHttpRequest} that also implement
+ * {@link StreamingHttpOutputMessage}. Ensures that headers and
+ * body are not written multiple times.
+ *
+ * @author Arjen Poutsma
+ * @since 6.1
+ */
+abstract class AbstractStreamingClientHttpRequest extends AbstractClientHttpRequest
+		implements StreamingHttpOutputMessage {
+
+	@Nullable
+	private Body body;
+
+	@Nullable
+	private FastByteArrayOutputStream bodyStream;
+
+
+	@Override
+	protected final OutputStream getBodyInternal(HttpHeaders headers) {
+		Assert.state(this.body == null, "Invoke either getBody or setBody; not both");
+
+		if (this.bodyStream == null) {
+			this.bodyStream = new FastByteArrayOutputStream(1024);
+		}
+		return this.bodyStream;
+	}
+
+	@Override
+	public final void setBody(Body body) {
+		Assert.notNull(body, "Body must not be null");
+		assertNotExecuted();
+		Assert.state(this.bodyStream == null, "Invoke either getBody or setBody; not both");
+
+		this.body = body;
+	}
+
+	@Override
+	protected final ClientHttpResponse executeInternal(HttpHeaders headers) throws IOException {
+		if (this.body == null && this.bodyStream != null) {
+			this.body = outputStream -> this.bodyStream.writeTo(outputStream);
+		}
+		return executeInternal(headers, this.body);
+	}
+
+
+	/**
+	 * Abstract template method that writes the given headers and content to the HTTP request.
+	 * @param headers the HTTP headers
+	 * @param body the HTTP body, may be {@code null} if no body was {@linkplain #setBody(Body) set}
+	 * @return the response object for the executed request
+	 * @since 6.1
+	 */
+	protected abstract ClientHttpResponse executeInternal(HttpHeaders headers, @Nullable Body body) throws IOException;
+
+}

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2022 the original author or authors.
+ * Copyright 2002-2023 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.
@@ -21,6 +21,7 @@
 
 import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpMethod;
+import org.springframework.http.StreamingHttpOutputMessage;
 import org.springframework.util.StreamUtils;
 
 /**
@@ -52,7 +53,14 @@ public URI getURI() {
 	@Override
 	protected ClientHttpResponse executeInternal(HttpHeaders headers, byte[] bufferedOutput) throws IOException {
 		this.request.getHeaders().putAll(headers);
-		StreamUtils.copy(bufferedOutput, this.request.getBody());
+
+		if (this.request instanceof StreamingHttpOutputMessage streamingHttpOutputMessage) {
+			streamingHttpOutputMessage.setBody(outputStream -> StreamUtils.copy(bufferedOutput, outputStream));
+		}
+		else {
+			StreamUtils.copy(bufferedOutput, this.request.getBody());
+		}
+
 		ClientHttpResponse response = this.request.execute();
 		return new BufferingClientHttpResponseWrapper(response);
 	}

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2022 the original author or authors.
+ * Copyright 2002-2023 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.
@@ -17,21 +17,24 @@
 package org.springframework.http.client;
 
 import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.util.List;
+import java.util.Set;
 
 import org.apache.hc.client5.http.classic.HttpClient;
+import org.apache.hc.core5.function.Supplier;
 import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.ClassicHttpResponse;
-import org.apache.hc.core5.http.ContentType;
+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.io.entity.ByteArrayEntity;
 import org.apache.hc.core5.http.protocol.HttpContext;
 
 import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpMethod;
-import org.springframework.util.Assert;
+import org.springframework.lang.Nullable;
 import org.springframework.util.StringUtils;
 
 /**
@@ -46,7 +49,7 @@
  * @since 3.1
  * @see HttpComponentsClientHttpRequestFactory#createRequest(URI, HttpMethod)
  */
-final class HttpComponentsClientHttpRequest extends AbstractBufferingClientHttpRequest {
+final class HttpComponentsClientHttpRequest extends AbstractStreamingClientHttpRequest {
 
 	private final HttpClient httpClient;
 
@@ -81,19 +84,16 @@ HttpContext getHttpContext() {
 		return this.httpContext;
 	}
 
-
-	@SuppressWarnings("deprecation")  // execute(ClassicHttpRequest, HttpContext)
 	@Override
-	protected ClientHttpResponse executeInternal(HttpHeaders headers, byte[] bufferedOutput) throws IOException {
+	protected ClientHttpResponse executeInternal(HttpHeaders headers, @Nullable Body body) throws IOException {
 		addHeaders(this.httpRequest, headers);
 
-		ContentType contentType = ContentType.parse(headers.getFirst(HttpHeaders.CONTENT_TYPE));
-		HttpEntity requestEntity = new ByteArrayEntity(bufferedOutput, contentType);
-		this.httpRequest.setEntity(requestEntity);
-		HttpResponse httpResponse = this.httpClient.execute(this.httpRequest, this.httpContext);
-		Assert.isInstanceOf(ClassicHttpResponse.class, httpResponse,
-				"HttpResponse not an instance of ClassicHttpResponse");
-		return new HttpComponentsClientHttpResponse((ClassicHttpResponse) httpResponse);
+		if (body != null) {
+			HttpEntity requestEntity = new BodyEntity(headers, body);
+			this.httpRequest.setEntity(requestEntity);
+		}
+		ClassicHttpResponse httpResponse = this.httpClient.executeOpen(null, this.httpRequest, this.httpContext);
+		return new HttpComponentsClientHttpResponse(httpResponse);
 	}
 
 	/**
@@ -116,4 +116,78 @@ else if (!HttpHeaders.CONTENT_LENGTH.equalsIgnoreCase(headerName) &&
 		});
 	}
 
+
+	private static class BodyEntity implements HttpEntity {
+
+		private final HttpHeaders headers;
+
+		private final Body body;
+
+
+		public BodyEntity(HttpHeaders headers, Body body) {
+			this.headers = headers;
+			this.body = body;
+		}
+
+
+		@Override
+		public long getContentLength() {
+			return this.headers.getContentLength();
+		}
+
+		@Override
+		@Nullable
+		public String getContentType() {
+			return this.headers.getFirst(HttpHeaders.CONTENT_TYPE);
+		}
+
+		@Override
+		public InputStream getContent() {
+			throw new UnsupportedOperationException();
+		}
+
+		@Override
+		public void writeTo(OutputStream outStream) throws IOException {
+			this.body.writeTo(outStream);
+		}
+
+		@Override
+		public boolean isRepeatable() {
+			return false;
+		}
+
+		@Override
+		public boolean isStreaming() {
+			return true;
+		}
+
+		@Override
+		@Nullable
+		public Supplier<List<? extends Header>> getTrailers() {
+			return null;
+		}
+
+		@Override
+		@Nullable
+		public String getContentEncoding() {
+			return this.headers.getFirst(HttpHeaders.CONTENT_ENCODING);
+		}
+
+		@Override
+		public boolean isChunked() {
+			return false;
+		}
+
+		@Override
+		@Nullable
+		public Set<String> getTrailerNames() {
+			return null;
+		}
+
+		@Override
+		public void close() {
+		}
+
+	}
+
 }

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

@@ -65,8 +65,6 @@ public class HttpComponentsClientHttpRequestFactory implements ClientHttpRequest
 
 	private HttpClient httpClient;
 
-	private boolean bufferRequestBody = true;
-
 	@Nullable
 	private BiFunction<HttpMethod, URI, HttpContext> httpContextFactory;
 
@@ -146,9 +144,11 @@ public void setConnectionRequestTimeout(int connectionRequestTimeout) {
 	 * <p>Default is {@code true}. When sending large amounts of data via POST or PUT, it is
 	 * recommended to change this property to {@code false}, so as not to run out of memory.
 	 * @since 4.0
+	 * @deprecated since 6.1 requests are never buffered, as if this property is {@code false}
 	 */
+	@Deprecated(since = "6.1", forRemoval = true)
 	public void setBufferRequestBody(boolean bufferRequestBody) {
-		this.bufferRequestBody = bufferRequestBody;
+		// no-op
 	}
 
 	/**
@@ -190,13 +190,7 @@ public ClientHttpRequest createRequest(URI uri, HttpMethod httpMethod) throws IO
 				context.setAttribute(HttpClientContext.REQUEST_CONFIG, config);
 			}
 		}
-
-		if (this.bufferRequestBody) {
-			return new HttpComponentsClientHttpRequest(client, httpRequest, context);
-		}
-		else {
-			return new HttpComponentsStreamingClientHttpRequest(client, httpRequest, context);
-		}
+		return new HttpComponentsClientHttpRequest(client, httpRequest, context);
 	}