javaee / jersey

This is no longer the active Jersey repository. Please see the README.md
http://jersey.github.io
Other
2.86k stars 2.35k forks source link

ApacheConnector: closing ClientResponse throws if not consumed #3486

Open glassfishrobot opened 7 years ago

glassfishrobot commented 7 years ago

3124 seems to have introduced a bug, with response.close() throwing an unexpected ProcessingException if the response has not been consumed. That is quite unfortunate because this ProcessingException overrides any original exception if close is called inside a finally-block. (And try-with-resources is not yet supported by JAX-RS 2.0).

javax.ws.rs.ProcessingException: Error closing message content input stream.
    at org.glassfish.jersey.message.internal.EntityInputStream.close(EntityInputStream.java:161)
    at org.glassfish.jersey.message.internal.InboundMessageContext$EntityContent.close(InboundMessageContext.java:156)
    at org.glassfish.jersey.message.internal.InboundMessageContext.close(InboundMessageContext.java:939)
    at org.glassfish.jersey.client.InboundJaxrsResponse.close(InboundJaxrsResponse.java:167)
Caused by: org.apache.http.ConnectionClosedException: Premature end of Content-Length delimited message body (expected: 4; received: 0
    at org.apache.http.impl.io.ContentLengthInputStream.read(ContentLengthInputStream.java:178)
    at org.apache.http.impl.io.ContentLengthInputStream.read(ContentLengthInputStream.java:198)
    at org.apache.http.impl.io.ContentLengthInputStream.close(ContentLengthInputStream.java:101)
    at org.apache.http.impl.execchain.ResponseEntityProxy.streamClosed(ResponseEntityProxy.java:140)
    at org.apache.http.conn.EofSensorInputStream.checkClose(EofSensorInputStream.java:228)
    at org.apache.http.conn.EofSensorInputStream.close(EofSensorInputStream.java:174)
    at java.io.BufferedInputStream.close(BufferedInputStream.java:483)
    at java.io.FilterInputStream.close(FilterInputStream.java:181)
    at org.glassfish.jersey.apache.connector.ApacheConnector$3.close(ApacheConnector.java:644)
    at java.io.FilterInputStream.close(FilterInputStream.java:181)
    at org.glassfish.jersey.apache.connector.ApacheConnector$HttpClientResponseInputStream.close(ApacheConnector.java:621)
    at org.glassfish.jersey.message.internal.EntityInputStream.close(EntityInputStream.java:158)
    ... 29 more

Seems to affect Jersey 2.22.2 upwards; experienced it in jersey 2.23.1

[INFO] +- org.glassfish.jersey.connectors:jersey-apache-connector:jar:2.23.1:test
[INFO] |  +- org.apache.httpcomponents:httpclient:jar:4.5.2:test (version managed from 4.5)
[INFO] |  |  +- org.apache.httpcomponents:httpcore:jar:4.4.5:test (version managed from 4.4.4)
[INFO] |  |  \- commons-codec:commons-codec:jar:1.10:test (version managed from 1.9)
[INFO] |  +- (org.glassfish.jersey.core:jersey-client:jar:2.23.1:test - omitted for duplicate)
[INFO] |  \- (javax.ws.rs:javax.ws.rs-api:jar:2.0.1:test - omitted for duplicate)

Reproducable by this UnitTest:

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

import javax.ws.rs.GET;
import javax.ws.rs.Path;
import javax.ws.rs.ProcessingException;
import javax.ws.rs.Produces;
import javax.ws.rs.core.Application;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;

import org.glassfish.jersey.apache.connector.ApacheConnectorProvider;
import org.glassfish.jersey.client.ClientConfig;
import org.glassfish.jersey.server.ResourceConfig;
import org.glassfish.jersey.test.JerseyTest;
import org.glassfish.jersey.test.jdkhttp.JdkHttpServerTestContainerFactory;
import org.glassfish.jersey.test.spi.TestContainerException;
import org.glassfish.jersey.test.spi.TestContainerFactory;
import org.junit.Test;

public class FunctionalCloseableItTest extends JerseyTest {

    @Override
    protected Application configure() {
        ResourceConfig config = new ResourceConfig();
        config.register(TestResource.class);
        return config;
    }

    @Override
    protected void configureClient(ClientConfig config) {
        config.connectorProvider(new ApacheConnectorProvider());
    }

    @Override
    protected TestContainerFactory getTestContainerFactory() throws TestContainerException {
        return new JdkHttpServerTestContainerFactory();
    }

    @Test
    public void testProcessingExceptionThrownInCloseOverridesMyException() throws Exception {
        try {
            Response response = target().path("productInfo")
.request(MediaType.TEXT_PLAIN_TYPE)
.get();
            try {
assertEquals(200, response.getStatus());
// this exception is unfortunately overridden in finally-block
throw new MyException();
            } finally {
// BUG: will throw because not consumed
response.close();
            }
        } catch (ProcessingException e) {
            e.printStackTrace();
            assertEquals("Error closing message content input stream.", e.getMessage());
            assertEquals(0, e.getSuppressed().length);
        }
    }

    @SuppressWarnings("serial")
    private static class MyException extends Exception {

    }

    @Path("/")
    public static class TestResource {
        @GET
        @Path("/productInfo")
        @Produces(MediaType.TEXT_PLAIN)
        public String getProductInfo() {
            return "foo\n";
        }
    }
}

The thrown exception seems like a bug to me because #3124 fully intended to prematurely close the connection instead of consuming it. An Exception would not make sense. There is also the javadoc of Response#close():

The 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.

The bug seems to be caused by Jersey closing both HttpResponse and response-Stream. First closing the HttpResponse closes the socket-connection. The second close of the response-stream tries to consume the remaining body, but the socket is already closed, which is considered a protocol error by apache httpclient. Not closing the stream probably circumvents the exception.

I may prepare a PullRequest if i get the OK from my higher-ups to sign OCA (real PITA).

glassfishrobot commented 7 years ago

Reported by mkull

glassfishrobot commented 7 years ago

predatorvi said: [Edit] Using Jersey 2.25.1 (client, core, apache-connector, entity-filtering, guava, media-json-jackson, media-multipart)

Summary of my experience with the same exception as noted above:

Detail: I recently made updates in my code to use PoolingHttpClientConnectionManager along with the ApacheConnectorProvider to improve scalability for test automation. My initialization code:

**RestClientJersey.RestClientJersey()** public RestClientJersey(int timeout, int chunkSize) {
        SSLContext ctx = initSSLTrustFactory();

        Registry<ConnectionSocketFactory> registry = RegistryBuilder.<ConnectionSocketFactory>create()
.register("http", PlainConnectionSocketFactory.getSocketFactory())
.register("https", new SSLConnectionSocketFactory(ctx, NoopHostnameVerifier.INSTANCE))
.build();

        ClientConfig cfg = new ClientConfig();

        PoolingHttpClientConnectionManager connMan = new PoolingHttpClientConnectionManager(registry);
        connMan.setMaxTotal(200);
        connMan.setDefaultMaxPerRoute(50);

        if (timeout != -1) {
            SocketConfig sc = SocketConfig.custom()
    .setSoTimeout(timeout)
    .setTcpNoDelay(true)
    .setSoReuseAddress(true)
    .build();
            connMan.setDefaultSocketConfig(sc);
            cfg.property(ApacheClientProperties.REQUEST_CONFIG, RequestConfig
    .custom()
    .setConnectTimeout(timeout)
    .setSocketTimeout(timeout)
    .build()
            );
        }
        cfg.property(ApacheClientProperties.CONNECTION_MANAGER, connMan);

        ApacheConnectorProvider acp = new ApacheConnectorProvider();
        cfg.connectorProvider(acp);

        ClientBuilder builder = ClientBuilder.newBuilder();
        client = builder
.hostnameVerifier((String hostname, SSLSession session) -> true)
.withConfig(cfg)
.register(JacksonFeature.class)
.register(MultiPartWriter.class)
.build();
    }

My cleanup code below is called before attempting another request. I store the last Response on a per-thread basis so my tests don't have to worry about cleaning up and I can make sure it is closed, regardless of whether the entity was consumed or not.

**myJerseyClient.java:responseCleanup()**    /**
     * If previous request didn't need/want the entity, make sure previous request
     * is closed to free up resources.
     */
    private void responseCleanup() {
        Response r = getLastResponse();
        if (r != null) {
            r.close();  //Should be idempotent and can be called again w/out issue
        }
    }

This worked fine for HTTPS requests, presumably using the SSLConnectionSocketFactory. However, when switching to HTTP (via the PlainConnectionSocketFactory), the exception occurs.

I changed my responseCleanup() method to be:

**myJerseyClient.java:fresponseCleanup()**    private void responseCleanup() {
        Response r = getLastResponse();
        if (r != null) {
            r.close();
            lastResponse.put(Thread.currentThread().getId(), null); //Works now if I prevent more than one call to close()
        }
    }

An additional scenario I noticed has issues is after calling readEntity(Class entityType) I want to grab the raw payload (such as JSON) in case of parsing problems. This is the code I used for that case:

**myJerseyClient.java:bufferEntitySnippet**        T retInstance = null;
        if (returnType != null) {
            r.bufferEntity();
            retInstance = r.readEntity(returnType);

            if (retInstance instanceof InjectRawPayload) {
((InjectRawPayload) retInstance).setRawPayload(r.readEntity(String.class));
            }
        }
        return retInstance;

However, when I call readEntity(String.class) to retrieve the raw JSON, an exception is thrown.

java.lang.IllegalStateException: Entity input stream has already been closed.
    at org.glassfish.jersey.message.internal.EntityInputStream.ensureNotClosed(EntityInputStream.java:228)
    at org.glassfish.jersey.message.internal.InboundMessageContext.readEntity(InboundMessageContext.java:854)
    at org.glassfish.jersey.message.internal.InboundMessageContext.readEntity(InboundMessageContext.java:808)
    at org.glassfish.jersey.client.ClientResponse.readEntity(ClientResponse.java:326)
    at org.glassfish.jersey.client.InboundJaxrsResponse$1.call(InboundJaxrsResponse.java:115)
    at org.glassfish.jersey.internal.Errors.process(Errors.java:315)
    at org.glassfish.jersey.internal.Errors.process(Errors.java:297)
    at org.glassfish.jersey.internal.Errors.process(Errors.java:228)
    at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:419)
    at org.glassfish.jersey.client.InboundJaxrsResponse.runInScopeIfPossible(InboundJaxrsResponse.java:267)
    at org.glassfish.jersey.client.InboundJaxrsResponse.readEntity(InboundJaxrsResponse.java:112)
...
glassfishrobot commented 7 years ago

Issue-Links: is cloned by JERSEY-3233

glassfishrobot commented 7 years ago

This issue was imported from java.net JIRA JERSEY-3214