From 92c7b991d8f625014d0051d2fe4d91567ef68821 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sat, 6 Aug 2016 12:29:22 +0200 Subject: [PATCH 1/2] Code cleanups. --- .../src/main/java/org/eclipse/jetty/http2/HTTP2Session.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java index 40a19142836..15b138e65c9 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java @@ -337,7 +337,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio if (reply) { - SettingsFrame replyFrame = new SettingsFrame(Collections.emptyMap(), true); + SettingsFrame replyFrame = new SettingsFrame(Collections.emptyMap(), true); settings(replyFrame, Callback.NOOP); } } @@ -658,7 +658,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio while (true) { int localCount = localStreamCount.get(); - int maxCount = maxLocalStreams; + int maxCount = getMaxLocalStreams(); if (maxCount >= 0 && localCount >= maxCount) { promise.failed(new IllegalStateException("Max local stream count " + maxCount + " exceeded")); From 45695e08aaefdd79152a90853605cfef9a4b73de Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sat, 6 Aug 2016 12:31:09 +0200 Subject: [PATCH 2/2] Fixes #792 - [HTTP/2] Socket seems to be not closed completely. Now sending the GOAWAY and then disconnecting from doStop(), ensuring that the underlying TCP connection is closed even if the server does not close. --- .../org/eclipse/jetty/http2/HTTP2Session.java | 20 +++ .../HttpClientTransportOverHTTP2Test.java | 115 ++++++++++++++++++ 2 files changed, 135 insertions(+) diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java index 15b138e65c9..914329656ad 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java @@ -113,6 +113,26 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio super.doStart(); } + @Override + protected void doStop() throws Exception + { + super.doStop(); + close(ErrorCode.NO_ERROR.code, "stop", new Callback.NonBlocking() + { + @Override + public void succeeded() + { + disconnect(); + } + + @Override + public void failed(Throwable x) + { + disconnect(); + } + }); + } + @ManagedAttribute(value = "The flow control strategy", readonly = true) public FlowControlStrategy getFlowControlStrategy() { diff --git a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2Test.java b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2Test.java index 6bf2e64d2f1..6090f76affb 100644 --- a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2Test.java +++ b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2Test.java @@ -19,8 +19,15 @@ package org.eclipse.jetty.http2.client.http; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.net.ServerSocket; +import java.net.Socket; +import java.net.SocketTimeoutException; import java.nio.ByteBuffer; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; @@ -33,6 +40,7 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.HttpDestination; import org.eclipse.jetty.client.HttpProxy; import org.eclipse.jetty.client.api.ContentResponse; import org.eclipse.jetty.http.HttpFields; @@ -41,6 +49,7 @@ import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http2.ErrorCode; +import org.eclipse.jetty.http2.HTTP2Session; import org.eclipse.jetty.http2.HTTP2Stream; import org.eclipse.jetty.http2.api.Session; import org.eclipse.jetty.http2.api.Stream; @@ -51,10 +60,15 @@ import org.eclipse.jetty.http2.frames.GoAwayFrame; import org.eclipse.jetty.http2.frames.HeadersFrame; import org.eclipse.jetty.http2.frames.ResetFrame; import org.eclipse.jetty.http2.frames.SettingsFrame; +import org.eclipse.jetty.http2.generator.Generator; +import org.eclipse.jetty.http2.parser.ServerParser; import org.eclipse.jetty.http2.server.RawHTTP2ServerConnectionFactory; +import org.eclipse.jetty.io.ByteBufferPool; +import org.eclipse.jetty.io.MappedByteBufferPool; import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.handler.AbstractHandler; +import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.eclipse.jetty.util.thread.QueuedThreadPool; @@ -390,6 +404,107 @@ public class HttpClientTransportOverHTTP2Test extends AbstractTest Assert.assertTrue(resetLatch.await(5, TimeUnit.SECONDS)); } + @Test + public void testClientStopsServerDoesNotCloseClientCloses() throws Exception + { + try (ServerSocket server = new ServerSocket(0)) + { + List sessions = new ArrayList<>(); + HTTP2Client h2Client = new HTTP2Client(); + HttpClient client = new HttpClient(new HttpClientTransportOverHTTP2(h2Client) + { + @Override + protected HttpConnectionOverHTTP2 newHttpConnection(HttpDestination destination, Session session) + { + sessions.add(session); + return super.newHttpConnection(destination, session); + } + }, null); + QueuedThreadPool clientExecutor = new QueuedThreadPool(); + clientExecutor.setName("client"); + client.setExecutor(clientExecutor); + client.start(); + + CountDownLatch resultLatch = new CountDownLatch(1); + client.newRequest("localhost", server.getLocalPort()) + .send(result -> + { + if (result.getResponse().getStatus() == HttpStatus.OK_200) + resultLatch.countDown(); + }); + + ByteBufferPool byteBufferPool = new MappedByteBufferPool(); + ByteBufferPool.Lease lease = new ByteBufferPool.Lease(byteBufferPool); + Generator generator = new Generator(byteBufferPool); + + try (Socket socket = server.accept()) + { + socket.setSoTimeout(1000); + OutputStream output = socket.getOutputStream(); + InputStream input = socket.getInputStream(); + + ServerParser parser = new ServerParser(byteBufferPool, new ServerParser.Listener.Adapter() + { + @Override + public void onHeaders(HeadersFrame request) + { + // Server's preface. + generator.control(lease, new SettingsFrame(new HashMap<>(), false)); + // Reply to client's SETTINGS. + generator.control(lease, new SettingsFrame(new HashMap<>(), true)); + // Response. + MetaData.Response metaData = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields()); + HeadersFrame response = new HeadersFrame(request.getStreamId(), metaData, null, true); + generator.control(lease, response); + + try + { + // Write the frames. + for (ByteBuffer buffer : lease.getByteBuffers()) + output.write(BufferUtil.toArray(buffer)); + } + catch (Throwable x) + { + x.printStackTrace(); + } + } + }, 4096, 8192); + + byte[] bytes = new byte[1024]; + while (true) + { + try + { + int read = input.read(bytes); + if (read < 0) + Assert.fail(); + parser.parse(ByteBuffer.wrap(bytes, 0, read)); + } + catch (SocketTimeoutException x) + { + break; + } + } + + Assert.assertTrue(resultLatch.await(5, TimeUnit.SECONDS)); + + // The client will send a GO_AWAY, but the server will not close. + client.stop(); + + // Give some time to process the stop/close operations. + Thread.sleep(1000); + + Assert.assertTrue(h2Client.getBeans(Session.class).isEmpty()); + + for (Session session : sessions) + { + Assert.assertTrue(session.isClosed()); + Assert.assertTrue(((HTTP2Session)session).isDisconnected()); + } + } + } + } + @Ignore @Test public void testExternalServer() throws Exception