From 846d560b44abafc5defbf79f58521d7ce532797b Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 4 Nov 2016 12:09:44 +0100 Subject: [PATCH] Fixes #905 - Jetty terminates SSL connections too early with Connection: close. Requests with "Connection: close" are now closed only after the request/response exchange has been terminated. --- .../client/http/HttpChannelOverHTTP.java | 4 +- .../jetty/client/http/HttpSenderOverHTTP.java | 8 +- .../client/ClientConnectionCloseTest.java | 229 ++++++++++++++---- 3 files changed, 192 insertions(+), 49 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpChannelOverHTTP.java b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpChannelOverHTTP.java index cdbf1fad239..a2ce655af4a 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpChannelOverHTTP.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpChannelOverHTTP.java @@ -103,6 +103,8 @@ public class HttpChannelOverHTTP extends HttpChannel closeReason = "failure"; else if (receiver.isShutdown()) closeReason = "server close"; + else if (sender.isShutdown()) + closeReason = "client close"; if (closeReason == null) { @@ -117,7 +119,7 @@ public class HttpChannelOverHTTP extends HttpChannel } else { - // HTTP 1.1 or greater closes only if it has an explicit close. + // HTTP 1.1 closes only if it has an explicit close. if (responseHeaders.contains(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString())) closeReason = "http/1.1"; } diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java index c16b8162c04..1e06e6b9058 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java @@ -38,6 +38,7 @@ import org.eclipse.jetty.util.IteratingCallback; public class HttpSenderOverHTTP extends HttpSender { private final HttpGenerator generator = new HttpGenerator(); + private boolean shutdown; public HttpSenderOverHTTP(HttpChannelOverHTTP channel) { @@ -149,7 +150,12 @@ public class HttpSenderOverHTTP extends HttpSender { if (LOG.isDebugEnabled()) LOG.debug("Request shutdown output {}", getHttpExchange().getRequest()); - getHttpChannel().getHttpConnection().getEndPoint().shutdownOutput(); + shutdown = true; + } + + protected boolean isShutdown() + { + return shutdown; } @Override diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/ClientConnectionCloseTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/ClientConnectionCloseTest.java index ff7eb33b8c3..cc7f3db4735 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/ClientConnectionCloseTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/ClientConnectionCloseTest.java @@ -19,26 +19,26 @@ package org.eclipse.jetty.client; import java.io.IOException; +import java.io.InterruptedIOException; import java.nio.ByteBuffer; -import java.nio.charset.StandardCharsets; -import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import javax.servlet.ServletException; import javax.servlet.ServletInputStream; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import org.eclipse.jetty.client.api.ContentProvider; import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.client.http.HttpConnectionOverHTTP; +import org.eclipse.jetty.client.http.HttpDestinationOverHTTP; import org.eclipse.jetty.client.util.DeferredContentProvider; import org.eclipse.jetty.client.util.StringContentProvider; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpHeaderValue; import org.eclipse.jetty.http.HttpStatus; -import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.handler.AbstractHandler; -import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.junit.Assert; import org.junit.Test; @@ -51,43 +51,16 @@ public class ClientConnectionCloseTest extends AbstractHttpClientServerTest } @Test - public void testClientConnectionCloseShutdownOutputWithoutRequestContent() throws Exception + public void test_ClientConnectionClose_ServerConnectionClose_ClientClosesAfterExchange() throws Exception { - testClientConnectionCloseShutdownOutput(null); - } - - @Test - public void testClientConnectionCloseShutdownOutputWithRequestContent() throws Exception - { - testClientConnectionCloseShutdownOutput(new StringContentProvider("data", StandardCharsets.UTF_8)); - } - - @Test - public void testClientConnectionCloseShutdownOutputWithChunkedRequestContent() throws Exception - { - DeferredContentProvider content = new DeferredContentProvider() - { - @Override - public long getLength() - { - return -1; - } - }; - content.offer(ByteBuffer.wrap("data".getBytes(StandardCharsets.UTF_8))); - content.close(); - testClientConnectionCloseShutdownOutput(content); - } - - private void testClientConnectionCloseShutdownOutput(ContentProvider content) throws Exception - { - AtomicReference ref = new AtomicReference<>(); + byte[] data = new byte[128 * 1024]; start(new AbstractHandler() { @Override public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { baseRequest.setHandled(true); - ref.set(baseRequest.getHttpChannel().getEndPoint()); + ServletInputStream input = request.getInputStream(); while (true) { @@ -95,28 +68,190 @@ public class ClientConnectionCloseTest extends AbstractHttpClientServerTest if (read < 0) break; } - response.setStatus(HttpStatus.OK_200); + + response.setContentLength(data.length); + response.getOutputStream().write(data); + + try + { + // Delay the server from sending the TCP FIN. + Thread.sleep(1000); + } + catch (InterruptedException x) + { + throw new InterruptedIOException(); + } } }); - ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) + String host = "localhost"; + int port = connector.getLocalPort(); + + HttpDestinationOverHTTP destination = (HttpDestinationOverHTTP)client.getDestination(scheme, host, port); + DuplexConnectionPool connectionPool = destination.getConnectionPool(); + + ContentResponse response = client.newRequest(host, port) .scheme(scheme) - .path("/ctx/path") .header(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString()) - .content(content) + .content(new StringContentProvider("0")) + .onRequestSuccess(request -> + { + HttpConnectionOverHTTP connection = (HttpConnectionOverHTTP)connectionPool.getActiveConnections().peek(); + Assert.assertFalse(connection.getEndPoint().isOutputShutdown()); + }) .send(); Assert.assertEquals(HttpStatus.OK_200, response.getStatus()); + Assert.assertArrayEquals(data, response.getContent()); + Assert.assertEquals(0, connectionPool.getConnectionCount()); + } - // Wait for the FIN to arrive to the server - Thread.sleep(1000); + @Test + public void test_ClientConnectionClose_ServerDoesNotRespond_ClientIdleTimeout() throws Exception + { + start(new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + request.startAsync(); + // Do not respond. + } + }); - // Do not read from the server because it will trigger - // the send of the TLS Close Message before the response. + String host = "localhost"; + int port = connector.getLocalPort(); - EndPoint serverEndPoint = ref.get(); - ByteBuffer buffer = BufferUtil.allocate(1); - int read = serverEndPoint.fill(buffer); - Assert.assertEquals(-1, read); + HttpDestinationOverHTTP destination = (HttpDestinationOverHTTP)client.getDestination(scheme, host, port); + DuplexConnectionPool connectionPool = destination.getConnectionPool(); + + CountDownLatch resultLatch = new CountDownLatch(1); + long idleTimeout = 1000; + client.newRequest(host, port) + .scheme(scheme) + .header(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString()) + .idleTimeout(idleTimeout, TimeUnit.MILLISECONDS) + .onRequestSuccess(request -> + { + HttpConnectionOverHTTP connection = (HttpConnectionOverHTTP)connectionPool.getActiveConnections().peek(); + Assert.assertFalse(connection.getEndPoint().isOutputShutdown()); + }) + .send(result -> + { + if (result.isFailed()) + resultLatch.countDown(); + }); + + Assert.assertTrue(resultLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); + Assert.assertEquals(0, connectionPool.getConnectionCount()); + } + + @Test + public void test_ClientConnectionClose_ServerPartialResponse_ClientIdleTimeout() throws Exception + { + long idleTimeout = 1000; + start(new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + + ServletInputStream input = request.getInputStream(); + while (true) + { + int read = input.read(); + if (read < 0) + break; + } + + response.getOutputStream().print("Hello"); + response.flushBuffer(); + + try + { + Thread.sleep(2 * idleTimeout); + } + catch (InterruptedException x) + { + throw new InterruptedIOException(); + } + } + }); + + String host = "localhost"; + int port = connector.getLocalPort(); + + HttpDestinationOverHTTP destination = (HttpDestinationOverHTTP)client.getDestination(scheme, host, port); + DuplexConnectionPool connectionPool = destination.getConnectionPool(); + + DeferredContentProvider content = new DeferredContentProvider(ByteBuffer.allocate(8)); + CountDownLatch resultLatch = new CountDownLatch(1); + client.newRequest(host, port) + .scheme(scheme) + .header(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString()) + .content(content) + .idleTimeout(idleTimeout, TimeUnit.MILLISECONDS) + .onRequestSuccess(request -> + { + HttpConnectionOverHTTP connection = (HttpConnectionOverHTTP)connectionPool.getActiveConnections().peek(); + Assert.assertFalse(connection.getEndPoint().isOutputShutdown()); + }) + .send(result -> + { + if (result.isFailed()) + resultLatch.countDown(); + }); + content.offer(ByteBuffer.allocate(8)); + content.close(); + + Assert.assertTrue(resultLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); + Assert.assertEquals(0, connectionPool.getConnectionCount()); + } + + @Test + public void test_ClientConnectionClose_ServerNoConnectionClose_ClientCloses() throws Exception + { + start(new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + response.setContentLength(0); + response.flushBuffer(); + + try + { + // Delay the server from sending the TCP FIN. + Thread.sleep(1000); + } + catch (InterruptedException x) + { + throw new InterruptedIOException(); + } + } + }); + + String host = "localhost"; + int port = connector.getLocalPort(); + + HttpDestinationOverHTTP destination = (HttpDestinationOverHTTP)client.getDestination(scheme, host, port); + DuplexConnectionPool connectionPool = destination.getConnectionPool(); + + ContentResponse response = client.newRequest(host, port) + .scheme(scheme) + .header(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString()) + .onRequestSuccess(request -> + { + HttpConnectionOverHTTP connection = (HttpConnectionOverHTTP)connectionPool.getActiveConnections().peek(); + Assert.assertFalse(connection.getEndPoint().isOutputShutdown()); + }) + .onResponseHeaders(r -> r.getHeaders().remove(HttpHeader.CONNECTION)) + .send(); + + Assert.assertEquals(HttpStatus.OK_200, response.getStatus()); + Assert.assertEquals(0, connectionPool.getConnectionCount()); } }