Skip to content
This repository was archived by the owner on May 28, 2018. It is now read-only.

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39438d8
Got rid of unnecessary class HttpClientResponseInputStream. Removed c…
Mar 18, 2017
cbe978c
Merge remote-tracking branch 'upstream/2.x' into 2.x
Apr 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 1 addition & 14 deletions ...apache-connector/src/main/java/org/glassfish/jersey/apache/connector/ApacheConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ public ClientResponse apply(final ClientRequest clientRequest) throws Processing
}

try {
responseContext.setEntityStream(new HttpClientResponseInputStream(getInputStream(response)));
responseContext.setEntityStream(getInputStream(response));
} catch (final IOException e) {
LOGGER.log(Level.SEVERE, null, e);
}
Expand Down Expand Up @@ -616,18 +616,6 @@ private static Map<String, String> writeOutBoundHeaders(final MultivaluedMap<Str
return stringHeaders;
}

private static final class HttpClientResponseInputStream extends FilterInputStream {

HttpClientResponseInputStream(final InputStream inputStream) throws IOException {
super(inputStream);
}

@Override
public void close() throws IOException {
super.close();
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private static InputStream getInputStream(final CloseableHttpResponse response) throws IOException {

final InputStream inputStream;
Expand All @@ -647,7 +635,6 @@ private static InputStream getInputStream(final CloseableHttpResponse response)
@Override
public void close() throws IOException {
response.close();
super.close();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@aaronjwhiteside aaronjwhiteside Apr 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@aaronjwhiteside aaronjwhiteside Apr 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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();
            }
        }
    };

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
 }

Copy link

@aaronjwhiteside aaronjwhiteside Apr 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@aaronjwhiteside aaronjwhiteside Apr 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

};
}
Expand Down
17 changes: 17 additions & 0 deletions ...s/apache-connector/src/test/java/org/glassfish/jersey/apache/connector/StreamingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import javax.ws.rs.client.WebTarget;
import javax.ws.rs.core.Application;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;

import javax.inject.Singleton;

Expand Down Expand Up @@ -85,6 +86,15 @@ public void clientCloseTest() throws IOException {
assertEquals("NOK", sendTarget.request().get().readEntity(String.class));
}

/**
* Tests that closing a request without reading the entity does not throw an exception.
*/
@Test
public void clientCloseThrowsNoExceptionTest() throws IOException {
Response response = target().path("/streamingEndpoint/get").request().get();
response.close();
}

@Override
protected void configureClient(ClientConfig config) {
config.connectorProvider(new ApacheConnectorProvider());
Expand Down Expand Up @@ -118,5 +128,12 @@ public String sendEvent() {
public ChunkedOutput<String> get() {
return output;
}

@GET
@Path("get")
@Produces(MediaType.TEXT_PLAIN)
public String getString() {
return "OK";
}
}
}