Apache Connector connection pooling is not supported properly

[jerseyrobot]

This issue has the same result as: https://java.net/jira/browse/JERSEY-2157 but the source cause is different.

In Jersey Client's JerseyInvocation.java (https://github.com/jersey/jersey/blob/927b4d0605aeb119d15e9623dab65c8aec2c8e20/core-client/src/main/java/org/glassfish/jersey/client/JerseyInvocation.java#L943-L953), if the callbackParamClass is of type Response.class, the resulting logic fails to consume or read the entity. It simply casts the response as an InboundJaxrsResponse.

This causes connection-pool starvation when using certain connectors (such as the ApacheConnector), which rely on the entity being "consumed" to release the connection back into the pool, and hitting JERSEY-based servers that return Response objects instead of actual entities.

I included the other "bug" to show how it was resolved: https://github.com/jersey/jersey/blob/927b4d0605aeb119d15e9623dab65c8aec2c8e20/core-client/src/main/java/org/glassfish/jersey/client/JerseyInvocation.java#L995-L999

In essence, on a non-200 response, response.bufferEntity() is called to notify connectors, such as the ApacheConnector, that the entity has been "consumed" and that it should release the current connection back into the Apache ConnectionManager pool (if being used).

I think a similar code-fix needs to be implemented for a successful 200 response that returns a Response object.

I have resorted to calling response.bufferEntity() in all my callbacks and response-handling code to prevent my ApacheConnector pool from suffering from connection starvation because connections are never released.

This issue doesn't affect JERSEY Server REST endpoints that return anything other than Response (unfortunately the JERSEY Server returns Response objects for all "async" server REST handlers...which in our case is quite a few).

Environment

Mac OSX 10.11.4; DropWizard 0.9.2;

Affected Versions

[2.23.1]

[jerseyrobot]

• Issue Imported From: https://github.com/jersey/jersey/issues/3421
• Original Issue Raised By:@glassfishrobot
• Original Issue Assigned To: @mpotociar

[jerseyrobot]

@glassfishrobot Commented Reported by nkoterba

[jerseyrobot]

@glassfishrobot Commented @pavelbucek said: Response has #close() method - have you tried calling that instead of #bufferEntity()?

BTW, the javadoc explicitly mentions that #close() should be called whenever you are getting a Response instance:

* The {@code close()} method should be invoked on all instances that
     * contain an un-consumed entity input stream to ensure the resources associated
     * with the instance are properly cleaned-up and prevent potential memory leaks.
     * This is typical for client-side scenarios where application layer code
     * processes only the response headers and ignores the response entity.

[jerseyrobot]

@glassfishrobot Commented nkoterba said: I agree that calling

bufferEntity

is not ideal. If the entity is being "streamed" this causes the entire entity to be fetched which may defeat the whole purpose of streaming.

However, calling

close

closes the underlying connection which completely defeats the purpose of using Apache HttpClient's PoolingHttpClientConnectionManager.

When I close the connection, this is the log:

DEBUG [2016-08-03 13:54:45,053] org.apache.http.impl.conn.DefaultManagedHttpClientConnection: http-outgoing-15: Close connection
DEBUG [2016-08-03 13:54:45,053] org.apache.http.impl.execchain.MainClientExec: Connection discarded
DEBUG [2016-08-03 13:54:45,053] org.apache.http.impl.conn.PoolingHttpClientConnectionManager: Connection released: [id: 15][route: {s}->https://localhost:8443][total kept alive: 2; route allocated: 2 of 100; total allocated: 2 of 50] DEBUG [2016-08-03 13:54:45,053] org.apache.http.wire: http-outgoing-15 << "[read] I/O error: Socket is closed"

When I call

bufferEntity

, here is the log:

DEBUG [2016-08-03 13:59:30,928] org.apache.http.impl.execchain.MainClientExec: Connection can be kept alive indefinitely
DEBUG [2016-08-03 13:59:30,929] org.apache.http.client.protocol.ResponseProcessCookies: Cookie accepted [rememberMe="deleteMe", version:0, domain:localhost, path:/xxx, expiry:Tue Aug 02 09:59:30 EDT 2016]
DEBUG [2016-08-03 13:59:30,929] org.apache.http.impl.conn.PoolingHttpClientConnectionManager: Connection [id: 4][route: {s}->https://localhost:8443] can be kept alive indefinitely DEBUG [2016-08-03 13:59:30,929] org.apache.http.impl.conn.PoolingHttpClientConnectionManager: Connection released: [id: 4][route: {s}->https://localhost:8443][total kept alive: 5; route allocated: 5 of 100; total allocated: 5 of 50]

It seems to me that calling

close

forces the underlying socket connection to close, whereas calling

bufferEntity

just releases the connection/socket back to the Apache ConnectionManager pool (performance gain).

I also don't like that print statement "[read] I/O error: Socket is closed".

If you trace the code in ConnectionHolder.java: https://github.com/apache/httpclient/blob/52c68d6a4f1a14e88f9817270580bb384138040d/httpclient5/src/main/java/org/apache/hc/client5/http/impl/sync/ConnectionHolder.java#L153-L156

You can see that when

close

is called it calls

releaseConnection(false)

. The false parameter is used in the relaseConnection method to determine whether to re-use or permanently close the connection. See: https://github.com/apache/httpclient/blob/52c68d6a4f1a14e88f9817270580bb384138040d/httpclient5/src/main/java/org/apache/hc/client5/http/impl/sync/ConnectionHolder.java#L94-L115

The ultimate crux of the issue is that: 1) Apache HTTPClient wraps entities in a proxy and uses the End-Of-Stream event on that entity to know when to "release" the used connection back to the pool. 2) When the JerseyInvocation sees a Response object it just casts it (instead of reading it) and End-Of-Stream is never called. 3) Response object has

close

and

bufferEntity

methods. The latter "triggers" the Apache HTTPClient to "release" the connection, whereas the former, causes the the Apache HTTPClient lib to "destroy/close" the actual connection/socket. 4) When using ASYNC capability of JERSEY Server (e.g.

@POST
  @Path("/batch")
  public void createObjects(@Suspended AsyncResponse asyncResponse, List<T> objects) {
    ...
  }

), all REST responses are sent as Response objects. Thus, the situation arises.

https://java.net/jira/browse/JERSEY-2157 was resolved by just adding a

response.bufferEntity()

to the error handling code. I get where this makes sense since in an error situation, who cares necessarily if the whole entity is buffered or not.

Ultimately, the best solution would be for Jersey's ApacheConnector to manually "release" the associated connection when an error occurs or a Response object is retrieved vs relying on the "wrapped-entity-proxy, end-of-stream event occurred" in the Apache HttpClient to "release" connections back to the pool but I imagine this would require some re-work of the ApacheConnector and/or Jersey Client (particularly the JerseyInvocation.java class).

Looking at this another way, is it correct logic that a "streaming" response should force a true connection close versus releasing the connection back to the pool? In that case, physically closing the connection may be the "correct" implementation and only those requests which return complete entities should be "recycled"? Without knowing the intent of Jersey and Apache HTTPClient developers, I'm not sure I can correctly answer this question.

But some logic or fix needs to be implemented. For right now, I've just added a ResponseContextFilter to my client that calls bufferEntity on all responses:

config.register(new ClientResponseFilter() {
    @Override
    public void filter(ClientRequestContext clientRequestContext,
       ClientResponseContext clientResponseContext) throws IOException
    {
        // Ideally, I would like to get the connection object out of either the request or      // response context and then use the connection manager we create above to call         // connectionMgr.releaseConnection(connection).  But I couldn't find any connection         // object on any of the objects we have here.  I'm also not sure if releasing the connection        // might cause errors in the processing pipeline b/c the entity (if being streamed) will        // not have its underlying connection.  So perhaps bufferEntity (as below) is best. 
        // Buffer the entity so that Apache HttpClient will release the connection back to the      // pool and prevent pool starvation for any response with "Response" object         if (clientResponseContext instanceof ClientResponse) {
            ClientResponse response = (ClientResponse) clientResponseContext;
            response.bufferEntity();
        }
    }
});

[jerseyrobot]

@glassfishrobot Commented @pavelbucek said: To sum that up:

You are violating JAX-RS client contract by not calling Response#close(), because Apache connector doesn't have connection pooling correctly implemented. Can you confirm?

If thats the case, I'm inclined to change this to Enhancement and set title to "Apache Connector connection pooling support".

[jerseyrobot]

@glassfishrobot Commented nkoterba said: @Pavel -

1) I'm not sure where Response#close() should actually be called. Is the burden: a) on the user who retrieves the Response object? If I call #close(), then I get the error message above and lose the connection re-use performance of the HttpClientConnection pool b) in JerseyInvocation.java (which is independent of Apache Connector): https://github.com/jersey/jersey/blob/927b4d0605aeb119d15e9623dab65c8aec2c8e20/core-client/src/main/java/org/glassfish/jersey/client/JerseyInvocation.java#L943-L953 c) in ApacheConnector (which is probably the best place since it knows about both Apache HTTPClient and the Response pipeline and I think what you meant in your last comment. Interestingly, at the top of this class, there's a statement that specifically says that if the entity is not read from the response then ClientResponse#close() must be called: https://github.com/jersey/jersey/blob/927b4d0605aeb119d15e9623dab65c8aec2c8e20/connectors/apache-connector/src/main/java/org/glassfish/jersey/apache/connector/ApacheConnector.java#L160-L163.

If ApacheConnector is fixed to call #close() would this then negate the benefits of using the Apache HttpPoolingConnectionManager?

Finally, I'm not sure I would change this to Enhancement vs an Issue/Bug. Without a user knowing they have to explicitly call Response#close() OR Response#entityBuffer(), when using ApacheConnector, they will get connection pool starvation with default connection and socket timeout values (which happen to be infinite by default).

Since most of Jersey now relies on Autocloseable to automatically close or properly handle responses for the Jersey Client API user, it would seem odd to me to force the end API user to have to call either of these methods, but only when using ApacheConnector. Thus, I think this should remain an Issue.

[jerseyrobot]

@glassfishrobot Commented @pavelbucek said: Response#close() needs to be called when you have Response with an entity which was not fully consumed. (see the pasted javadoc above). This user of the API is required to call that, since Jersey shouldn't close the entity stream "arbitrarily", and in this case (I assume you have the HTTP 200 OK response), we cannot really do much. Please note that Jersey doesn't want to buffer any entity unless really required.

Apache connector is just a single connector we currently have and it is not the "primary" one, so there are for sure things which could be improved. This issue seems to fall into that category.

It would help if you'd share minimal reproducible testcase.

And the last thing - user needs always call #close, per javadoc (if there is unprocessed entity), so I don't see that as something, which needs to be done only for this connector.

[jerseyrobot]

@glassfishrobot Commented This issue was imported from java.net JIRA JERSEY-3149

[jerseyrobot]

@vivasur Commented Hi,

We are using jersey client with ApacheConnector and PoolingHttpClientConnectionManager. We are facing an issue where-in socket connections are not getting re-used. Our scale is very high, and we end in more than 28000 sockets lying in TIME_WAIT state.

We were using the connection pool to reuse the connection, but it looks like we are not.

I guess it related to the above issue. Is there any fix for this at least in the newer version?

[jerseyrobot]

@lachee10 Commented @vivasur,

you might want to check https://github.com/jersey/jersey/issues/3124

I think that one was responsible for the issues I had with connection pooling. I had to revert to 2.22.1 to get connection pooling working.

[cjcollins]

• The {@code close()} method should be invoked on all instances that
  • contain an un-consumed entity input stream

bufferEntity is supposed to consume and release the input stream so it is not a violation of the contract to just call bufferEntity