diff --git a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/BrokenServerNegotiationTest.java b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/BrokenServerNegotiationTest.java new file mode 100644 index 00000000000..ccecfb4a35f --- /dev/null +++ b/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); + } +} diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/NegotiatingClientConnection.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/NegotiatingClientConnection.java index c250baa4ee5..551968a86f7 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/NegotiatingClientConnection.java +++ b/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,55 +74,45 @@ 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); } } @@ -129,11 +120,7 @@ private void replaceConnection() public boolean onIdleExpired(TimeoutException timeout) { getEndPoint().close(timeout); - - @SuppressWarnings("unchecked") - Promise promise = (Promise)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 promise = (Promise)context.get(ClientConnector.CONNECTION_PROMISE_CONTEXT_KEY); + promise.failed(failure); + } }