Apache connector: Removed unnecessary InputStream.close() call that causes exception on httpclient 4.5.2

[agherardi]

I believe this should fix https://java.net/jira/browse/JERSEY-3214

[agherardi]

The only FilterInputStream method that this class overrides is close(). However the override close() method calls the superclass' close() method, so it's unnecessary.

[agherardi]

See httpclient documentation. To properly dispose of resources, one must either close the CloseableHttpResponse or the InputStream. Closing both is unnecessary and causes an exception in httpclient 4.5.2.

[aaronjwhiteside]

Calling response.close() causes no connections to be released back to the pool.

https://java.net/jira/browse/JERSEY-3260

The return new FilterInputStream(inputStream) { is redundant and inputStream can just be returned instead.

[agherardi]

If the application consumes the entity, by the time response.close() runs the connection has already released to the pool, so response.close() does nothing.

If the application does NOT consume the entity, there is no other choice but closing the connection - reusing it is not possible in that case.

[aaronjwhiteside]

@agherardi I agree with you. However response.close() will always cause the connection to be closed regardless of whether or not the entity was read in its entirety. Follow the chain of calls that response.close() makes and you will see it eventually calls ConnectionHolder.releaseConnection(false) - the false bit means the connection should not be reused (returned to the pool).

So right now, connection pooling with the Apache Connector is broken.

HttpClientResponseInputStream.close() -> CloseableHttpResponse.close() -> HttpResponseProxy.close() -> ConnectionHolder.close() -> ConnectionHolder.releaseConnection(false)

There needs to be some conditional check to see if the entity has not been consumed yet and then call response.close() otherwise call super.close().

Another alternative could be to have an option to consume/read any remaining bytes of the entity and discard them, I'm not sure this should be the default behavior but it seems very useful.

Perhaps something along these lines:

    return new FilterInputStream(inputStream) {
        private boolean consumed;

        @Override
        public int read() throws IOException {
            int b = super.read();
            if (b == -1) {
                consumed = true;
            }
            return b;
        }

        @Override
        public void close() throws IOException {
            if (consumed) {
                super.close();
            } else {
                response.close();
            }
        }
    };

[agherardi]

ConnectionHolder.releaseConnection(false) - the false bit means the connection should not be reused (returned to the pool).

True. But if you check ConnectionHolder.releaseConnection(final boolean reusable), notice that it enters the block that aborts the connection only if:

if (this.released.compareAndSet(false, true)) {

When I test this code under debugger, if the application has already consumed the entity, this.released is already set to TRUE so the block of code that aborts the connection doesn't run.

[agherardi]

Another way to verify that my patch does NOT break connection reuse is running the code below - with my patch - while wiresharking traffic. Assuming the server honors Connection: Keep-alive, you'll see that both GET requests use the same TCP/HTTP connection.

public static void main(String[] args) throws Exception
{
  Client client = getClient();
   doJersey(client);
   doJersey(client);
 }

 public static void doJersey(Client client) throws Exception
 {
   String url = "http://...";

   WebTarget resource = client.target(url);
   Response response = resource
    .request()
    .get();

   response.readEntity(String.class);
   response.close();
 }

[aaronjwhiteside]

Interesting, my use case uses WebResourceFactory for proxy generation, where the interface method returns a Model (so the assumption is that if the model is returned, the entity must be consumed), maybe the bug has something to do with the code in the proxy.

Sorry I should have said before I don't think this PR introduces the problem, as it exists for me on 2.25.1

I haven't looked at the ConnectionHolder while debugging yet, but off the top of my head
if (this.released.compareAndSet(false, true)) only stops the connection from being released twice. Not prematurely as in my case.

The real problem is that calling close() on the ConnectionHolder will cause the connection to always be closed and never released.

    public void close() throws IOException {
        releaseConnection(false);
    }

    private void releaseConnection(final boolean reusable) {
        if (this.released.compareAndSet(false, true)) {
            synchronized (this.managedConn) {
                if (reusable) {
                    this.manager.releaseConnection(this.managedConn,
                            this.state, this.validDuration, this.tunit);
                } else {
                    try {
                        this.managedConn.close();

Let me come up with a minimal reproducible test case. My current codebase is much too large to post examples here.

And thank you for your time in reading and responding to my comments.

[aaronjwhiteside]

So I've narrowed the issue down to the type of entity you request when calling .get()

This causes connections to be closed:

        target.path("/test").request().get(Model.class);
        target.path("/test").request().get(Model.class);

This causes connections to be released back to the pool:

        target.path("/test").request().get(String.class);
        target.path("/test").request().get(String.class);

Thanks again for your time, I think I'll go bother someone else now :)

[MarkusKull]

Oh. There is already #267 which also tries to address this problem.

Difference between the two Pull Requests: #268 always aborts the connection (when not already consumed). #267 aborts the connection when closing the response, but consumes when closing the inputstream (identical behaviour to apache httpclient).

[agherardi]

If the application closes the inputstream, most likely the application is not interested in the response content. In that case, aborting the connection is the best course of action - consuming the response blocks the application until it receives the entire content.

To summarize:

• If the application is interested in the response content, the application can consume the entity then close the response. Barring other issues, the connection is reusable and released to the pool.
• If the application is not interested in the response context, the application can immediately close the response. In this case, the connection is aborted and cannot be reused. That's better than blocking the application in an attempt to reuse the connection.

That's why I believe this pull request provides the best solution to the exception-at-close issue.

[agherardi]

OCA is here http://www.oracle.com/technetwork/community/oca-486395.html

[agherardi]

Re-opened against master as https://github.com/jersey/jersey/pull/3565 .