SimpleClientHttpResponse.close() does not call close() on URLConnection.getInputStream() if SimpleClientHttpResponse.getBody() was not called before [SPR-17181]

[spring-issuemaster]

Jonas Woerlein opened SPR-17181 and commented

We've noticed SimpleClientHttpResponse.close() does not call close() on URLConnection.getInputStream() if SimpleClientHttpResponse.getBody() was not called before as the responseStream field is lazily initalized within the latter method.

JDK8 documentation states the InputStream returned by URLConnection.getInputStream() has to be read till the end or explicitly close()d to allow the JDK's protocol handler to clean up the connection and put it into a connection cache for reuse by future HTTP requests.

By debugging, we found the KeepAliveStream being returned by URLConnection.getInputStream() to be created not on calling this method but on parsing the HTTP response.

Caused by this issue, we experience HTTP connections created or reused by a PUT request not being reused for any subsequent requests. This leads to higher network load and increased latency due to additional TLS handshakes.

The requests were emitted with RestTemplate

restTemplate.put("https://www.example.com/resource", body)

The server responds with something like

HTTP/1.1 200 OK

Connection: Keep-Alive

Content-Length: 61

Content-Type: application/json;charset=utf-8

Date: Tue, 14 Aug 2018 10:37:08 GMT

Keep-Alive: timeout=5, max=100

Server: Apache


<REDACTED-JSON-RESPONSE>

So there is a response body, but as it contains no interesting information, taking the status into consideration is sufficient for us.

The current workaround is to read the body into a String but ignore it, like so:

RequestEntity<RequestType> requestEntity = RequestEntity            .put(UriComponentsBuilder.fromHttpUrl("https://www.example.com/resource")).build().toUri()).body(body);

restTemplate.exchange(requestEntity, String.class);

This way, the HttpMessageConverterExtractor calls SimpleClientHttpResponse.getBody() and thus initializes the responseStream field.  When the RestTemplate then close()s the SimpleClientHttpResponse, the responseStream is also closed.

As we believe creating a String object which is never read is bad practice, we would appreciate the Spring Team to consider changing the behavior in SimpleClientHttpResponse. Calling getBody() in close() if responseStream is null might be the solution to this issue.

We encountered this issue with version 4.3.12, but it also exists in 5.0.7 so I marked this version as affected. The issue might be related to #12775.

---

Affects: 4.3.18, 5.0.7

Issue Links:

• #12775 SimpleClientHttpResponse disconnects the underlying HttpURLConnection when closing
• #18612 HTTP persistent connections for HTTP Invoker and RestTemplate
• #12015 Provide a way to read an InputStream with RestTemplate

Referenced from: commits 23fc6f6

[spring-issuemaster]

Brian Clozel commented

Hello Jonas Woerlein,

Indeed, this is something we looked at in #18612 and we're always happy to improve the situation there.

By reading the response body into a String even if you don't need it, you're making sure that the response is drained, making it possible to return that HTTP connection to the pool and reuse it later. This commit is making sure of that, in case the response could not be fully read.

Now in your case, I see that we're missing something - but I'm not sure the fix you're suggesting is the best choice.

When closing the response if its body has not been read, we could get the response stream anyway and close it. In the "What can you do to help with Keep-Alive" section of the JDK, they rightfully state:

Reading the response body cleans up the connection even if you are not interested in the response content itself. But if the response body is long and you are not interested in the rest of it after seeing the beginning, you can close the InputStream. But you need to be aware that more data could be on its way. Thus the connection may not be cleared for reuse.

In your case, this means that the connection could be closed and not placed back in the pool, leading to the same issue. It's not deterministic as it depends on the response size and buffering.

Or, when closing the response if its body has not been read, we could get the response stream anyway, drain it, and then close it. In this case, this would solve your particular problem but could cause a bigger issue for others. If the response body is large/takes a lot of time to read, then we're forcing everybody to pay the cost of reading the whole response body, even if in their case closing the connection and opening a new one was cheaper.

Because RestTemplate is not meant for response streaming and is mostly meant to buffer the response in memory, I'm thinking about draining the response body in all cases. But this is a behavior change that's important enough to reconsider applying it in maintenance branches.

Please let me know if you've got an opinion about this.

[spring-issuemaster]

Brian Clozel commented

I've applied this change to 5.1.RC2 for now, as this behavior change might be problematic for some cases. Let's see if we get feedback from the community on that.

[spring-issuemaster]

Jonas Woerlein commented

Hello Brian Clozel,

thanks for your immediate reply and code implementation. It's always a pleasure to discuss with the Spring Team!

We're absolutely fine with your approach. We also understand the behaviour change is too big to put into the maintenance branches. Until we can use Spring 5.1, we will implement the workaround described in the ticket description. As the response we're receiving is rather small, it shouldn't be a problem to store it in memory for a short amount of time.