eclipse-ee4j / jersey

Eclipse Jersey Project - Read our Wiki:
https://github.com/eclipse-ee4j/jersey/wiki
Other
692 stars 355 forks source link

Client using Netty connector throws `NullPointerException` when non-empty response has no `Content-Length` and is not `chunked` #4794

Open reggert opened 3 years ago

reggert commented 3 years ago

In cases when neither the Content-Length header is set nor the Transfer-Encoding header is set to chunked, the HTTP 1.1 specification prescribes that the content length be determined by when the server closes the connection (for responses with status codes that are allowed to provide content). https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.4

However, the implementation of channelRead0 in the JerseyClientHandler class in jersey-netty-connector instantiates (as its nis field) a NettyInputStream if and only if one of the above headers is set (and is nonzero, in the case of Content-Length), within the if block at https://github.com/eclipse-ee4j/jersey/blob/e129ced692959513a14a8d2d0e4e117aeaaee8ee/connectors/netty-connector/src/main/java/org/glassfish/jersey/netty/connector/JerseyClientHandler.java#L117-L118

This results in a NullPointerException at https://github.com/eclipse-ee4j/jersey/blob/e129ced692959513a14a8d2d0e4e117aeaaee8ee/connectors/netty-connector/src/main/java/org/glassfish/jersey/netty/connector/JerseyClientHandler.java#L141 when processing the first HttpContent objects representing the bodies (when present) of such responses.

I observed the error using jersey-netty-connector 2.32 and netty-all 4.1.29.Final.

jbescos commented 3 years ago

Hi @reggert do you have a reproducer?.

I have created the next test to debug it, it closes the server meanwhile the request is ongoing:

public class JerseyClientHandlerTest extends JerseyTest {

    private static final Logger LOGGER = Logger.getLogger(JerseyClientHandlerTest.class.getName());
    private static final CountDownLatch requestLatch = new CountDownLatch(1);
    private static final CountDownLatch serverLatch = new CountDownLatch(1);

    @Path("test")
    public static class Resource {
        @GET
        @Produces("text/plain")
        public String test() throws InterruptedException {
            requestLatch.countDown();
            // Do not complete the response. We want the server shuts down before responding.
            serverLatch.await();
            return "test";
        }
    }

    @Override
    protected Application configure() {
        ResourceConfig config = new ResourceConfig(Resource.class);
        config.register(new LoggingFeature(LOGGER, LoggingFeature.Verbosity.PAYLOAD_ANY));
        return config;
    }

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

    @Test
    public void issue4794() throws Exception {
        ExecutorService executor = Executors.newSingleThreadExecutor();
        Future<Response> future = executor.submit(() -> target().path("/test").request().get());
        // Wait till request reaches the resource before shuting the server down
        requestLatch.await();
        // Force server shutdown
        super.tearDown();
        // Release the thread
        serverLatch.countDown();
        assertEquals(500, future.get().getStatus());
    }
}

And I am able to reach the part you mention with empty headers: image

But it does not fail with null pointer exception because msg instanceof HttpResponse and is not msg instanceof HttpContent. Then, it does not execute the line where the exception should happen.