Disable streaming when reading to Resources in RestTemplate [SPR-14882]

[spring-projects-issues]

Hadrien Kohl opened SPR-14882 and commented

This issue has been updated to reflect the actual outcome of the discussion. See next section and comments for the original report.

The ResourceHttpMessageConverter supports converting from an HttpInputMessage to an InputStreamResource. This is valid when reading resources on the server side, but it's not compatible with the way RestTemplate works.

The API exposed by RestOperations imply that the HTTP server response should be fully consumed and properly closed by the time the exchange method returns. In other words, this HTTP client does not support streaming the HTTP response.

The current arrangement allows reading InputStreamResource with RestTemplate, which should not be possible.

Original report:

InputStreamResource are closed by SimpleClientHttpResponse

The http stream wrapped by InputStreamResource is closed by SimpleClientHttpResponse, rendering it unusable.

#1017 (comment)

I'll make a PR with a test.

---

Affects: 4.3.3

Reference URL: #18612

Issue Links:

• RestTemplate should support streaming download and upload [SPR-16885] #21424 RestTemplate should support streaming download and upload ("is duplicated by")
• HTTP persistent connections for HTTP Invoker and RestTemplate [SPR-14040] #18612 HTTP persistent connections for HTTP Invoker and RestTemplate
• ResourceHttpMessageConverter should read the Content-Disposition header (if available) to get the file name [SPR-15191] #19757 ResourceHttpMessageConverter should read the Content-Disposition header (if available) to get the file name

Referenced from: commits afd93a0

[spring-projects-issues]

Hadrien Kohl commented

Hi, I am having trouble running the spring project tests on my machine so I am copy pasting the code I came up with:

**
 * @author Arjen Poutsma
 * @author Rossen Stoyanchev
 */
@SuppressWarnings("unchecked")
public class RestTemplateTests {

	// [...]

	@Test // SPR-14882
	public void shouldLeaveInputStreamOpenIfResourceInputStream() throws IOException, URISyntaxException {
		ByteArrayInputStream stream = spy(
				new ByteArrayInputStream("Spring".getBytes(Charset.defaultCharset()))
		);

		InputStreamResource resource = new InputStreamResource(stream);
		given(requestFactory.createRequest(new URI("http://example.com/stream"), HttpMethod.GET))
				.willReturn(request);
		given(request.execute()).willReturn(response);
		given(converter.canRead(InputStreamResource.class, MediaType.TEXT_PLAIN)).willReturn(true);
		given(converter.read(eq(InputStreamResource.class), any(HttpInputMessage.class))).willReturn(resource);

		HttpHeaders responseHeaders = new HttpHeaders();
		responseHeaders.setContentType(MediaType.TEXT_PLAIN);
		responseHeaders.setContentLength(10);
		given(response.getStatusCode()).willReturn(HttpStatus.OK);
		given(response.getHeaders()).willReturn(responseHeaders);
		given(response.getBody()).willReturn(stream);
		resource = template.getForObject("http://example.com/stream", InputStreamResource.class);
		verify(stream, never()).close();
		resource.getInputStream().close();
		verify(stream).close();
	}
}

[spring-projects-issues]

Hadrien Kohl commented

I am wondering if I am pushing the client to its limit actually. I made tests setting up a SimpleClientHttpRequestFactory with setBufferRequestBody(false) and it seems to run correctly, meaning that the body stream is not closed. Still, allowing the RestTemplate to return InputStreamResource yet closing it was very hard to predict from the documentation.

[spring-projects-issues]

Brian Clozel commented

The goal of the change in SimpleClientHttpResponse (#18612) was to avoid closing the connection for every request/response exchange. To really leverage persistent connections, you need to avoid closing connections and to completely consume and close the response streams. This is done with that commit.

The problem with your use case is that you're implicitly using the ResourceHttpMessageConverter (which allows converting to InputStreamResource and ByteArrayResource), whereas RestTemplate is closing the response (in doExecute). So indeed, you're getting a reference to an InputStreamResource that has just been closed.

Now disabling buffering in the response with setBufferRequestBody(false) tells the client to use a SimpleStreamingClientHttpRequest instead of a SimpleBufferingClientHttpRequest. The streaming variant wraps the request body with StreamUtils.nonClosing(this.body) to make the close operation a no-op. So I'm not sure why it's linked to that case.

So in summary:

• when getting the full response body as a String, or deserialized Object, etc - it makes total sense to read the whole response, drain, and close. This allows to reuse the persistent connection for better performance.
• I don't see how this particular commit changed the deal since the previous version was closing the connection altogether.

Unless I'm missing something, both RestOperations and AsyncRestOperations implementations seem to close the response once data has been extracted; which means they're not tailored for streaming the response body and build full response entities.

The only improvement I could see is to prevent that use case with InputStreamResource; ByteArrayResource is still valid but duplicates a bit what the ByteArrayHttpMessageConverter does already.

What do you think?

[spring-projects-issues]

Jörn Horstmann commented

I'm also running into an issue with SimpleClientHttpResponse draining the InputStream. My usecase is handling of streaming json data, trying to drain this stream just blocks the thread until the other side of the connection decides to close the stream.

According to the last paragraph on http://docs.oracle.com/javase/8/docs/technotes/guides/net/http-keepalive.html, closing the InputStream should not be needed:

Now in JDK 6, the behavior is to read up to 512 Kbytes off the connection in a background thread, thus allowing the connection to be reused. The exact amount of data which may be read is configurable through the http.KeepAlive.remainingData system property.

There is a discussion about the implementation at http://stackoverflow.com/questions/14378908/java-httpurlconnection-inputstream-close-hangs-or-works-too-long which shows that it also only tries to consume immediately available data.

I have two open issues in my project about this problem, zalando-nakadi/fahrschein#83 and zalando-nakadi/fahrschein#17, the latter one about similar behaviour with the apache http components implementation.

To really solve the issue, there would need to be two close methods on ClientHttpResponse:

• graceful close, leaving it up to the implementation to consume remaining data. SimpleClientHttpResponse should let the above mentioned background thread handle this, HttpComponentsClientHttpResponse does this implicitly in its ChunkedInputStream implementation
• aborting the connection, using HttpURLConnection.disconnect for SimpleClientHttpResponse and ConnectionReleaseTrigger.abortConnection for the apache version.

[spring-projects-issues]

Brian Clozel commented

To complete my previous comment:

RestTemplate, like any other Spring template (JDBC, etc), is meant to close the underlying resources when it provides you with the return value. In this case, the response is closed and you can't expect to work with the live response stream outside provided by RestTemplate.

Of course, you can write your own HttpMessageConverter or ResponseExtractor implementation to work with the live response within a RestTemplate call.

Jörn Horstmann,
According to the JDK documentation you're sharing here, the JDK implementation is "automatically draining" the response body by reading up to 512Kbytes of data. We're trying to achieve the same thing here: reading the response body before closing it so the connection can be recycled.

I understand the rationale behind your proposal but that would go against the RestTemplate contract which does not allow the streaming of the response outside of the template methods.

Note that we're working on a new WebClient in the reactive module (scheduled for Spring 5.0, already available in the milestone and snapshot builds); this new client is perfectly tailored for those streaming use cases and fully support reactive streams.

[spring-projects-issues]

Jörn Horstmann commented

I understand the rationale behind your proposal but that would go against the RestTemplate contract which does not allow the streaming of the response outside of the template methods.

Note that I'm not using RestTemplate, but the ClientHttpRequestFactory directly, just as an abstraction for the underlying http library. This abstraction works and fits the streaming usecase very nicely. I will probably end up forking the "Simple" and "HttpComponents" implementations to change the close implementation, and bundle these with my library.

[spring-projects-issues]

vivek sanhai commented

if this fix will disable Streaming in Rest Template while reading large files what are my options to call a Rest API that returns Large File and if I need to use Spring 4.x in my project?

 

[spring-projects-issues]

Brian Clozel commented

vivek sanhai,

This fix has been applied to Spring Framework 5.0. If you're using a 4.x version, this does not apply to you.

When you'll switch to 5.x, using WebClient will be the best option.