SimpleClientHttpResponse disconnects the underlying HttpURLConnection when closing [SPR-8123]

[spring-projects-issues]

Juan Antonio Farré Basurte opened SPR-8123 and commented

I've noticed that SimpleClientHttpResponse.close() (which it is documented you MUST always call it after completing the request) invokes HttpURLConnection.disconnect() for the wrapped connection.
JDK javadoc for HttpURLConnection states the following:

Calling the close() methods on the InputStream or OutputStream of an HttpURLConnection after a request may free network resources associated with this instance but has no effect on any shared persistent connection. Calling the disconnect() method may close the underlying socket if a persistent connection is otherwise idle at that time.

So, when you close SimpleClientHttpResponse you may be disconnecting the underlying socket for a connection that could otherwise be reused (not the HttpURLConnection instance itself, but the underlying socket connection).
I believe close should close any open InputStreams (if either HttpURLConnection.getErrorStream() or URLConnection.getInputStream() were invoked), but not invoke the disconnect method.
If such functionality is required to be available for users of SimpleHttpUrlConnection, then I believe a new method should be added (for example disconnect()) that is not required to be called.

Thanks,

Juan

---

Affects: 3.0.5

Issue Links:

• HTTP persistent connections for HTTP Invoker and RestTemplate [SPR-14040] #18612 HTTP persistent connections for HTTP Invoker and RestTemplate ("duplicates")
• SimpleClientHttpResponse.close() does not call close() on URLConnection.getInputStream() if SimpleClientHttpResponse.getBody() was not called before [SPR-17181] #21717 SimpleClientHttpResponse.close() does not call close() on URLConnection.getInputStream() if SimpleClientHttpResponse.getBody() was not called before

[spring-projects-issues]

Chris Beams commented

Arjen - you implemented the code in question. Can you comment?

[spring-projects-issues]

Arjen Poutsma commented

While I do agree that closing the underlying connection for every RestTemplate call is a bit overaggressive, I don't see a good alternative. Closing the body stream, as suggested, is simply not good enough, as I believe this will keep the underlying socket open indefinitely.

That said, it's rather hard to find good documentation about the way HttpUrlConnection actually works. The javadoc only contains hollow phrases with 'may close' and 'may disconnect', which is completely useless IMO.

Also, we should consider the fact that Google App Engine has their own implementation of HttpURLConnection, and that there is no guarantee that GAE deals with reusing sockets the same 'intelligent' way the SUN JDK does. By calling disconnect(), we at least know for sure that everything is properly cleaned up.

[spring-projects-issues]

Juan Antonio Farré Basurte commented

I understand your point and the fact that calling disconnect in the close method is a save way of avoiding connection leaks.
But I'd like to comment a couple more details:

HttpURLConnection javadoc starts referring to HTTP spec for details. So, when it talks about persistent connections, it's referring to the definition in HTTP spec and its explanation of the standard-compliant behaviour regarding to them (using persistent connections is, in fact, the default for HTTP 1.1 and hence, calling disconnect for every request, wouldn't be standard-compliant strictly speaking).

This also implies that an idle persistent connection can always be explicitly closed in two possible ways:

• Calling disconnect, which, following the javadoc, "Indicates that other requests to the server are unlikely in the near future", and could be done through a specialized method of SimpleHttpUrlConnection.
• Just sending "Connection: close" header in any request or response, as HTTP standard specifies.

There are more details about persistent connections, their importance and their implementation in java.net doc, which you probably have already read:

http://download.oracle.com/javase/6/docs/technotes/guides/net/http-keepalive.html

It also gives interesting guidelines for applications using this API.
One of the many interesting points in that documentation is that you can control keep-alive behavior via system properties http.keepAlive and http.maxConnections. This gives users of HttpURLConnection (or SimpleHttpUrlConnection) the hability to change default behavior if they wish. Having SimpleHttpUrlConnection.close() call disconnect() gives no chance to change default behavior, unless you do the bad practice of not following the specs strictly and just not calling close().

As a last point, if google implementation does not manage connections "well enough" it must mean that they fail to follow HTTP and/or HttpURLConnection specs. This would be a bug and I don't think spring API implementation should be conditioned by a buggy underlaying implementation (at most, im my oppinion, it could include a configurable workaround).

For my concrete needs, having close() call disconnect() makes SimpleHttpUrlConnection completely unusable, as I could never reuse connections, and then I must use apache-commons implementation.

Of course, you have the last word about this, and for sure you know better the needs (and politics) of spring, so I accept that your final decission be "won't fix".

Thanks a lot,

Juan

PS: By the way, is there intention of implementing a new wrapper for the new apache HTTP-client implementation?

[spring-projects-issues]

Arjen Poutsma commented

@Juan

I am aware of HTTP 1.1 persistent connections, and the settings HttpURLConnection has to enable it. What I am struggling with, however, is to ensure that the TCP connection is properly closed in the end. As far as I can tell, the only way to ensure that is to use disconnect(), so that's what I do.

Apache commons and the new HTTP client (that will be in 3.1 M2) solve this by having a separate connection factory, which I can close as the application context is closed. I could write such a connection factory object for the HttpURLConnection, but since we already have superior alternatives in the form of the Apache clients, I don't see the point.

Regarding the Google App Engine: reports about key differences between the GAE and JDK HttpURLConnection have already been made. For instance, see #11805, which could definitely be labeled as a GAE bug, as it doesn't follow the HttpURLConnection javadoc. Doing so wouldn't help Spring users at all, however, so I wrote a workaround.

Thanks for the feedback, and do let me know if you know of a way to solve the issues I've described above.

[spring-projects-issues]

Juan Antonio Farré Basurte commented

Thanks, now I understand the problem much better :)
The only idea that comes to my mind that could ensure that TCP connections are properly closed in the end and looks straightforward enough to implement is to keep WeakReferences to all SimpleHttpUrlConnection instances and have a thread invoke disconnect for the underlaying HttpURLConnections when these references have been cleared by the garbage collector.
But you're right that this might not be worth the effort having Apache alternatives.
Thanks again for your time and interest.

[spring-projects-issues]

Brian Clozel commented

This has been resolved as part of #18612