Issue #4855 - Occasional h2spec failures on CI

Code cleanups. Signed-off-by: Simone Bordet <[email protected]>
jetty · Jun 5, 2020 · e721717 · e721717
1 parent eac4187
commit e721717
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 91 deletions.
diff --git a/...tp2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java b/...tp2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java
@@ -77,7 +77,7 @@ public Connection newConnection(EndPoint endPoint, Map<String, Object> context)
         return customize(connection, context);
     }
 
-    private class HTTP2ClientConnection extends HTTP2Connection implements Callback
+    private static class HTTP2ClientConnection extends HTTP2Connection implements Callback
     {
         private final HTTP2Client client;
         private final Promise<Session> promise;
@@ -154,7 +154,7 @@ public void failed(Throwable x)
         }
     }
 
-    private class ConnectionListener implements Connection.Listener
+    private static class ConnectionListener implements Connection.Listener
     {
         @Override
         public void onOpened(Connection connection)

diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/AbstractTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/AbstractTest.java
@@ -21,7 +21,6 @@
 import java.net.InetSocketAddress;
 import java.util.concurrent.TimeUnit;
 import java.util.function.Consumer;
-
 import javax.servlet.http.HttpServlet;
 
 import org.eclipse.jetty.http.HostPortHttpField;
@@ -33,7 +32,7 @@
 import org.eclipse.jetty.http2.api.Session;
 import org.eclipse.jetty.http2.api.server.ServerSessionListener;
 import org.eclipse.jetty.http2.server.AbstractHTTP2ServerConnectionFactory;
-import org.eclipse.jetty.http2.server.HTTP2ServerConnectionFactory;
+import org.eclipse.jetty.http2.server.HTTP2CServerConnectionFactory;
 import org.eclipse.jetty.http2.server.RawHTTP2ServerConnectionFactory;
 import org.eclipse.jetty.server.ConnectionFactory;
 import org.eclipse.jetty.server.HttpConfiguration;
@@ -54,7 +53,7 @@ public class AbstractTest
 
     protected void start(HttpServlet servlet) throws Exception
     {
-        HTTP2ServerConnectionFactory connectionFactory = new HTTP2ServerConnectionFactory(new HttpConfiguration());
+        HTTP2CServerConnectionFactory connectionFactory = new HTTP2CServerConnectionFactory(new HttpConfiguration());
         connectionFactory.setInitialSessionRecvWindow(FlowControlStrategy.DEFAULT_WINDOW_SIZE);
         connectionFactory.setInitialStreamRecvWindow(FlowControlStrategy.DEFAULT_WINDOW_SIZE);
         prepareServer(connectionFactory);

diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/InvalidServerTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/InvalidServerTest.java
diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/PrefaceTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/PrefaceTest.java
@@ -18,7 +18,11 @@
 
 package org.eclipse.jetty.http2.client;
 
+import java.io.InputStream;
+import java.io.OutputStream;
 import java.net.InetSocketAddress;
+import java.net.ServerSocket;
+import java.net.Socket;
 import java.nio.ByteBuffer;
 import java.nio.channels.SocketChannel;
 import java.nio.charset.StandardCharsets;
@@ -41,6 +45,7 @@
 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.FrameType;
 import org.eclipse.jetty.http2.frames.HeadersFrame;
 import org.eclipse.jetty.http2.frames.PingFrame;
 import org.eclipse.jetty.http2.frames.PrefaceFrame;
@@ -63,6 +68,7 @@
 import static org.hamcrest.Matchers.greaterThanOrEqualTo;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class PrefaceTest extends AbstractTest
@@ -332,4 +338,71 @@ public void onHeaders(HeadersFrame frame)
             assertTrue(clientSettingsLatch.await(5, TimeUnit.SECONDS));
         }
     }
+
+    @Test
+    public void testInvalidServerPreface() throws Exception
+    {
+        try (ServerSocket server = new ServerSocket(0))
+        {
+            prepareClient();
+            client.start();
+
+            CountDownLatch failureLatch = new CountDownLatch(1);
+            Promise.Completable<Session> promise = new Promise.Completable<>();
+            InetSocketAddress address = new InetSocketAddress("localhost", server.getLocalPort());
+            client.connect(address, new Session.Listener.Adapter()
+            {
+                @Override
+                public void onFailure(Session session, Throwable failure)
+                {
+                    failureLatch.countDown();
+                }
+            }, promise);
+
+            try (Socket socket = server.accept())
+            {
+                OutputStream output = socket.getOutputStream();
+                output.write("enough_junk_bytes".getBytes(StandardCharsets.UTF_8));
+
+                Session session = promise.get(5, TimeUnit.SECONDS);
+                assertNotNull(session);
+
+                assertTrue(failureLatch.await(5, TimeUnit.SECONDS));
+
+                // Verify that the client closed the socket.
+                InputStream input = socket.getInputStream();
+                while (true)
+                {
+                    int read = input.read();
+                    if (read < 0)
+                        break;
+                }
+            }
+        }
+    }
+
+    @Test
+    public void testInvalidClientPreface() throws Exception
+    {
+        start(new ServerSessionListener.Adapter());
+
+        try (Socket client = new Socket("localhost", connector.getLocalPort()))
+        {
+            OutputStream output = client.getOutputStream();
+            output.write("enough_junk_bytes".getBytes(StandardCharsets.UTF_8));
+            output.flush();
+
+            byte[] bytes = new byte[1024];
+            InputStream input = client.getInputStream();
+            int read = input.read(bytes);
+            if (read < 0)
+            {
+                // Closing the connection without GOAWAY frame is fine.
+                return;
+            }
+
+            int type = bytes[3];
+            assertEquals(FrameType.GO_AWAY.getType(), type);
+        }
+    }
 }
diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Flusher.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Flusher.java
@@ -425,12 +425,21 @@ public void failed(Throwable x)
             super.failed(x);
         }
 
+        /**
+         * @return whether the entry is stale and must not be processed
+         */
         private boolean isStale()
         {
-            return !isProtocol() && stream != null && stream.isReset();
+            // If it is a protocol frame, process it.
+            if (isProtocolFrame(frame))
+                return false;
+            // It's an application frame; is the stream gone already?
+            if (stream == null)
+                return true;
+            return stream.isReset();
         }
 
-        private boolean isProtocol()
+        private boolean isProtocolFrame(Frame frame)
         {
             switch (frame.getType())
             {

diff --git a/...ttp2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnection.java b/...ttp2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnection.java
@@ -214,7 +214,7 @@ public boolean onStreamTimeout(IStream stream, Throwable failure)
     public void onStreamFailure(IStream stream, Throwable failure, Callback callback)
     {
         if (LOG.isDebugEnabled())
-            LOG.debug("Processing failure on {}: {}", stream, failure);
+            LOG.debug("Processing stream failure on {}", stream, failure);
         HttpChannelOverHTTP2 channel = (HttpChannelOverHTTP2)stream.getAttachment();
         if (channel != null)
         {

diff --git a/...tp2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnectionFactory.java b/...tp2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnectionFactory.java
@@ -119,7 +119,8 @@ public void onClose(Session session, GoAwayFrame frame, Callback callback)
             String reason = frame.tryConvertPayload();
             if (!StringUtil.isEmpty(reason))
                 reason = " (" + reason + ")";
-            getConnection().onSessionFailure(new EofException(String.format("Close %s/%s", ErrorCode.toString(frame.getError(), null), reason)), callback);
+            EofException failure = new EofException(String.format("Close %s/%s", ErrorCode.toString(frame.getError(), null), reason));
+            onFailure(session, failure, callback);
         }
 
         @Override
@@ -154,13 +155,20 @@ public void onData(Stream stream, DataFrame frame, Callback callback)
         @Override
         public void onReset(Stream stream, ResetFrame frame, Callback callback)
         {
-            getConnection().onStreamFailure((IStream)stream, new EofException("Reset " + ErrorCode.toString(frame.getError(), null)), callback);
+            EofException failure = new EofException("Reset " + ErrorCode.toString(frame.getError(), null));
+            onFailure(stream, failure, callback);
         }
 
         @Override
         public void onFailure(Stream stream, int error, String reason, Callback callback)
         {
-            getConnection().onStreamFailure((IStream)stream, new EofException(String.format("Failure %s/%s", ErrorCode.toString(error, null), reason)), callback);
+            EofException failure = new EofException(String.format("Failure %s/%s", ErrorCode.toString(error, null), reason));
+            onFailure(stream, failure, callback);
+        }
+
+        private void onFailure(Stream stream, Throwable failure, Callback callback)
+        {
+            getConnection().onStreamFailure((IStream)stream, failure, callback);
         }
 
         @Override