HTTP persistent connections for HTTP Invoker and RestTemplate [SPR-14040]

[spring-projects-issues]

Benjamin Bargeton opened SPR-14040 and commented

Normally HttpURLConnection is maintaining persistent connections under the hood.

One condition to reuse a connection is that the http response must be fully consumed (https://docs.oracle.com/javase/1.5.0/docs/guide/net/http-keepalive.html)

The combination of using an ObjectOutputStream over an Http response output stream as done in HttpInvokerServiceExporter make this condition hard to be satisfied.

The oos.close() method called in writeRemoteInvocationResult(...) is doing the following things (i've inlined a bit the code):

// bout = underlying BlockDataOutputStream
// out = response.getOutputStream() inside bout
public void close() throws IOException {
    bout.flush(); // will call bout.drain() and out.flush()
    clear();
    bout.close(); // will call bout.flush() again (so bout.drain() and out.flush()) and out.close()
}

Forcing the flush on the servlet reponse output stream before closing it has 2 side effects:

1. It will prevent the servlet container to have a chance to use the Content-Length header if the response size is inferior to response.getBufferSize() and therefore force a chunked transfer encoding for every response
2. I guess this is could differ depending on the container but at least on Tomcat and Websphere, it will send the chunked encoding trailer (0\r\n\r\n) in an additional tcp packet (see attached Wireshark printscreen). This is the main issue since most of the time if the server that runs the HttpInvoker client is fast enough, it will not have received that packet before closing the input stream of the HttpUrlConnection (ObjectInputStream.readObject() is returning as soon as the object is read), and so will prevent the keep alive of the tcp connection (see sun.net.www.http.ChunkedInputStream.hurry() for the method that clean the chunked trailer from the connection).

They are many way of fixing this issue.
The one I've picked for the moment is to enclose the response stream in a wrapper that do nothing on flush (using decorateOutputStream()) but I think this should be the default in HttpInvokerServiceExporter because this can lead to serious performance issue under load

Note: I believe this is the same issue as the one described in #12645

---

Affects: 4.2.5

Attachments:

• wireshark.png (97.57 kB)

Issue Links:

• SimpleClientHttpResponse disconnects the underlying HttpURLConnection when closing [SPR-8123] #12775 SimpleClientHttpResponse disconnects the underlying HttpURLConnection when closing ("is duplicated by")
• No support of persistent HTTP connections within org.springframework.http.client.SimpleClientHttpRequestFactory (v3.0.4) [SPR-7990] #12645 No support of persistent HTTP connections within org.springframework.http.client.SimpleClientHttpRequestFactory (v3.0.4)
• NPE in SimpleClient-HttpURLConnection with errorstream-buffering [SPR-16773] #21313 NPE in SimpleClient-HttpURLConnection with errorstream-buffering
• AsyncRestTemplate should trigger no-output HTTP requests immediately as well [SPR-14093] #18665 AsyncRestTemplate should trigger no-output HTTP requests immediately as well
• Disable streaming when reading to Resources in RestTemplate [SPR-14882] #19448 Disable streaming when reading to Resources in RestTemplate
• 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

Referenced from: commits b947bfe, eec22f5

[spring-projects-issues]

Benjamin Bargeton commented

When I searched for any similar ticket prior to opening that case I've felt on #12775
Since I had to understand how HttpUrlConnection is working for #18612, the resolution of the #12775 case was bothering me as well.

After double checking I think that calling connection.disconnect() in SimpleClientHttpResponse is wrong and that the original reporter of the ticket was right.

Calling disconnect for every RestTemplate call, will not only prevent the http persistent connections but it can even lead to closing two connections at once as stated in the source comments of HttpUrlConnection (see below).

Calling close on the inputstream as described in the original ticket is better because it already guarantees that the connection will be closed at some point:

• If it's not a keep alive connection it will be close immediately
• If's it's a persistent connection, there is a thread in sun.net.www.http.KeepAliveCache that will close idle connections after they reach their timeout value (5 sec by default of the value of the "KeepAlive: timeout=xxx" header if present).

If you agree, could you reopen that ticket, or do you want me to create another one?
Or maybe I can just rename this one with a more generic subject (Like Improving HTTP persistent connections)?

/*
         * If we have an input stream this means we received a response
         * from the server. That stream may have been read to EOF and
         * dependening on the stream type may already be closed or the
         * the http client may be returned to the keep-alive cache.
         * If the http client has been returned to the keep-alive cache
         * it may be closed (idle timeout) or may be allocated to
         * another request.
         *
         * In other to avoid timing issues we close the input stream
         * which will either close the underlying connection or return
         * the client to the cache. If there's a possibility that the
         * client has been returned to the cache (ie: stream is a keep
         * alive stream or a chunked input stream) then we remove an
         * idle connection to the server. Note that this approach
         * can be considered an approximation in that we may close a
         * different idle connection to that used by the request.
         * Additionally it's possible that we close two connections
         * - the first becuase it wasn't an EOF (and couldn't be
         * hurried) - the second, another idle connection to the
         * same server. The is okay because "disconnect" is an
         * indication that the application doesn't intend to access
         * this http server for a while.
         */

http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/sun/net/www/protocol/http/HttpURLConnection.java
http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/sun/net/www/http/KeepAliveCache.java

[spring-projects-issues]

Juergen Hoeller commented

Let's repurpose this one for a broader context...

[spring-projects-issues]

Brian Clozel commented

Hi Benjamin Bargeton

I've made progress on the SimpleClientHttpResponse and I'm testing its behavior.

Now I've got a few questions regarding HttpInvokerServiceExporter and ObjectOutputStream.
First, let me know if I fully understand the problem:

1. when handling an exchange in SimpleHttpInvokerServiceExporter, we read an invocation from the HTTP request and write the result to the HTTP response
2. when writing that result, we close() the ObjectOutputStream, which drains the bytes from the buffer to the outputstream and flushes the outputstream
3. flushing the HTTP response stream adds chunked encoding trailer chars which are not read by the client
4. since the client doesn't read the whole response inputstream, the connection can't be reused

When looking at the proposed solution (i.e. not flushing the response outputstream on the server side), it seems that we're losing the guarantee of our data being actually written to the socket if they're buffered by the outputstream implementation. In all cases, the outputstream will be closed (see SimpleHttpInvokerServiceExporter.handle where the exchange is closed). Not flushing it before closing it can be problematic.

Here are my questions:

• could you explain a bit more how/why this problem should be solved on the writeRemoteInvocationResult side, while still draining all the data from the ObjectOutputStream and making sure that everything is written to the HTTP response outputstream?
• from where I see it, the problem may actually be on the client side, not properly reading the whole response and not freeing the resources properly so the connection can't be reused. Shouldn't we look at the SimpleHttpInvokerRequestExecutor for a fix? For example, draining the whole inputstream before we close it in AbstractHttpInvokerRequestExecutor.readRemoteInvocationResult?

Thanks for this (very) detailed report, this gives us the chance to revisit things we previously missed.

[spring-projects-issues]

Benjamin Bargeton commented

Hi Brian,

For step 3 I don't know if it's what you wanted to say but the flush doesn't add the chunked encoding trailer in itself.
It just triggers the send of the response in one or more tcp packets (if the servlet container not already decided to send some data at that point).
Since there is nothing more to add to the response at closing time, the close will then complete the chunked transfer by sending the expected trailer, but always in a separate tcp packet.
If there was no flush both (the last data chunk and the trailer) could have been sent in the same tcp packet (if it fits in the packet size).
The probability that some of the last five characters are sent in a separate packet is therefore very small.

About the close in SimpleHttpInvokerServiceExporter.handle, I didn't test with that one (I'm using the HttpInvokerServiceExporter version) but it should be the same (see below).

To try to answer your questions:

• Maybe I'm wrong but to me close() should always guarantee that previously written data are not lost.
  flush() just allow to send it sooner.
  I've implemented it that way:

    @Override
    protected OutputStream decorateOutputStream(HttpServletRequest request, HttpServletResponse response, OutputStream os) throws IOException {
        return new FlushGuardedOutputStream(super.decorateOutputStream(request, response, os));
    }

    public class FlushGuardedOutputStream extends FilterOutputStream {
        public FlushGuardedOutputStream(OutputStream out) {
            super(out);
        }

        @Override
        public void flush() throws IOException {
            // Do nothing
        }
    }

So basically every time flush is call on the ObjectOutputStream it will still drain the data to the underlying stream but prevent the on flush it, so I believe that no data should ever be lost.

• It was my initial idea too, but I think this is done on purpose (I mean in the jdk HttpUrlConnection implementation).
  I guess the idea is that we don't want to wait on the socket and block the client side if we never receive the trailer.
  If you look at ChunkedInputStream.hurry(), it will do a non-blocking read on the socket with that explanation:

/**
 * Hurry the input stream by reading everything from the underlying
 * stream. If the last chunk (and optional trailers) can be read without
 * blocking then the stream is considered hurried.
 * <p>
 * Note that if an error has occurred or we can't get to last chunk
 * without blocking then this stream can't be hurried and should be
 * closed.
 */

Also from that comment it seems that extra headers could be sent after after the last data chunk and before the closing trailer (never had the case though).
See "Trailers in chunked messages" chapter in this book sample
So I think that finding a good spot between how long to block on the socket to try to reuse the connection vs closing it is a hard guess.

Anyway on my tests, fixing it on the server side was good enough (I didn't see any close due to a not received trailer) and we didn't had a case of lost data since it was delivered.
I agree that it could feel a bit hacky, but I really struggle to understand why the ObjectOutputStream.close() is forcing the flush on the underlying stream twice.
I should have some wireshark capture from our network team next week, I can update the issue with the result of that modification if you are interested.

Thanks for your last comment, I've learn so much by reading/using Spring projects that it's a pleasure to bring at last a contribution (even if it's a very small one:-)).

[spring-projects-issues]

Brian Clozel commented

Thanks Benjamin Bargeton!

I was indeed mixing both issues which are quite different but in the end have a strong impact on HTTP performance.
I've just pushed a couple of commits to master (see eec22f507 and b947bfe8e). Those should be available shortly in the 4.3.0.BUILD-SNAPSHOT build (when SPR-PUB-3390 is finished).

Again, thanks for this very detailed report!

[spring-projects-issues]

Rossen Stoyanchev commented

Should the resolution of #12775 be updated as duplicate or superseded by the fix for this ticket?