Merge pull request #4702 from eclipse/jetty-10.0.x-4603-WS_HTTP2_NPE

Issue #4603 - avoid NPE during websocket HTTP/2 upgrade

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java

@@ -314,18 +314,25 @@ public boolean onStreamTimeout(Throwable failure)
         return transportCallback.onIdleTimeout(failure);
     }
 
+    /**
+     * @return true if error sent, false if upgraded or aborted.
+     */
     boolean prepareUpgrade()
     {
         HttpChannelOverHTTP2 channel = (HttpChannelOverHTTP2)stream.getAttachment();
         Request request = channel.getRequest();
         if (request.getHttpInput().hasContent())
             return channel.sendErrorOrAbort("Unexpected content in CONNECT request");
+
         Connection connection = (Connection)request.getAttribute(UPGRADE_CONNECTION_ATTRIBUTE);
+        if (connection == null)
+            return channel.sendErrorOrAbort("No UPGRADE_CONNECTION_ATTRIBUTE available");
+
         EndPoint endPoint = connection.getEndPoint();
         endPoint.upgrade(connection);
         stream.setAttachment(endPoint);
-        // Only now that we have switched the attachment,
-        // we can demand DATA frames to process them.
+
+        // Only now that we have switched the attachment, we can demand DATA frames to process them.
         stream.demand(1);
 
         if (LOG.isDebugEnabled())
@@ -340,21 +347,6 @@ public void onCompleted()
         Object attachment = stream.getAttachment();
         if (attachment instanceof HttpChannelOverHTTP2)
         {
-            // TODO: we used to "fake" a 101 response to upgrade the endpoint
-            //  but we don't anymore, so this code should be deleted.
-            HttpChannelOverHTTP2 channel = (HttpChannelOverHTTP2)attachment;
-            if (channel.getResponse().getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101)
-            {
-                Connection connection = (Connection)channel.getRequest().getAttribute(UPGRADE_CONNECTION_ATTRIBUTE);
-                EndPoint endPoint = connection.getEndPoint();
-                // TODO: check that endPoint implements HTTP2Channel.
-                if (LOG.isDebugEnabled())
-                    LOG.debug("Tunnelling DATA frames through {}", endPoint);
-                endPoint.upgrade(connection);
-                stream.setAttachment(endPoint);
-                return;
-            }
-
             // If the stream is not closed, it is still reading the request content.
             // Send a reset to the other end so that it stops sending data.
             if (!stream.isClosed())
@@ -366,6 +358,7 @@ public void onCompleted()
 
             // Consume the existing queued data frames to
             // avoid stalling the session flow control.
+            HttpChannelOverHTTP2 channel = (HttpChannelOverHTTP2)attachment;
             channel.consumeInput();
         }
     }

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java

@@ -490,7 +490,7 @@ public boolean handle()
                                 break;
                         }
 
-                        // Check if an update is done (if so, do not close)
+                        // If send error is called we need to break.
                         if (checkAndPrepareUpgrade())
                             break;
 
@@ -527,6 +527,10 @@ public boolean handle()
         return !suspended;
     }
 
+    /**
+     * @param message the error message.
+     * @return true if we have sent an error, false if we have aborted.
+     */
     public boolean sendErrorOrAbort(String message)
     {
         try

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java

@@ -479,7 +479,6 @@ private boolean upgrade() throws BadMessageException
         if (LOG.isDebugEnabled())
             LOG.debug("Upgrade from {} to {}", getEndPoint().getConnection(), upgradeConnection);
         getRequest().setAttribute(HttpTransport.UPGRADE_CONNECTION_ATTRIBUTE, upgradeConnection);
-        getResponse().setStatus(HttpStatus.SWITCHING_PROTOCOLS_101);
         getHttpTransport().onCompleted();
         return true;
     }

jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/server/internal/AbstractHandshaker.java

@@ -160,12 +160,12 @@ public boolean upgradeRequest(WebSocketNegotiator negotiator, HttpServletRequest
 
         coreSession.setWebSocketConnection(connection);
 
+        baseRequest.setHandled(true);
         Response baseResponse = baseRequest.getResponse();
         prepareResponse(baseResponse, negotiation);
         if (httpConfig.getSendServerVersion())
             baseResponse.getHttpFields().put(SERVER_VERSION);
         baseResponse.flushBuffer();
-        baseRequest.setHandled(true);
 
         baseRequest.setAttribute(HttpTransport.UPGRADE_CONNECTION_ATTRIBUTE, connection);
 

jetty-websocket/websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketOverHTTP2Test.java

@@ -23,6 +23,7 @@
 import java.net.ConnectException;
 import java.net.URI;
 import java.nio.channels.ClosedChannelException;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.function.Function;
@@ -294,11 +295,21 @@ public void testThrowFromCreator() throws Exception
         startServer();
         startClient(clientConnector -> new ClientConnectionFactoryOverHTTP2.H2(new HTTP2Client(clientConnector)));
 
+        CountDownLatch latch = new CountDownLatch(1);
+        connector.addBean(new HttpChannel.Listener()
+        {
+            @Override
+            public void onComplete(Request request)
+            {
+                latch.countDown();
+            }
+        });
+
         EventSocket wsEndPoint = new EventSocket();
         URI uri = URI.create("ws://localhost:" + connector.getLocalPort() + "/ws/throw");
 
         ExecutionException failure;
-        try (StacklessLogging stacklessLogging = new StacklessLogging(HttpChannel.class))
+        try (StacklessLogging ignored = new StacklessLogging(HttpChannel.class))
         {
             failure = Assertions.assertThrows(ExecutionException.class, () ->
                 wsClient.connect(wsEndPoint, uri).get(5, TimeUnit.SECONDS));
@@ -307,6 +318,9 @@ public void testThrowFromCreator() throws Exception
         Throwable cause = failure.getCause();
         assertThat(cause, instanceOf(UpgradeException.class));
         assertThat(cause.getMessage(), containsStringIgnoringCase("Unexpected HTTP Response Status Code: 500"));
+
+        // Wait for the request to complete on server before stopping.
+        assertTrue(latch.await(5, TimeUnit.SECONDS));
     }
 
     @Test