From c3889873f66c946f8848291d887d20d6f125a02b Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 11 Dec 2015 12:25:54 +0100 Subject: [PATCH] 484167 - GOAWAY frames aren't handling disconnects appropriately on Client. Fixed by overriding onClose() to listen for GOAWAY frames, and acting appropriately. --- .../http/HttpClientTransportOverHTTP2.java | 67 ++++++++++++------- .../client/http/HttpConnectionOverHTTP2.java | 2 +- .../client/HttpClientIdleTimeoutTest.java | 26 +++++++ 3 files changed, 69 insertions(+), 26 deletions(-) diff --git a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java index ea16c18feaf..6e2adcb1f17 100644 --- a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java +++ b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java @@ -32,6 +32,7 @@ import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.http2.api.Session; import org.eclipse.jetty.http2.client.HTTP2Client; import org.eclipse.jetty.http2.client.HTTP2ClientConnectionFactory; +import org.eclipse.jetty.http2.frames.GoAwayFrame; import org.eclipse.jetty.io.ClientConnectionFactory; import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.io.ssl.SslClientConnectionFactory; @@ -107,37 +108,15 @@ public class HttpClientTransportOverHTTP2 extends ContainerLifeCycle implements final HttpDestination destination = (HttpDestination)context.get(HTTP_DESTINATION_CONTEXT_KEY); @SuppressWarnings("unchecked") - final Promise connection = (Promise)context.get(HTTP_CONNECTION_PROMISE_CONTEXT_KEY); + final Promise connectionPromise = (Promise)context.get(HTTP_CONNECTION_PROMISE_CONTEXT_KEY); - Session.Listener listener = new Session.Listener.Adapter() - { - @Override - public void onFailure(Session session, Throwable failure) - { - destination.abort(failure); - } - }; - - final Promise promise = new Promise() - { - @Override - public void succeeded(Session session) - { - connection.succeeded(newHttpConnection(destination, session)); - } - - @Override - public void failed(Throwable failure) - { - connection.failed(failure); - } - }; + SessionListenerPromise listenerPromise = new SessionListenerPromise(destination, connectionPromise); SslContextFactory sslContextFactory = null; if (HttpScheme.HTTPS.is(destination.getScheme())) sslContextFactory = httpClient.getSslContextFactory(); - client.connect(sslContextFactory, address, listener, promise, context); + client.connect(sslContextFactory, address, listenerPromise, listenerPromise, context); } @Override @@ -154,4 +133,42 @@ public class HttpClientTransportOverHTTP2 extends ContainerLifeCycle implements { return new HttpConnectionOverHTTP2(destination, session); } + + private class SessionListenerPromise extends Session.Listener.Adapter implements Promise + { + private final HttpDestination destination; + private final Promise promise; + private HttpConnectionOverHTTP2 connection; + + public SessionListenerPromise(HttpDestination destination, Promise promise) + { + this.destination = destination; + this.promise = promise; + } + + @Override + public void succeeded(Session session) + { + connection = newHttpConnection(destination, session); + promise.succeeded(connection); + } + + @Override + public void failed(Throwable failure) + { + promise.failed(failure); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + connection.close(); + } + + @Override + public void onFailure(Session session, Throwable failure) + { + connection.abort(failure); + } + } } diff --git a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java index 0ad973f28f1..98bff3286a1 100644 --- a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java +++ b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java @@ -69,7 +69,7 @@ public class HttpConnectionOverHTTP2 extends HttpConnection abort(new AsynchronousCloseException()); } - private void abort(Throwable failure) + protected void abort(Throwable failure) { for (HttpChannel channel : channels) { diff --git a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientIdleTimeoutTest.java b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientIdleTimeoutTest.java index e168a0f3ef4..c38bceb2960 100644 --- a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientIdleTimeoutTest.java +++ b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientIdleTimeoutTest.java @@ -27,6 +27,8 @@ import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.handler.AbstractHandler; import org.junit.Assert; @@ -93,4 +95,28 @@ public class HttpClientIdleTimeoutTest extends AbstractTest Assert.assertTrue(latch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); } + + @Test + public void testServerIdleTimeout() throws Exception + { + start(new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + } + }); + connector.setIdleTimeout(idleTimeout); + + ContentResponse response1 = client.newRequest(newURI()).send(); + Assert.assertEquals(HttpStatus.OK_200, response1.getStatus()); + + // Let the server idle timeout. + Thread.sleep(2 * idleTimeout); + + // Make sure we can make another request successfully. + ContentResponse response2 = client.newRequest(newURI()).send(); + Assert.assertEquals(HttpStatus.OK_200, response2.getStatus()); + } }