12.0.x: Fix loop caused by NegotiatingClientConnection talking to broken server

jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/BrokenServerNegotiationTest.java

@@ -0,0 +1,112 @@
+//
+// ========================================================================
+// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
+//
+// This program and the accompanying materials are made available under the
+// terms of the Eclipse Public License v. 2.0 which is available at
+// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
+// which is available at https://www.apache.org/licenses/LICENSE-2.0.
+//
+// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
+// ========================================================================
+//
+
+package org.eclipse.jetty.http2.tests;
+
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import javax.net.ssl.SSLHandshakeException;
+
+import org.eclipse.jetty.client.HttpClient;
+import org.eclipse.jetty.client.transport.HttpClientConnectionFactory;
+import org.eclipse.jetty.client.transport.HttpClientTransportDynamic;
+import org.eclipse.jetty.http2.client.HTTP2Client;
+import org.eclipse.jetty.http2.client.transport.ClientConnectionFactoryOverHTTP2;
+import org.eclipse.jetty.http2.client.transport.HttpClientTransportOverHTTP2;
+import org.eclipse.jetty.io.ClientConnectionFactory;
+import org.eclipse.jetty.io.ClientConnector;
+import org.eclipse.jetty.util.component.LifeCycle;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import static org.awaitility.Awaitility.await;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+public class BrokenServerNegotiationTest
+{
+    private volatile ServerSocket serverSocket;
+
+    public int startServer() throws Exception
+    {
+        if (serverSocket != null)
+            return serverSocket.getLocalPort();
+        Thread server = new Thread(() ->
+        {
+            try
+            {
+                serverSocket = new ServerSocket(0);
+                while (true)
+                {
+                    Socket accept = serverSocket.accept();
+                    accept.close();
+                }
+            }
+            catch (Exception e)
+            {
+                // ignore, time to shut down
+            }
+        });
+        server.start();
+        await().atMost(5, TimeUnit.SECONDS).until(() -> serverSocket, notNullValue());
+        return serverSocket.getLocalPort();
+    }
+
+    @AfterEach
+    public void stopServer() throws Exception
+    {
+        if (serverSocket != null)
+            serverSocket.close();
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = {false, true})
+    public void testAlpnNegotiation(boolean useAlpn) throws Exception
+    {
+        int port = startServer();
+        ClientConnector clientConnector = new ClientConnector();
+        HTTP2Client h2Client = new HTTP2Client();
+        h2Client.setUseALPN(useAlpn);
+        ClientConnectionFactoryOverHTTP2.HTTP2 http2 = new ClientConnectionFactoryOverHTTP2.HTTP2(h2Client);
+        ClientConnectionFactory.Info http1 = HttpClientConnectionFactory.HTTP11;
+        HttpClientTransportDynamic transportDynamic = new HttpClientTransportDynamic(clientConnector, http1,  http2);
+        HttpClient client = new HttpClient(transportDynamic);
+        client.start();
+
+        ExecutionException ee = assertThrows(ExecutionException.class, () -> client.GET("https://localhost:" + port + "/hello"));
+        assertInstanceOf(SSLHandshakeException.class, ee.getCause());
+
+        LifeCycle.stop(client);
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = {false, true})
+    public void testH2Only(boolean useAlpn) throws Exception
+    {
+        int port = startServer();
+        HTTP2Client h2Client = new HTTP2Client();
+        h2Client.setUseALPN(useAlpn);
+        HttpClientTransportOverHTTP2 httpClientTransportOverHTTP2 = new HttpClientTransportOverHTTP2(h2Client);
+        HttpClient client = new HttpClient(httpClientTransportOverHTTP2);
+        client.start();
+
+        ExecutionException ee = assertThrows(ExecutionException.class, () -> client.GET("https://localhost:" + port + "/hello"));
+        assertInstanceOf(SSLHandshakeException.class, ee.getCause());
+
+        LifeCycle.stop(client);
+    }
+}

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/NegotiatingClientConnection.java

@@ -18,6 +18,7 @@
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeoutException;
 import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLHandshakeException;
 
 import org.eclipse.jetty.util.BufferUtil;
 import org.eclipse.jetty.util.Promise;
@@ -55,7 +56,7 @@ public String getProtocol()
     protected void completed(String protocol)
     {
         this.protocol = protocol;
-        completed = true;
+        this.completed = true;
     }
 
     @Override
@@ -73,67 +74,53 @@ public void onOpen()
         catch (Throwable x)
         {
             close();
-            // TODO: should we not fail the promise in the context here?
-            throw new RuntimeIOException(x);
+            failConnectionPromise(x);
         }
     }
 
     @Override
     public void onFillable()
     {
-        while (true)
+        Throwable failure = null;
+        try
         {
-            int filled = fill();
-            if (completed || filled < 0)
+            while (true)
             {
-                replaceConnection();
-                break;
-            }
-            if (filled == 0)
-            {
-                fillInterested();
-                break;
+                int filled = getEndPoint().fill(BufferUtil.EMPTY_BUFFER);
+                if (completed)
+                {
+                    replaceConnection();
+                    break;
+                }
+                else if (filled < 0)
+                {
+                    throw new SSLHandshakeException("Abruptly closed by peer");
+                }
+                else if (filled == 0)
+                {
+                    fillInterested();
+                    break;
+                }
             }
         }
-    }
-
-    private int fill()
-    {
-        try
-        {
-            return getEndPoint().fill(BufferUtil.EMPTY_BUFFER);
-        }
-        catch (IOException x)
+        catch (Throwable x)
         {
-            LOG.debug("Unable to fill from endpoint", x);
-            close();
-            return -1;
+            failure = x;
         }
-    }
 
-    private void replaceConnection()
-    {
-        EndPoint endPoint = getEndPoint();
-        try
+        if (failure != null)
         {
-            endPoint.upgrade(connectionFactory.newConnection(endPoint, context));
-        }
-        catch (Throwable x)
-        {
-            LOG.debug("Unable to replace connection", x);
+            LOG.atDebug().setCause(failure).log("Unable to fill from endpoint");
             close();
+            failConnectionPromise(failure);
         }
     }
 
     @Override
     public boolean onIdleExpired(TimeoutException timeout)
     {
         getEndPoint().close(timeout);
-
-        @SuppressWarnings("unchecked")
-        Promise<Connection> promise = (Promise<Connection>)context.get(ClientConnector.CONNECTION_PROMISE_CONTEXT_KEY);
-        promise.failed(timeout);
-
+        failConnectionPromise(timeout);
         return false;
     }
 
@@ -144,4 +131,17 @@ public void close()
         getEndPoint().shutdownOutput();
         super.close();
     }
+
+    private void replaceConnection() throws IOException
+    {
+        EndPoint endPoint = getEndPoint();
+        endPoint.upgrade(connectionFactory.newConnection(endPoint, context));
+    }
+
+    private void failConnectionPromise(Throwable failure)
+    {
+        @SuppressWarnings("unchecked")
+        Promise<Connection> promise = (Promise<Connection>)context.get(ClientConnector.CONNECTION_PROMISE_CONTEXT_KEY);
+        promise.failed(failure);
+    }
 }