helidon-io / helidon

Java libraries for writing microservices
https://helidon.io
Apache License 2.0
3.5k stars 566 forks source link

Enhancement : Helidon Jersey Connector : Not reading the response fully causes connection to be discarded #8931

Closed vasanth-bhat closed 2 months ago

vasanth-bhat commented 3 months ago

Environment Details

Helidon connector does not reuse connection , when the Response is not Full Read , even when the client program explicitly closes the response. We again end up creating a new request each connection. Even when we are reusing the same Jersey client instance in the client code across requests.

Code Snippet is something like below where just the status is read from response and response is closed..

public  int  ping(String url) {  
         int status;      
         WebTarget webTarget = client.target(url); 
         try(Response response = webTarget.request().accept(MediaType.APPLICATION_JSON).get()){
             status = response.getStatus();
         }
         catch( .) {        
         }
         return status;
}

In the profile , we see that. the Http1ConenctionCache.keepAliveConenction() does not find the connection in the cache always ends up creating a new connection.

We debugged the issue further , and found that the connection was never getting added to the cache.
The adding to the cache happened only when connection.releaseResource() is invoked. This is invoked only when the response is fully read. ( entityFullyRead =true). rr in case where there is no response body. (Content-Length ‘0’). All other cases connection.closeResource() Is invoked which closes the connection.

    @Override
    public void close() {
        if (closed.compareAndSet(false, true)) {
            try {
                if (headers().contains(HeaderValues.CONNECTION_CLOSE)) {
                    connection.closeResource();
                } else {
                    **if (entityFullyRead || entityLength == 0) {**
                        connection.releaseResource();
                    } else {
                        connection.closeResource();
                    }
                }
            } finally {
                whenComplete.complete(null);
            }
        }
    }
vasanth-bhat commented 3 months ago

To test further we added additional line to read and discard the response.

public  int  check(String url) {  
         int status;      
         WebTarget webTarget = client.target(url); 
         try(Response response = webTarget.request().accept(MediaType.APPLICATION_JSON).get()){
             status = response.getStatus();
             **String responseStr = response.readEntity(String.class);**
         }
         catch( .) {        
         }
         return status;
}

Now. We see that connections are re-used and. there is improvement in performance. Below is supporting data from our tests. We see 29% improvement in throughput while using 22.6% less CPU.

vasanth-bhat commented 3 months ago

The creation of new connections an be very expensive when it involves HTTPS end points, that involves SSL handshake.

While we could say that it’s application coding issue that response is not read , it’s not uncommon for service developers to read only status or partially read the response and not read the full Response.

When close() is called on the implementation of Response() , it’s clear that there won’t be further use of that Response by the app. At this point, the implementation of close() , For ex : For example Http1ClientResponseImpl.close(), method of the Response, fully read the response and re-use the connection instead of closing the physical connection .

vasanth-bhat commented 3 months ago

It looks like the HttpURLConenction in JDK code , does seem to read the remaining response, while doing a reset of the connection for reuse.

https://github.com/openjdk/jdk/blob/jdk-21%2B35/src/java.base/share/classes/sun/net/www/protocol/http/HttpURLConnection.java#L2986

spericas commented 2 months ago

@vasanth-bhat Seems less problematic to support this if responses are length-prefixed (include Content-Length). Is that true in your use case?

vasanth-bhat commented 2 months ago

@spericas
In our specific use-case, these were simple GET requests and did include Content-Length Header in Response.

Her is my thought , We could support this approach of connection re-use with reading remaining responses where possible.

For cases where it's not feasible like, where Response is chunked ( Content-Length is not specified) , the connection is not re-used and WARNING Is logged about connection not being reused , so that app developers can see and take appropriate actions.

logovind commented 2 months ago

We have have verified with the official fix, we no longer see the issue and the connection is reused with helidon connector even though we are not reading the response entity.