From 3c26ffb227089fc02b59c439c2000850b3399621 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 19 Oct 2015 11:17:22 +0200 Subject: [PATCH] 480061 - HTTP/2 server doesn't send GOAWAY frame when shutting down. Fixed by overriding Connection.close() and sending the GOAWAY from there, which is triggered when the SelectorManager stops. This allowed simplification of client code too. --- .../jetty/http2/client/HTTP2Client.java | 36 --------------- .../client/HTTP2ClientConnectionFactory.java | 8 ---- .../eclipse/jetty/http2/client/HTTP2Test.java | 46 +++++++++++++++++++ .../eclipse/jetty/http2/HTTP2Connection.java | 9 ++++ .../org/eclipse/jetty/http2/HTTP2Session.java | 2 - 5 files changed, 55 insertions(+), 46 deletions(-) diff --git a/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2Client.java b/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2Client.java index f0388955387..6256a6d2c40 100644 --- a/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2Client.java +++ b/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2Client.java @@ -26,13 +26,9 @@ import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Queue; -import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.Executor; import org.eclipse.jetty.alpn.client.ALPNClientConnectionFactory; -import org.eclipse.jetty.http2.ErrorCode; -import org.eclipse.jetty.http2.ISession; import org.eclipse.jetty.http2.api.Session; import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.ClientConnectionFactory; @@ -43,7 +39,6 @@ import org.eclipse.jetty.io.MappedByteBufferPool; import org.eclipse.jetty.io.SelectChannelEndPoint; import org.eclipse.jetty.io.SelectorManager; import org.eclipse.jetty.io.ssl.SslClientConnectionFactory; -import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.Promise; import org.eclipse.jetty.util.component.ContainerLifeCycle; import org.eclipse.jetty.util.ssl.SslContextFactory; @@ -117,7 +112,6 @@ public class HTTP2Client extends ContainerLifeCycle private Scheduler scheduler; private ByteBufferPool bufferPool; private ClientConnectionFactory connectionFactory; - private Queue sessions; private SelectorManager selector; private int selectors = 1; private long idleTimeout = 30000; @@ -150,12 +144,6 @@ public class HTTP2Client extends ContainerLifeCycle }); } - if (sessions == null) - { - sessions = new ConcurrentLinkedQueue<>(); - addBean(sessions); - } - if (selector == null) { selector = newSelectorManager(); @@ -171,13 +159,6 @@ public class HTTP2Client extends ContainerLifeCycle return new ClientSelectorManager(getExecutor(), getScheduler(), getSelectors()); } - @Override - protected void doStop() throws Exception - { - closeConnections(); - super.doStop(); - } - public Executor getExecutor() { return executor; @@ -329,23 +310,6 @@ public class HTTP2Client extends ContainerLifeCycle channel.socket().setTcpNoDelay(true); } - private void closeConnections() - { - for (ISession session : sessions) - session.close(ErrorCode.NO_ERROR.code, null, Callback.NOOP); - sessions.clear(); - } - - public boolean addSession(ISession session) - { - return sessions.offer(session); - } - - public boolean removeSession(ISession session) - { - return sessions.remove(session); - } - private class ClientSelectorManager extends SelectorManager { private ClientSelectorManager(Executor executor, Scheduler scheduler, int selectors) diff --git a/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java b/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java index e630517d9a0..eda74e35c2d 100644 --- a/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java +++ b/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java @@ -125,17 +125,9 @@ public class HTTP2ClientConnectionFactory implements ClientConnectionFactory super.onOpen(); } - @Override - public void onClose() - { - super.onClose(); - client.removeSession(getSession()); - } - @Override public void succeeded() { - client.addSession(getSession()); promise.succeeded(getSession()); } diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/HTTP2Test.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/HTTP2Test.java index 4cf327a7311..03ad6b23c60 100644 --- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/HTTP2Test.java +++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/HTTP2Test.java @@ -37,7 +37,9 @@ import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http2.api.Session; import org.eclipse.jetty.http2.api.Stream; +import org.eclipse.jetty.http2.api.server.ServerSessionListener; import org.eclipse.jetty.http2.frames.DataFrame; +import org.eclipse.jetty.http2.frames.GoAwayFrame; import org.eclipse.jetty.http2.frames.HeadersFrame; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.Jetty; @@ -239,4 +241,48 @@ public class HTTP2Test extends AbstractTest Assert.assertTrue(latch.await(5, TimeUnit.SECONDS)); } + + @Test + public void testServerSendsGoAwayOnStop() throws Exception + { + start(new ServerSessionListener.Adapter()); + + CountDownLatch closeLatch = new CountDownLatch(1); + newClient(new Session.Listener.Adapter() + { + @Override + public void onClose(Session session, GoAwayFrame frame) + { + closeLatch.countDown(); + } + }); + + Thread.sleep(1000); + + server.stop(); + + Assert.assertTrue(closeLatch.await(5, TimeUnit.SECONDS)); + } + + @Test + public void testClientSendsGoAwayOnStop() throws Exception + { + CountDownLatch closeLatch = new CountDownLatch(1); + start(new ServerSessionListener.Adapter() + { + @Override + public void onClose(Session session, GoAwayFrame frame) + { + closeLatch.countDown(); + } + }); + + newClient(new Session.Listener.Adapter()); + + Thread.sleep(1000); + + client.stop(); + + Assert.assertTrue(closeLatch.await(5, TimeUnit.SECONDS)); + } } diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java index 5b9c8f773a4..48d5d9ce3ae 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java @@ -28,6 +28,7 @@ import org.eclipse.jetty.io.AbstractConnection; import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.ConcurrentArrayQueue; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; @@ -129,6 +130,14 @@ public class HTTP2Connection extends AbstractConnection executionStrategy.execute(); } + @Override + public void close() + { + // We don't call super from here, otherwise we close the + // endPoint and we're not able to read or write anymore. + session.close(ErrorCode.NO_ERROR.code, "close", Callback.NOOP); + } + protected class HTTP2Producer implements ExecutionStrategy.Producer { private ByteBuffer buffer; 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 ab4eb5f2392..17b6e0bbafd 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 @@ -537,8 +537,6 @@ public abstract class HTTP2Session implements ISession, Parser.Listener { byte[] payload = reason == null ? null : reason.getBytes(StandardCharsets.UTF_8); GoAwayFrame frame = new GoAwayFrame(lastStreamId.get(), error, payload); - if (LOG.isDebugEnabled()) - LOG.debug("Sending {}", frame); control(null, callback, frame); return true; }